mirrored from git://develop.git.wordpress.org/
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTML API: Ensure that full processor can seek to earlier bookmarks #7649
Closed
Closed
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
f9777aa
Add failing test case
sirreal 5d75bb7
Work on fix
sirreal 541b4f6
wip
sirreal dc3f1e6
try fixes
sirreal 89aa01b
Fix the issue
sirreal 737bf92
Restore original bookmark matching
sirreal cb63bbc
Lint: remove empty line
sirreal 744cf01
Add commentary around state resetting in seek
sirreal 6106a56
Use do-while loop to iterate after first check
sirreal 851df38
Fix fragment parser bug that should reset breadcrumbs before seeking
sirreal 9af204c
Assert setting bookmark two
sirreal 660dc85
Remove condition that may not be necessary
sirreal 9a0c017
Set parsing namespace correctly when seeking
sirreal d181938
Fix fragment state reset for different context nodes
sirreal d5bf14c
Add and improve comments
sirreal 388bf19
Add and improve HTML processor bookmark tests
sirreal f7af6e3
Add test seeking from SVG namespace
sirreal d5a7d5c
Rework with tests for fragment and full processors
sirreal b95e402
Restore private stack properties to private
sirreal 90eb6e2
Use a temporary bookmark to avoid modifying tag processor internal state
sirreal 75ab9c2
Break at root-node when popping elements on seek
sirreal 05ca2a4
Remove context-node cases from open elements
sirreal 1cde425
Merge branch 'trunk' into html-api/fix-63390-seek
sirreal c48be70
Add test for root-node regression
sirreal 710c5f7
Fix root-node pop regression in fragment parser seek
sirreal 0efe691
Merge branch 'trunk' into html-api/fix-63390-seek
sirreal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
160 changes: 160 additions & 0 deletions
160
tests/phpunit/tests/html-api/wpHtmlProcessor-bookmark.php
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,160 @@ | ||
<?php | ||
/** | ||
* Unit tests covering WP_HTML_Processor bookmark functionality. | ||
* | ||
* @package WordPress | ||
* @subpackage HTML-API | ||
*/ | ||
|
||
/** | ||
* @group html-api | ||
* | ||
* @coversDefaultClass WP_HTML_Processor | ||
*/ | ||
class Tests_HtmlApi_WpHtmlProcessor_Bookmark extends WP_UnitTestCase { | ||
/** | ||
* @dataProvider data_processor_constructors | ||
* | ||
* @ticket 62290 | ||
*/ | ||
public function test_processor_seek_same_location( callable $factory ) { | ||
$processor = $factory( '<div><span>' ); | ||
$this->assertTrue( $processor->next_tag( 'DIV' ) ); | ||
$this->assertTrue( $processor->set_bookmark( 'mark' ), 'Failed to set bookmark.' ); | ||
$this->assertTrue( $processor->has_bookmark( 'mark' ), 'Failed has_bookmark check.' ); | ||
|
||
// Confirm the bookmark works and processing continues normally. | ||
$this->assertTrue( $processor->seek( 'mark' ), 'Failed to seek to bookmark.' ); | ||
$this->assertSame( 'DIV', $processor->get_tag() ); | ||
$this->assertSame( array( 'HTML', 'BODY', 'DIV' ), $processor->get_breadcrumbs() ); | ||
$this->assertTrue( $processor->next_tag() ); | ||
$this->assertSame( 'SPAN', $processor->get_tag() ); | ||
$this->assertSame( array( 'HTML', 'BODY', 'DIV', 'SPAN' ), $processor->get_breadcrumbs() ); | ||
} | ||
|
||
/** | ||
* @dataProvider data_processor_constructors | ||
* | ||
* @ticket 62290 | ||
*/ | ||
public function test_processor_seek_backward( callable $factory ) { | ||
$processor = $factory( '<div><span>' ); | ||
$this->assertTrue( $processor->next_tag( 'DIV' ) ); | ||
$this->assertTrue( $processor->set_bookmark( 'mark' ), 'Failed to set bookmark.' ); | ||
$this->assertTrue( $processor->has_bookmark( 'mark' ), 'Failed has_bookmark check.' ); | ||
|
||
// Move past the bookmark so it must scan backwards. | ||
$this->assertTrue( $processor->next_tag( 'SPAN' ) ); | ||
|
||
// Confirm the bookmark works. | ||
$this->assertTrue( $processor->seek( 'mark' ), 'Failed to seek to bookmark.' ); | ||
$this->assertSame( 'DIV', $processor->get_tag() ); | ||
} | ||
|
||
/** | ||
* @dataProvider data_processor_constructors | ||
* | ||
* @ticket 62290 | ||
*/ | ||
public function test_processor_seek_forward( callable $factory ) { | ||
$processor = $factory( '<div one></div><span two></span><a three>' ); | ||
$this->assertTrue( $processor->next_tag( 'DIV' ) ); | ||
$this->assertTrue( $processor->set_bookmark( 'one' ), 'Failed to set bookmark "one".' ); | ||
$this->assertTrue( $processor->has_bookmark( 'one' ), 'Failed "one" has_bookmark check.' ); | ||
|
||
// Move past the bookmark so it must scan backwards. | ||
$this->assertTrue( $processor->next_tag( 'SPAN' ) ); | ||
$this->assertTrue( $processor->get_attribute( 'two' ) ); | ||
$this->assertTrue( $processor->set_bookmark( 'two' ), 'Failed to set bookmark "two".' ); | ||
$this->assertTrue( $processor->has_bookmark( 'two' ), 'Failed "two" has_bookmark check.' ); | ||
|
||
// Seek back. | ||
$this->assertTrue( $processor->seek( 'one' ), 'Failed to seek to bookmark "one".' ); | ||
$this->assertSame( 'DIV', $processor->get_tag() ); | ||
|
||
// Seek forward and continue processing. | ||
$this->assertTrue( $processor->seek( 'two' ), 'Failed to seek to bookmark "two".' ); | ||
$this->assertSame( 'SPAN', $processor->get_tag() ); | ||
$this->assertTrue( $processor->get_attribute( 'two' ) ); | ||
|
||
$this->assertTrue( $processor->next_tag() ); | ||
$this->assertSame( 'A', $processor->get_tag() ); | ||
$this->assertTrue( $processor->get_attribute( 'three' ) ); | ||
} | ||
|
||
/** | ||
* Ensure the parsing namespace is handled when seeking from foreign content. | ||
* | ||
* @dataProvider data_processor_constructors | ||
* | ||
* @ticket 62290 | ||
*/ | ||
public function test_seek_back_from_foreign_content( callable $factory ) { | ||
$processor = $factory( '<custom-element /><svg><rect />' ); | ||
$this->assertTrue( $processor->next_tag( 'CUSTOM-ELEMENT' ) ); | ||
$this->assertTrue( $processor->set_bookmark( 'mark' ), 'Failed to set bookmark "mark".' ); | ||
$this->assertTrue( $processor->has_bookmark( 'mark' ), 'Failed "mark" has_bookmark check.' ); | ||
|
||
/* | ||
* <custom-element /> has self-closing flag, but HTML elements (that are not void elements) cannot self-close, | ||
* they must be closed by some means, usually a closing tag. | ||
* | ||
* If the div were interpreted as foreign content, it would self-close. | ||
*/ | ||
$this->assertTrue( $processor->has_self_closing_flag() ); | ||
$this->assertTrue( $processor->expects_closer(), 'Incorrectly interpreted HTML custom-element with self-closing flag as self-closing element.' ); | ||
|
||
// Proceed into foreign content. | ||
$this->assertTrue( $processor->next_tag( 'RECT' ) ); | ||
$this->assertSame( 'svg', $processor->get_namespace() ); | ||
$this->assertTrue( $processor->has_self_closing_flag() ); | ||
$this->assertFalse( $processor->expects_closer() ); | ||
$this->assertSame( array( 'HTML', 'BODY', 'CUSTOM-ELEMENT', 'SVG', 'RECT' ), $processor->get_breadcrumbs() ); | ||
|
||
// Seek back. | ||
$this->assertTrue( $processor->seek( 'mark' ), 'Failed to seek to bookmark "mark".' ); | ||
$this->assertSame( 'CUSTOM-ELEMENT', $processor->get_tag() ); | ||
// If the parsing namespace were not correct here (html), | ||
// then the self-closing flag would be misinterpreted. | ||
$this->assertTrue( $processor->has_self_closing_flag() ); | ||
$this->assertTrue( $processor->expects_closer(), 'Incorrectly interpreted HTML custom-element with self-closing flag as self-closing element.' ); | ||
|
||
// Proceed into foreign content again. | ||
$this->assertTrue( $processor->next_tag( 'RECT' ) ); | ||
$this->assertSame( 'svg', $processor->get_namespace() ); | ||
$this->assertTrue( $processor->has_self_closing_flag() ); | ||
$this->assertFalse( $processor->expects_closer() ); | ||
|
||
// The RECT should still descend from the CUSTOM-ELEMENT despite its self-closing flag. | ||
$this->assertSame( array( 'HTML', 'BODY', 'CUSTOM-ELEMENT', 'SVG', 'RECT' ), $processor->get_breadcrumbs() ); | ||
} | ||
|
||
/** | ||
* Covers a regression where the root node may not be present on the stack of open elements. | ||
* | ||
* Heading elements (h1, h2, etc.) check the current node on the stack of open elements | ||
* and expect it to be defined. If the root-node has been popped, pushing a new heading | ||
* onto the stack will create a warning and fail the test. | ||
* | ||
* @ticket 62290 | ||
*/ | ||
public function test_fragment_starts_with_h1() { | ||
$processor = WP_HTML_Processor::create_fragment( '<h1>' ); | ||
$this->assertTrue( $processor->next_tag( 'H1' ) ); | ||
$this->assertTrue( $processor->set_bookmark( 'mark' ) ); | ||
$this->assertTrue( $processor->next_token() ); | ||
$this->assertTrue( $processor->seek( 'mark' ) ); | ||
} | ||
|
||
/** | ||
* Data provider. | ||
* | ||
* @return array | ||
*/ | ||
public static function data_processor_constructors(): array { | ||
return array( | ||
'Full parser' => array( array( WP_HTML_Processor::class, 'create_full_parser' ) ), | ||
'Fragment parser' => array( array( WP_HTML_Processor::class, 'create_fragment' ) ), | ||
); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add a
reset()
method toWP_HTML_Processor_State
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that but ultimately decided against it. This isn't really a full reset, and the reset behaves a bit differently between the fragment/full processors.
To simplify this, I'd probably create a new state instance and copy over the important properties. That may be worth exploring but that felt like a larger refactor.