HTML API: Ensure that full processor can seek to earlier bookmarks #7649

sirreal · 2024-10-25T21:16:59Z

#62290 describes an issue where the full processor cannot seek to earlier bookmarks.

When the processor seeks to an earlier place, it returns the the beginning of the document and proceeds forward until it reaches the appropriate location. This requires resetting internal state so that the processor can correctly proceed from the beginning of the document.

The seeking reset logic was not adapted to account for the full processor. This change updates the seek logic to account for the full and fragment parsers as well as other state that has been introduced in the interim and should be reset.

Trac ticket: https://core.trac.wordpress.org/ticket/62290

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

github-actions · 2024-10-25T21:32:23Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

This is consisten with the assertion on bookmark one

This seems related to virutal tokens and it likely covered by the virtual token condition

sirreal · 2024-11-06T11:08:17Z

There are a lot of parallels with #7348 where the state should be reset appropriately according to the context node.

sirreal · 2024-11-06T11:20:32Z

src/wp-includes/html-api/class-wp-html-processor.php

+				$this->change_parsing_namespace(
+					$this->context_node->integration_node_type
+						? 'html'
+						: $this->context_node->namespace
+				);
+
+				if ( 'TEMPLATE' === $this->context_node->node_name ) {
+					$this->state->stack_of_template_insertion_modes[] = WP_HTML_Processor_State::INSERTION_MODE_IN_TEMPLATE;
+				}

