HTML API: Close all elements at the end of a document. by dmsnell · Pull Request #7085 · WordPress/wordpress-develop

dmsnell · 2024-07-24T20:08:02Z

When the model of breadcrumb generation in the HTML Processor and node traversal was simplified, the change introduced a bug whereby unclosed nodes at the end of a document would remain unvisited and unclosed.

In this patch, a fix is applied to ensure that all open elements close while traversing a document. A couple of minor documentation typos are fixed in the patch as well.

Follow-up to [58713].

See #61576.

When the model of breadcrumb generation in the HTML Processor and node traversal was simplified, the change introduced a bug whereby unclosed nodes at the end of a document would remain unvisited and unclosed. In this patch, a fix is applied to ensure that all open elements close while traversing a document. A couple of minor documentation typos are fixed in the patch as well. Follow-up to [58713]. See #61576.

github-actions · 2024-07-24T20:08:19Z

Trac Ticket Missing

This pull request is missing a link to a Trac ticket. For a contribution to be considered, there must be a corresponding ticket in Trac.

To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. More information about contributing to WordPress on GitHub can be found in the Core Handbook.

github-actions · 2024-07-24T20:08:28Z

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 dmsnell, gziolo, jonsurrell.

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

github-actions · 2024-07-24T20:20:21Z

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.

gziolo · 2024-07-25T05:34:50Z

It would be helpful to add test coverage for this case. Something simple and similar to the existing tests here:

wordpress-develop/tests/phpunit/tests/html-api/wpHtmlProcessorBreadcrumbs.php

Line 291 in 4acefac

    
           public function test_reports_correct_breadcrumbs_for_html( $html, $breadcrumbs, $ignored_n ) {

dmsnell · 2024-07-26T00:23:26Z

It would be helpful to add test coverage for this case. Something simple and similar to the existing tests here:

Your wish is my command!

@gziolo I confirmed that the new test fails in trunk but passes here.

gziolo

Excellent, this looks great!

sirreal

Good fix 👍

sirreal · 2024-07-29T07:07:18Z

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

+			while ( $this->state->stack_of_open_elements->pop() ) {
+				continue;
+			}


Minor, but something I've wondered about. the continue doesn't do anything here. Is there a reason to include it?

Suggested change

while ( $this->state->stack_of_open_elements->pop() ) {

continue;

}

while ( $this->state->stack_of_open_elements->pop() ) {}

I think you would have to put a code comment inside to document the intent otherwise.

originally I had a comment inside these loops but WPCS thought it was bad so required the introduction of continue, which surprisingly, introduces actual executable code and slows down the runtime

How strict WPCS is for while ( $this->state->stack_of_open_elements->pop() ) {} in a single line?

hmm. I might not have tried that, but I'm personally not a fan because of unexplained empty body. we can prep a PR and see if it complains. do you consider that better?

It was only thought exercise as it might go against a linting rule if the new line after an opening curly bracket is enforced.

All good with the code that landed 👍

When the model of breadcrumb generation in the HTML Processor and node traversal was simplified, the change introduced a bug whereby unclosed nodes at the end of a document would remain unvisited and unclosed. In this patch, a fix is applied to ensure that all open elements close while traversing a document. A couple of minor documentation typos are fixed in the patch as well. Developed in #7085 Discussed in https://core.trac.wordpress.org/ticket/61576 Follow-up to [58713]. Props: dmsnell, gziolo, jonsurrell. See #61576. git-svn-id: https://develop.svn.wordpress.org/trunk@58828 602fd350-edb4-49c9-b593-d223f7449a82

dmsnell · 2024-07-29T17:38:57Z

Merged in [58828]
c73d8a4

When the model of breadcrumb generation in the HTML Processor and node traversal was simplified, the change introduced a bug whereby unclosed nodes at the end of a document would remain unvisited and unclosed. In this patch, a fix is applied to ensure that all open elements close while traversing a document. A couple of minor documentation typos are fixed in the patch as well. Developed in WordPress/wordpress-develop#7085 Discussed in https://core.trac.wordpress.org/ticket/61576 Follow-up to [58713]. Props: dmsnell, gziolo, jonsurrell. See #61576. Built from https://develop.svn.wordpress.org/trunk@58828 git-svn-id: http://core.svn.wordpress.org/trunk@58224 1a063a9b-81f0-0310-95a4-ce76da25c4cd

When the model of breadcrumb generation in the HTML Processor and node traversal was simplified, the change introduced a bug whereby unclosed nodes at the end of a document would remain unvisited and unclosed. In this patch, a fix is applied to ensure that all open elements close while traversing a document. A couple of minor documentation typos are fixed in the patch as well. Developed in WordPress/wordpress-develop#7085 Discussed in https://core.trac.wordpress.org/ticket/61576 Follow-up to [58713]. Props: dmsnell, gziolo, jonsurrell. See #61576. Built from https://develop.svn.wordpress.org/trunk@58828 git-svn-id: https://core.svn.wordpress.org/trunk@58224 1a063a9b-81f0-0310-95a4-ce76da25c4cd

When the model of breadcrumb generation in the HTML Processor and node traversal was simplified, the change introduced a bug whereby unclosed nodes at the end of a document would remain unvisited and unclosed. In this patch, a fix is applied to ensure that all open elements close while traversing a document. A couple of minor documentation typos are fixed in the patch as well. Developed in WordPress#7085 Discussed in https://core.trac.wordpress.org/ticket/61576 Follow-up to [58713]. Props: dmsnell, gziolo, jonsurrell. See #61576. git-svn-id: https://develop.svn.wordpress.org/trunk@58828 602fd350-edb4-49c9-b593-d223f7449a82

dmsnell mentioned this pull request Jul 24, 2024

HTML API: Plans for 6.7 WordPress/gutenberg#60396

Closed

19 tasks

dmsnell added 2 commits July 25, 2024 16:52

Merge branch 'trunk' into html-api/fix-close-all-elements

7397897

Add test to ensure elements close.

380e379

gziolo approved these changes Jul 26, 2024

View reviewed changes

sirreal approved these changes Jul 29, 2024

View reviewed changes

Merge branch 'trunk' into html-api/fix-close-all-elements

c6532f7

dmsnell closed this Jul 29, 2024

dmsnell deleted the html-api/fix-close-all-elements branch July 29, 2024 17:39

Conversation

dmsnell commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 24, 2024

Trac Ticket Missing

Uh oh!

github-actions bot commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 24, 2024

Test using WordPress Playground

Some things to be aware of

Uh oh!

gziolo commented Jul 25, 2024

Uh oh!

dmsnell commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

sirreal Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmsnell Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

dmsnell Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

dmsnell commented Jul 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmsnell commented Jul 24, 2024 •

edited

Loading

github-actions bot commented Jul 24, 2024 •

edited

Loading

dmsnell commented Jul 26, 2024 •

edited

Loading

gziolo Jul 29, 2024 •

edited

Loading