HTML API: Implement reset_insertion_mode #6020

sirreal · 2024-02-02T15:16:19Z

See HTML Spec: Add an implementation and tests for "reset the insertion mode appropriately".

This unblocks work to support more tags:

The HTML standard describes an algorithm to "reset the insertion mode appropriately" that will be necessary for the HTML API to support a number of different tags such as TABLE or SELECT.
An implementation should be added so that the HTML Processor can continue to add tag support.

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-02-02T15:16:40Z

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.

github-actions · 2024-02-02T15:35:53Z

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 allows it to be better tested via public properties.

tests/phpunit/tests/html-api/wpHtmlProcessorState.php

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

dmsnell · 2024-07-02T17:52:31Z

tests/phpunit/tests/html-api/wpHtmlProcessorState.php

+
+		foreach ( $stack_of_open_elements as $i => $tag_name ) {
+			if ( ! ctype_upper( $tag_name ) ) {
+				throw new Error( 'Expected upper case tag names.' );


I don't like this error message. for one, it's testing the wrong thing in the wrong test. for two, it doesn't help explain why this test is failing.

what's the purpose of this? is it to get around markers and text nodes and comments, etc…?

dmsnell

@sirreal I really wanted to push this out but I still have some concerns. I think that the code is wrong, and I've left two test cases failing because I think that they should be asserting different results than they are, and it hinges on how the context node is handled.

I tried running some JS code to force the context node in a fragment parser but I ended up getting confused.

with all this being said it brings me back to some doubt about how we're testing the internals and not the public interface. I started moving the reset_insertion_mode() method into the HTML Processor, as I feel like it belongs there, but then I realized that makes testing it really difficult, as you can't set it up without the rest of the HTML Processor.

how did you establish your test cases? how do we know they are correct? I'm a bit worried that some of the test cases are setting themselves up in ways that I don't think we should ever get into in real code.

As the HTML Processor starts to support other insertion modes outside of "IN BODY" it needs to be aware of those other modes. This patch introduces the missing insertion modes in preparation for adding that support. Extracted as necessary prep work to the following more complete change: WordPress#6020 Props jonsurrell. See #61549.

As the HTML Processor starts to support other insertion modes outside of "IN BODY" it needs to be aware of those other modes. This patch introduces the missing insertion modes in preparation for adding that support. Extracted as necessary prep work to the following more complete change: #6020 Props jonsurrell. See #61549. git-svn-id: https://develop.svn.wordpress.org/trunk@58631 602fd350-edb4-49c9-b593-d223f7449a82

As the HTML Processor starts to support other insertion modes outside of "IN BODY" it needs to be aware of those other modes. This patch introduces the missing insertion modes in preparation for adding that support. Extracted as necessary prep work to the following more complete change: WordPress/wordpress-develop#6020 Props jonsurrell. See #61549. Built from https://develop.svn.wordpress.org/trunk@58631 git-svn-id: http://core.svn.wordpress.org/trunk@58060 1a063a9b-81f0-0310-95a4-ce76da25c4cd

dmsnell · 2024-07-02T21:50:08Z

@sirreal I've merged in trunk after adding the new insertion modes. I hope that's helpful, as it was clouding the diff for me.

As the HTML Processor starts to support other insertion modes outside of "IN BODY" it needs to be aware of those other modes. This patch introduces the missing insertion modes in preparation for adding that support. Extracted as necessary prep work to the following more complete change: WordPress/wordpress-develop#6020 Props jonsurrell. See #61549. Built from https://develop.svn.wordpress.org/trunk@58631 git-svn-id: https://core.svn.wordpress.org/trunk@58060 1a063a9b-81f0-0310-95a4-ce76da25c4cd

sirreal · 2024-07-03T15:09:39Z

We engaged in some discussion in Core Slack.

sirreal · 2024-07-03T15:13:16Z

At this point I feel pretty good about the implementation and whatever direction you want to take it.

@dmsnell I think you wanted to move it to the HTML Processor, that's fine with me. That's where I put it originally and only moved it so it was easily testable. I'm fine with removing the internals tests and moving the implementation 👍

sirreal · 2024-07-03T15:50:35Z

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

+			if ( $node === $first_node ) {
+				$last = true;
+				if ( isset( $this->context_node ) ) {
+					$node = new WP_HTML_Token( 'context-node', $this->context_node[0], false );


In this class, we use the context_node directly:

Suggested change

$node = new WP_HTML_Token( 'context-node', $this->context_node[0], false );

$node = $this->context_node;

dmsnell · 2024-07-03T16:40:09Z

Noting that because of the inherent challenges in testing this change, we're going to lean on the existing html5lib test suite, and new tests introduced with additional tag support in order to verify the behavior of the implementation.

The challenge is that this is an internal implementation and it's difficult to ascertain what the tests should be verifying. The test cases are unclear, and testing it in isolation doesn't bear a direct impact on whether the HTML API is working properly or not.

Therefore in order to avoid accidentally encoding incorrect behaviors in a unit test suite on an internal details, and to avoid architecting our code around the ability to test such internal details, we're foregoing specific new tests for this code.

In order to add support for the SELECT and TABLE tags in the HTML Processor, it needs to implement the HTML algorithm named "reset the insertion mode appropriately". This patch implements that algorithm to unblock the additional tag support. The algorithm resets the parsing mode after specific state changes in complicated situations where alternative rules are in effect (such as rules governing how the parser handles tags found within a TABLE element). Developed in #6020 Discussed in https://core.trac.wordpress.org/ticket/61549 Props dmsnell, jonsurrell. Fixes #61549. git-svn-id: https://develop.svn.wordpress.org/trunk@58656 602fd350-edb4-49c9-b593-d223f7449a82

dmsnell · 2024-07-03T17:07:15Z

Merged in [58656]
28930fc

In order to add support for the SELECT and TABLE tags in the HTML Processor, it needs to implement the HTML algorithm named "reset the insertion mode appropriately". This patch implements that algorithm to unblock the additional tag support. The algorithm resets the parsing mode after specific state changes in complicated situations where alternative rules are in effect (such as rules governing how the parser handles tags found within a TABLE element). Developed in WordPress/wordpress-develop#6020 Discussed in https://core.trac.wordpress.org/ticket/61549 Props dmsnell, jonsurrell. Fixes #61549. Built from https://develop.svn.wordpress.org/trunk@58656 git-svn-id: http://core.svn.wordpress.org/trunk@58070 1a063a9b-81f0-0310-95a4-ce76da25c4cd

In order to add support for the SELECT and TABLE tags in the HTML Processor, it needs to implement the HTML algorithm named "reset the insertion mode appropriately". This patch implements that algorithm to unblock the additional tag support. The algorithm resets the parsing mode after specific state changes in complicated situations where alternative rules are in effect (such as rules governing how the parser handles tags found within a TABLE element). Developed in WordPress/wordpress-develop#6020 Discussed in https://core.trac.wordpress.org/ticket/61549 Props dmsnell, jonsurrell. Fixes #61549. Built from https://develop.svn.wordpress.org/trunk@58656 git-svn-id: https://core.svn.wordpress.org/trunk@58070 1a063a9b-81f0-0310-95a4-ce76da25c4cd

sirreal force-pushed the html-api/add-reset-insertion-mode-algo branch from 596a977 to b893d3c Compare June 13, 2024 14:42

sirreal force-pushed the html-api/add-reset-insertion-mode-algo branch from b893d3c to 39d04fb Compare June 28, 2024 14:22

sirreal added 5 commits July 2, 2024 18:41

Implement insertion mode algorithm

8f7cb54

Document complete algorithm, fix last handling

91c6bf7

Add tests

2f246a8

Move reset insertion mode to state class

180f54c

This allows it to be better tested via public properties.

Add unsupported tests

17935b9

sirreal force-pushed the html-api/add-reset-insertion-mode-algo branch from 39d04fb to 17935b9 Compare July 2, 2024 16:41

sirreal commented Jul 2, 2024

View reviewed changes

tests/phpunit/tests/html-api/wpHtmlProcessorState.php Outdated Show resolved Hide resolved

tests/phpunit/tests/html-api/wpHtmlProcessorState.php Outdated Show resolved Hide resolved

sirreal added 3 commits July 2, 2024 19:10

Update ticket annotations

af2d9ba

Add tests and fix TD/TH last element

599e39a

Add more modes and tests

8d34a3a

sirreal marked this pull request as ready for review July 2, 2024 17:48

dmsnell reviewed Jul 2, 2024

View reviewed changes

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

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

dmsnell reviewed Jul 2, 2024

View reviewed changes

dmsnell added 2 commits July 2, 2024 13:45

Add test docs, rearrange setup code.

479d6f4

Adjust for context node in reset insertion mode algorithm.

5bcff17

dmsnell reviewed Jul 2, 2024

View reviewed changes

dmsnell mentioned this pull request Jul 2, 2024

HTML API: Add missing insertion mode constants. #6957

Closed

Merge branch 'trunk' into html-api/add-reset-insertion-mode-algo

0675543

dmsnell mentioned this pull request Jul 3, 2024

HTML API: Plans for 6.7 WordPress/gutenberg#60396

Open

19 tasks

Move reset algorithm into HTML Processor

631cfb1

sirreal commented Jul 3, 2024

View reviewed changes

Remove tests and directly reference context node.

b2c1ef4

Remove personality

8b125b4

dmsnell closed this Jul 3, 2024

dmsnell deleted the html-api/add-reset-insertion-mode-algo branch July 3, 2024 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML API: Implement reset_insertion_mode #6020

HTML API: Implement reset_insertion_mode #6020

sirreal commented Feb 2, 2024 •

edited by dmsnell

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024

dmsnell Jul 2, 2024

dmsnell left a comment •

edited

Loading

dmsnell commented Jul 2, 2024

sirreal commented Jul 3, 2024

sirreal commented Jul 3, 2024

sirreal Jul 3, 2024

dmsnell commented Jul 3, 2024

dmsnell commented Jul 3, 2024

	$node = new WP_HTML_Token( 'context-node', $this->context_node[0], false );
	$node = $this->context_node;

HTML API: Implement reset_insertion_mode #6020

HTML API: Implement reset_insertion_mode #6020

Conversation

sirreal commented Feb 2, 2024 • edited by dmsnell Loading

github-actions bot commented Feb 2, 2024 • edited Loading

github-actions bot commented Feb 2, 2024

Test using WordPress Playground

Some things to be aware of

dmsnell Jul 2, 2024

Choose a reason for hiding this comment

dmsnell left a comment • edited Loading

Choose a reason for hiding this comment

dmsnell commented Jul 2, 2024

sirreal commented Jul 3, 2024

sirreal commented Jul 3, 2024

sirreal Jul 3, 2024

Choose a reason for hiding this comment

dmsnell commented Jul 3, 2024

dmsnell commented Jul 3, 2024

sirreal commented Feb 2, 2024 •

edited by dmsnell

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

dmsnell left a comment •

edited

Loading