-		// When moving forwards, reparse the document until reaching the same location as the original bookmark.
-		if ( $bookmark_starts_at === $this->bookmarks[ $this->state->current_token->bookmark_name ]->start ) {
-			return true;
+				$this->reset_insertion_mode_appropriately();


This logic is inspired by #7348 where a fragment parser is created on arbitrary context nodes and must be initialized properly.

This is very similar, the state must be reset to the appropriate initial state for the context.

This follows the fragment parsing algorithm: https://html.spec.whatwg.org/multipage/parsing.html#html-fragment-parsing-algorithm

Use a dataprovider to get a factory function for processors

src/wp-includes/html-api/class-wp-html-processor.php

github-actions · 2024-11-06T13:01:44Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, dmsnell.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

sirreal · 2024-11-06T13:01:49Z

This is ready for review @gziolo @westonruter @dmsnell.

src/wp-includes/html-api/class-wp-html-processor.php

The context node is not pushed to the stack of open elements, this code was either doing nothing or incorrect.

The context node is not pushed to the stack of open elements, and therefore does not need special handling.

Squashed commit of the following: commit 1cde425 Merge: 05ca2a4 8ad5281 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 20:40:05 2024 +0100 Merge branch 'trunk' into html-api/fix-63390-seek commit 05ca2a4 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 20:29:36 2024 +0100 Remove context-node cases from open elements The context node is not pushed to the stack of open elements, and therefore does not need special handling. commit 75ab9c2 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 20:27:57 2024 +0100 Break at root-node when popping elements on seek The context node is not pushed to the stack of open elements, this code was either doing nothing or incorrect. commit 90eb6e2 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:53:15 2024 +0100 Use a temporary bookmark to avoid modifying tag processor internal state commit b95e402 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:45:13 2024 +0100 Restore private stack properties to private commit d5a7d5c Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:40:18 2024 +0100 Rework with tests for fragment and full processors Use a dataprovider to get a factory function for processors commit f7af6e3 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:19:58 2024 +0100 Add test seeking from SVG namespace commit 388bf19 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:05:34 2024 +0100 Add and improve HTML processor bookmark tests commit d5bf14c Author: Jon Surrell <[email protected]> Date: Wed Nov 6 12:17:47 2024 +0100 Add and improve comments commit d181938 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 12:13:48 2024 +0100 Fix fragment state reset for different context nodes commit 9a0c017 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 12:05:43 2024 +0100 Set parsing namespace correctly when seeking commit 660dc85 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:49:21 2024 +0100 Remove condition that may not be necessary This seems related to virutal tokens and it likely covered by the virtual token condition commit 9af204c Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:47:56 2024 +0100 Assert setting bookmark two This is consisten with the assertion on bookmark one commit 851df38 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:47:17 2024 +0100 Fix fragment parser bug that should reset breadcrumbs before seeking commit 6106a56 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:46:49 2024 +0100 Use do-while loop to iterate after first check commit 744cf01 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:42:42 2024 +0100 Add commentary around state resetting in seek commit cb63bbc Author: Jon Surrell <[email protected]> Date: Tue Nov 5 19:51:29 2024 +0100 Lint: remove empty line commit 737bf92 Author: Jon Surrell <[email protected]> Date: Tue Nov 5 19:49:01 2024 +0100 Restore original bookmark matching commit 89aa01b Author: Jon Surrell <[email protected]> Date: Tue Nov 5 19:47:16 2024 +0100 Fix the issue commit dc3f1e6 Author: Jon Surrell <[email protected]> Date: Tue Nov 5 19:06:23 2024 +0100 try fixes commit 541b4f6 Author: Jon Surrell <[email protected]> Date: Fri Oct 25 23:19:19 2024 +0200 wip commit 5d75bb7 Author: Jon Surrell <[email protected]> Date: Fri Oct 25 23:15:51 2024 +0200 Work on fix commit f9777aa Author: Jon Surrell <[email protected]> Date: Fri Oct 25 22:51:44 2024 +0200 Add failing test case

sirreal · 2024-11-08T11:14:58Z

@mi5t4n you were active on https://core.trac.wordpress.org/ticket/62290, if you could provide testing for this change it would help move things along 🙏

Squashed commit of the following: commit 1cde425 Merge: 05ca2a4 8ad5281 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 20:40:05 2024 +0100 Merge branch 'trunk' into html-api/fix-63390-seek commit 05ca2a4 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 20:29:36 2024 +0100 Remove context-node cases from open elements The context node is not pushed to the stack of open elements, and therefore does not need special handling. commit 75ab9c2 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 20:27:57 2024 +0100 Break at root-node when popping elements on seek The context node is not pushed to the stack of open elements, this code was either doing nothing or incorrect. commit 90eb6e2 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:53:15 2024 +0100 Use a temporary bookmark to avoid modifying tag processor internal state commit b95e402 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:45:13 2024 +0100 Restore private stack properties to private commit d5a7d5c Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:40:18 2024 +0100 Rework with tests for fragment and full processors Use a dataprovider to get a factory function for processors commit f7af6e3 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:19:58 2024 +0100 Add test seeking from SVG namespace commit 388bf19 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 13:05:34 2024 +0100 Add and improve HTML processor bookmark tests commit d5bf14c Author: Jon Surrell <[email protected]> Date: Wed Nov 6 12:17:47 2024 +0100 Add and improve comments commit d181938 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 12:13:48 2024 +0100 Fix fragment state reset for different context nodes commit 9a0c017 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 12:05:43 2024 +0100 Set parsing namespace correctly when seeking commit 660dc85 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:49:21 2024 +0100 Remove condition that may not be necessary This seems related to virutal tokens and it likely covered by the virtual token condition commit 9af204c Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:47:56 2024 +0100 Assert setting bookmark two This is consisten with the assertion on bookmark one commit 851df38 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:47:17 2024 +0100 Fix fragment parser bug that should reset breadcrumbs before seeking commit 6106a56 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:46:49 2024 +0100 Use do-while loop to iterate after first check commit 744cf01 Author: Jon Surrell <[email protected]> Date: Wed Nov 6 11:42:42 2024 +0100 Add commentary around state resetting in seek commit cb63bbc Author: Jon Surrell <[email protected]> Date: Tue Nov 5 19:51:29 2024 +0100 Lint: remove empty line commit 737bf92 Author: Jon Surrell <[email protected]> Date: Tue Nov 5 19:49:01 2024 +0100 Restore original bookmark matching commit 89aa01b Author: Jon Surrell <[email protected]> Date: Tue Nov 5 19:47:16 2024 +0100 Fix the issue commit dc3f1e6 Author: Jon Surrell <[email protected]> Date: Tue Nov 5 19:06:23 2024 +0100 try fixes commit 541b4f6 Author: Jon Surrell <[email protected]> Date: Fri Oct 25 23:19:19 2024 +0200 wip commit 5d75bb7 Author: Jon Surrell <[email protected]> Date: Fri Oct 25 23:15:51 2024 +0200 Work on fix commit f9777aa Author: Jon Surrell <[email protected]> Date: Fri Oct 25 22:51:44 2024 +0200 Add failing test case

sirreal · 2024-11-11T19:30:26Z

Testing in #7742 discovered an issue present on trunk where the fragment parser root-node may not be present on the stack of open elements in a fragment parser.

I've pushed a test and a fix for that to this PR.

ockham · 2024-11-12T09:42:34Z

src/wp-includes/html-api/class-wp-html-processor.php

+			$this->state->frameset_ok                       = true;
+			$this->state->stack_of_template_insertion_modes = array();
+			$this->state->head_element                      = null;
+			$this->state->form_element                      = null;
+			$this->state->current_token                     = null;


Would it make sense to add a reset() method to WP_HTML_Processor_State?

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.

ockham

This is looking good to me! Thank you for the helpful inline comments and unit test coverage 👍

I'm going to commit this later today (unless someone else beats me to it).

ockham · 2024-11-12T10:31:10Z

Committed to Core trunk in https://core.trac.wordpress.org/changeset/59391.

sirreal changed the title ~~html api/fix 63390 seek~~ html api/fix 62290 seek Oct 25, 2024

sirreal added 4 commits November 5, 2024 19:23

Add failing test case

f9777aa

Work on fix

5d75bb7

wip

541b4f6

try fixes

dc3f1e6

sirreal force-pushed the html-api/fix-63390-seek branch from fb385b5 to dc3f1e6 Compare November 5, 2024 18:23

sirreal added 8 commits November 5, 2024 19:47

Fix the issue

89aa01b

Restore original bookmark matching

737bf92

Lint: remove empty line

cb63bbc

Add commentary around state resetting in seek

744cf01

Use do-while loop to iterate after first check

6106a56

Fix fragment parser bug that should reset breadcrumbs before seeking

851df38

Assert setting bookmark two

9af204c

This is consisten with the assertion on bookmark one

Remove condition that may not be necessary

660dc85

This seems related to virutal tokens and it likely covered by the virtual token condition

sirreal changed the title ~~html api/fix 62290 seek~~ HTML API: Ensure that full processor can seek to earlier bookmarks Nov 6, 2024

Set parsing namespace correctly when seeking

9a0c017

sirreal added 2 commits November 6, 2024 12:13

Fix fragment state reset for different context nodes

d181938

Add and improve comments

d5bf14c

sirreal commented Nov 6, 2024

View reviewed changes

sirreal added 4 commits November 6, 2024 13:05

Add and improve HTML processor bookmark tests

388bf19

Add test seeking from SVG namespace

f7af6e3

Rework with tests for fragment and full processors

d5a7d5c

Use a dataprovider to get a factory function for processors

Restore private stack properties to private

b95e402

sirreal force-pushed the html-api/fix-63390-seek branch from b1a6c00 to b95e402 Compare November 6, 2024 12:45

Use a temporary bookmark to avoid modifying tag processor internal state

90eb6e2

sirreal commented Nov 6, 2024

View reviewed changes

src/wp-includes/html-api/class-wp-html-processor.php Show resolved Hide resolved

sirreal marked this pull request as ready for review November 6, 2024 13:01

dmsnell reviewed Nov 6, 2024

View reviewed changes

src/wp-includes/html-api/class-wp-html-processor.php Outdated Show resolved Hide resolved

src/wp-includes/html-api/class-wp-html-processor.php Show resolved Hide resolved

sirreal added 2 commits November 6, 2024 20:27

Break at root-node when popping elements on seek

75ab9c2

The context node is not pushed to the stack of open elements, this code was either doing nothing or incorrect.

Remove context-node cases from open elements

05ca2a4

The context node is not pushed to the stack of open elements, and therefore does not need special handling.

sirreal requested a review from dmsnell November 6, 2024 19:37

Merge branch 'trunk' into html-api/fix-63390-seek

1cde425

sirreal mentioned this pull request Nov 7, 2024

Add advanced fragment handling sirreal/html-api-debugger#3

Merged

sirreal added 2 commits November 11, 2024 20:26

Add test for root-node regression

c48be70

Fix root-node pop regression in fragment parser seek

710c5f7

Merge branch 'trunk' into html-api/fix-63390-seek

0efe691

ockham reviewed Nov 12, 2024

View reviewed changes

ockham approved these changes Nov 12, 2024

View reviewed changes

ockham closed this Nov 12, 2024

sirreal deleted the html-api/fix-63390-seek branch November 12, 2024 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML API: Ensure that full processor can seek to earlier bookmarks #7649

HTML API: Ensure that full processor can seek to earlier bookmarks #7649

sirreal commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024

sirreal commented Nov 6, 2024

sirreal Nov 6, 2024

github-actions bot commented Nov 6, 2024 •

edited

Loading

sirreal commented Nov 6, 2024

sirreal commented Nov 8, 2024

sirreal commented Nov 11, 2024

ockham Nov 12, 2024

sirreal Nov 12, 2024

ockham left a comment

ockham commented Nov 12, 2024

HTML API: Ensure that full processor can seek to earlier bookmarks #7649

HTML API: Ensure that full processor can seek to earlier bookmarks #7649

Conversation

sirreal commented Oct 25, 2024 • edited Loading

github-actions bot commented Oct 25, 2024

Test using WordPress Playground

Some things to be aware of

sirreal commented Nov 6, 2024

sirreal Nov 6, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2024 • edited Loading

sirreal commented Nov 6, 2024

sirreal commented Nov 8, 2024

sirreal commented Nov 11, 2024

ockham Nov 12, 2024

Choose a reason for hiding this comment

sirreal Nov 12, 2024

Choose a reason for hiding this comment

ockham left a comment

Choose a reason for hiding this comment

ockham commented Nov 12, 2024

sirreal commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Nov 6, 2024 •

edited

Loading