-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Interactivity API: Fix directives in tags whose closing tag is not visited not working #6247
Conversation
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
src/wp-includes/interactivity-api/class-wp-interactivity-api.php
Outdated
Show resolved
Hide resolved
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
cc for review: @luisherranz, maybe @westonruter too |
Merge commit 'f2dd8b64448cb670665e4b1b8bc417343f7db435' into interactivity-api/test-dmsnell
* | ||
* @covers ::wp_interactivity_process_directives_of_interactive_blocks | ||
*/ | ||
public function test_process_directives_in_tags_that_dont_visit_closer_tag() { |
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.
Could this test be expanded (or another one added) which test that the absence of the new if
condition does indeed cause breaking the whole server-side rendering processing as described?
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.
Not sure if I understand what you mean. The existing test already checks that the SSR doesn't break when there is an iframe. The same test was failing without the final solution.
By the way, I've reverted the if
condition because it is solved by another PR as explained here.
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 thought that there were wider breakages than just with a single element. So that's what I thought would be good to test.
After a lot of debugging I'm wondering if the IFRAME is a red herring and the problem is that we have directive processors that don't handle elements without closing tags. For example, I added a test which adds context on an IMG element and it also failed to process in the same way that the test added here fails. <div data-wp-context='{"text": "outer"}'>
<img data-wp-context='{"text": "inner"}'>
<p data-wp-text="context.text"></p>
</div> and then in the test I call Or these tests simply aren't running the server directive processor because when I output the input |
please ignore my last comment: I finally reproduced this and I think I can say that it's fixed already in #6120, which is supposed to go in to 6.5 anyway, and this gives it even more reason to do so. what's happening? once the attribute was updated and unfortunately this short-circuit was programmed with the old assumption that everything is a tag, and we missed updating it earlier in the cycle for all tokens, including the handling of special tokens. in #6120 the Tag Processor creates its own internal |
This branch, without the proposed fix, but with the deferred updates PR passes the tests. |
f2dd8b6
to
c5d4457
Compare
Thanks a lot for taking a deeper look at it, @dmsnell I've rebased |
…ents. Adds a test to ensure proper processing of directives on special HTML elements, or HTML which contains special elements. These special elements are defined by the HTML API and are the HTML elements which cannot contain other tags, such as the IFRAME, SCRIPT, TEXTAREA, TITLE, elements, etc... The server diretive processor performs a custom tracking of HTML structure and this test ensures it isn't mislead by the handling of those special elements. Developed in #6247 Discussed in https://core.trac.wordpress.org/ticket/60746 Props santosguillamot, cbravobernal, mukesh27, westonruter, swissspidy, dmsnell. Follow-up to [57348]. Fixes #60746. git-svn-id: https://develop.svn.wordpress.org/trunk@57822 602fd350-edb4-49c9-b593-d223f7449a82
…ents. Adds a test to ensure proper processing of directives on special HTML elements, or HTML which contains special elements. These special elements are defined by the HTML API and are the HTML elements which cannot contain other tags, such as the IFRAME, SCRIPT, TEXTAREA, TITLE, elements, etc... The server diretive processor performs a custom tracking of HTML structure and this test ensures it isn't mislead by the handling of those special elements. Developed in WordPress/wordpress-develop#6247 Discussed in https://core.trac.wordpress.org/ticket/60746 Props santosguillamot, cbravobernal, mukesh27, westonruter, swissspidy, dmsnell. Follow-up to [57348]. Fixes #60746. Built from https://develop.svn.wordpress.org/trunk@57822 git-svn-id: http://core.svn.wordpress.org/trunk@57323 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…ents. Adds a test to ensure proper processing of directives on special HTML elements, or HTML which contains special elements. These special elements are defined by the HTML API and are the HTML elements which cannot contain other tags, such as the IFRAME, SCRIPT, TEXTAREA, TITLE, elements, etc... The server diretive processor performs a custom tracking of HTML structure and this test ensures it isn't mislead by the handling of those special elements. Developed in WordPress/wordpress-develop#6247 Discussed in https://core.trac.wordpress.org/ticket/60746 Props santosguillamot, cbravobernal, mukesh27, westonruter, swissspidy, dmsnell. Follow-up to [57348]. Fixes #60746. Built from https://develop.svn.wordpress.org/trunk@57822 git-svn-id: https://core.svn.wordpress.org/trunk@57323 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Trac ticket: https://core.trac.wordpress.org/ticket/60746
Right now, if I add directives to tags with closing tag that is not visited by the tag processor, it breaks the whole server-side rendering processing.
This is the list of those tags:
wordpress-develop/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php
Lines 25 to 34 in 186eeaf
For example, if I try to add a directive to an iframe, it breaks the processing.
Co-authored by @cbravobernal
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.