Skip to content
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: Fix - avoid calling subclass method while internally scanning in Tag Processor #5475

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Oct 12, 2023

Trac ticket: Core-59607

After modifying tags in the HTML API, the Tag Processor backs up to before the tag being modified and then re-parses its attributes. This saves on the code-complexity involved in applying updates, which have already been transformed to "lexical updates" by the time they are applied.

In order to do that, get_updated_html() calls next_tag() to reuse its logic. Unfortunately, as a public method, subclasses may change the behavior of that method, and the HTML Processor does just this. It maintains an HTML stack of open elements and when the Tag Processor calls this method to re-scan a tag and its attributes, it leads to a broken stack.

To fix this, this patch replaces the call to next_tag() with a more appropriate reapplication of its internal parsing logic to rescan the tag name and its attributes. Given the limited nature of what's occurring in get_updated_html() this should bring with it certain guarantees that no HTML structure is being changed (that structure will only be changed by subclasses like the HTML Processor).

This patch resolves an issue discovered by @adamziel during testing of the HTML Processor.

There is no evidence that this makes a clear impact on performance

5475-comparison

@dmsnell dmsnell changed the title HTML API: Fix - unwind stack when modifying tag. HTML API: Fix - avoid calling subclass method while internally scanning in Tag Processor Oct 12, 2023
@dmsnell dmsnell marked this pull request as ready for review October 12, 2023 23:24
@dmsnell dmsnell force-pushed the html-api/fix-problems-found-during-testing branch from 838f4f4 to 7aebc80 Compare October 12, 2023 23:33
@dmsnell
Copy link
Member Author

dmsnell commented Oct 12, 2023

cc: @adamziel @westonruter @ockham

Copy link
Contributor

@adamziel adamziel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey this is better than the previous implementation ❤️

@dmsnell dmsnell force-pushed the html-api/fix-problems-found-during-testing branch from 7aebc80 to 3026215 Compare October 13, 2023 21:23
@SergeyBiryukov
Copy link
Member

Thanks for the PR! Merged in r56941.

@dmsnell
Copy link
Member Author

dmsnell commented Oct 16, 2023

thanks @SergeyBiryukov!

@dmsnell dmsnell deleted the html-api/fix-problems-found-during-testing branch October 16, 2023 17:50
@andrewserong
Copy link
Contributor

I'm not sure if it's related, but Gutenberg trunk PHP tests have started failing, in particular WP_Directive_Processor tests:

There were 2 failures:

1) WP_Directive_Processor_Test::test_set_inner_html_subsequent_updates_on_the_same_tag_work
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<div>outside</div><section>This is the even newer section content.</section>'
+'<This is the even newer section content.</section>'

/var/www/html/wp-content/plugins/gutenberg/phpunit/experimental/interactivity-api/class-wp-directive-processor-test.php:90
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:60

2) WP_Directive_Processor_Test::test_set_inner_html_preceded_by_set_attribute_works
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<div>outside</div><section id="thesection">This is the new section content.</section>'
+'<This is the new section content.</section>'

/var/www/html/wp-content/plugins/gutenberg/phpunit/experimental/interactivity-api/class-wp-directive-processor-test.php:108
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:60

Could that be related to this change, since WP_Directive_Processor extends WP_HTML_Tag_Processor?

There's also a bit of discussion over in the #core-editor Slack channel: https://wordpress.slack.com/archives/C02QB2JS7/p1697514694299429?thread_ts=1697471144.510409&cid=C02QB2JS7

dmsnell added a commit to dmsnell/wordpress-develop that referenced this pull request Oct 17, 2023
Fixes a bug introduced in WordPress#5475.

When applying updates to HTML, one step was left out in WordPress#5475 which
updated the position of the end of the current tag. This made it
possible to create bookmarks with null or earlier end positions than
their start position. This in turn broke the Directive Processor in
Gutenberg during the backport of changes from Core into Gutenberg.

In this patch, after applying updates, the HTML document is now scanned
fully to the end of the current tag, updating the internal pointer to
its end, so that nothing else will be broken or misaligned.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants