Interactivity API: Skip instead of bail out if HTML contains SVG or MATH #6091

cbravobernal · 2024-02-12T20:18:19Z

When there is a SVG icon in the navigation menu. The Interactivity API is not server-side processing the directives. This is causing a flash effect when the runtime process the JavaScript part.

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

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-12T20:18:33Z

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 cbravobernal, westonruter, dmsnell, swissspidy.

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

github-actions · 2024-02-12T20:37:10Z

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.

westonruter

LGTM!

src/wp-content/fonts/OpenSans-Regular.ttf

dmsnell

This has been something that @sirreal and I have been working with lately.

#6006

it's complicated, and I might recommend creating something marginally more robust in the meantime, because the rules for the self-closing flag change within SVG and MathML.

that is, non-void elements which contain the self-closing flag are treated as empty elements.

there are escapes from SVG and MathML that I think we can practically overlook for now because the logic for detecting them is complicated, but a new method $p->skip_foreign_content() might serve us better.

this method would be like next_balanced_tag() except it will only pay attention to the current tag.

private function skip_foreign_content() {
	$tag_name = $this->get_tag_name();
	$depth    = 1;

	while ( $depth > && $this->next_tag( $tag_name ) ) {
		if ( $this->has_self_closing_flag() ) {
			continue;
		}

		$depth += $this->is_tag_closer() ? -1 : 1;
	}

	return 0 === $depth;
}

cbravobernal · 2024-02-13T18:32:32Z

This has been something that @sirreal and I have been working with lately.

#6006

it's complicated, and I might recommend creating something marginally more robust in the meantime, because the rules for the self-closing flag change within SVG and MathML.

that is, non-void elements which contain the self-closing flag are treated as empty elements.

there are escapes from SVG and MathML that I think we can practically overlook for now because the logic for detecting them is complicated, but a new method $p->skip_foreign_content() might serve us better.

this method would be like next_balanced_tag() except it will only pay attention to the current tag.
private function skip_foreign_content() {
	$tag_name = $this->get_tag_name();
	$depth    = 1;

	while ( $depth > && $this->next_tag( $tag_name ) ) {
		if ( $this->has_self_closing_flag() ) {
			continue;
		}

		$depth += $this->is_tag_closer() ? -1 : 1;
	}

	return 0 === $depth;
}

I created the function as suggested, with small tweaks to make it work as expected. I also added some tests in a pair programming session with @michalczaplinski

I would feel much more confident with a second review, thanks @dmsnell!

westonruter · 2024-02-13T19:19:21Z

src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php

+			if ( $this->has_self_closing_flag() ) {
+				continue;
+			}
+			$depth += $this->is_tag_closer() ? -1 : 1;


@dmsnell What about a title tag that appears inside of an SVG? Since it's in XML parsing mode, would it still be processed as RCDATA here?

@westonruter this is a great question, and you're right. it's not RCDATA when found inside SVG. this is all the more reason to focus only on SVG and MathML tags and ignore the others.

in #6006 what I'm looking to do is have the HTML Processor tell the Tag Processor that it's inside foreign content, and while that flag is active, we'll disable the special handling of tokens like TITLE and the others.

it's complicated and we explored trying to put the foreign-content handling code inside the Tag Processor, but that ended up demanding that we re-implement much of the HTML Processor's functionality there, so it will probably be the case that you need the HTML Processor to adequately parse inside SVG and MathML.

in the meantime, if one overlooks the HTML elements which break out into the nearest "integration point," then I think that skipping from SVG tag to SVG tag will be reliable enough for now.

aha I misread your statement @westonruter concerning TITLE. in this case it shouldn't matter that we're checking is_tag_closer() because we're not visiting the TITLE tag - we're skipping straight to whichever tag (SVG or MATH) opened the foreign content.

cbravobernal · 2024-02-13T21:10:30Z

Sorry @westonruter for re-requesting your review again, but the code has change quite a bit 😅

westonruter · 2024-02-13T22:38:02Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

+		);
+		$html           = '
+			<header>
+				<svg height="100">


Let's add a title tag here just to make sure it works as expected.

Suggested change

<svg height="100">

<svg height="100">

<title>Red Circle</title>

westonruter · 2024-02-13T22:38:19Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

+			<header>
+				<svg height="100">
+					<circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />
+					<div data-wp-bind--id="myPlugin::state.id"></div>


Why not an SVG element?

Suggested change

<div data-wp-bind--id="myPlugin::state.id"></div>

<g data-wp-bind--id="myPlugin::state.id" />

this is a good example of a bad example anyway 🙃 as the div here would close the open SVG element 🤦‍♂️

the full set of HTML tags which break out to the nearest integration point are:

"b", "big", "blockquote", "body", "br", "center", "code", "dd", "div", "dl", "dt", "em", "embed", "h1", "h2", "h3", "h4", "h5", "h6", "head", "hr", "i", "img", "li", "listing", "menu", "meta", "nobr", "ol", "p", "pre", "ruby", "s", "small", "span", "strong", "strike", "sub", "sup", "table", "tt", "u", "ul", "var"

and some FONT start tags containing specific attributes. also some end tags.

https://html.spec.whatwg.org/#parsing-main-inforeign

the "integration point" thing is a bit more complicated than it appears at a glance. there can be a stack of open elements that zigzags between HTML and foreign content, and some SVG/MathML tags themselves create new integration points, so a DIV inside one of those regions won't close out the SVG - it will only close out everything up until that element.

The idea was to use an element that we know the server directive processing would process. As far as I know, it should skip SVG tags and its inner accepted elements by default.

@c4rl0sbr4v0 my point was mostly that in an HTML document, the DIV in this example closes out the SVG, and so the skip_foreign_content() function would fail here.

if you wanted to use an element that's neutral on this, it would be fine to use something like <aside> or <data> or <abbr>, as these and any other elements not listed above do not affect the foreign content stack.

If they are edge cases, maybe is better to just use what should be inside a SVG tag.

if you wanted to use an element that's neutral on this, it would be fine to use something like
or or , as these and any other elements not listed above do not affect the foreign content stack.

You are becoming a huge expert in HTML processing @dmsnell 😃

westonruter · 2024-02-13T22:40:34Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

+		$html           = '
+			<header>
+				<math data-wp-bind--id="myPlugin::state.math">
+				<div data-wp-bind--id="myPlugin::state.id"></div>


Probably better to use actual MathML, right?

Suggested change

<div data-wp-bind--id="myPlugin::state.id"></div>

<mrow data-wp-bind--id="myPlugin::state.id" />

The idea was to use an element that we know the server directive processing would process. As far as I know, it should skip SVG tags and its inner accepted elements by default.

Similar here but with MathML

westonruter · 2024-02-13T22:42:19Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPIDirectivesProcessor.php

+		$content = '<div><span>Not closed</div>';
+		$p       = new WP_Interactivity_API_Directives_Processor( $content );
+		$p->next_tag();
+		$this->assertTrue( $p->skip_foreign_content() );


There's actually no foreign content here. Should a better name be chosen than skip_foreign_content() like jump_to_tag_closer()?

cbravobernal · 2024-02-14T10:44:45Z

I addressed the requested changes in 23f25c1

westonruter · 2024-02-14T16:04:55Z

src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php

+	 *
+	 * @return bool Whether the foreign content was successfully skipped.
+	 */
+	public function jump_to_tag_closer(): bool {


Just realized that skip is the terminology used in the tag processor and also in the description so it would probably be better here.

Suggested change

public function jump_to_tag_closer(): bool {

public function skip_to_tag_closer(): bool {

That's true.

westonruter · 2024-02-14T16:05:50Z

tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php

+		$html           = '
+			<header>
+				<math data-wp-bind--id="myPlugin::state.math">
+				<mrow data-wp-bind--id="myPlugin::state.id" />


Indentation nit.

Suggested change

<mrow data-wp-bind--id="myPlugin::state.id" />

<mrow data-wp-bind--id="myPlugin::state.id" />

westonruter

Just a couple nits left, so I'm pre-approving.

cbravobernal · 2024-02-14T16:40:01Z

Just a couple nits left, so I'm pre-approving.

Done in 8ea4a69

Ready to commit then!

swissspidy · 2024-02-17T13:47:41Z

@dmsnell Is this one good to go in your opinion as well?

cbravobernal · 2024-02-17T14:25:13Z

@dmsnell Is this one good to go in your opinion as well?

I'm afraid Dennis is AFK until the 26th of February. Not sure if he will answer on time 😅

swissspidy · 2024-02-17T15:27:54Z

Alright. Committed in https://core.trac.wordpress.org/changeset/57649. We can always commit follow-ups if needed.

cbravobernal · 2024-02-18T14:24:17Z

@sirreal has a lot of experience with HTML API, he could take a look at it too

sirreal · 2024-02-19T13:27:05Z

👍 The changes as committed here look good to me.

dmsnell · 2024-02-20T09:08:40Z

All good from my end too. Of course we could make this more robust, but doing so inches us towards the HTML Processor and isn't worth recreating. I think we're going to start exploring using the HTML Processor for the Interactivity API, so in time this will be a non-issue.

cbravobernal added 2 commits February 12, 2024 21:11

Skip instead of bail out if HTML contains SVG or MATH

cd9aa70

Make sure that directives after SVG or MATH are proccessed

4f4e576

cbravobernal mentioned this pull request Feb 12, 2024

Epic: Interactivity API - public 1.0 version WordPress/gutenberg#56803

Closed

47 tasks

westonruter requested a review from dmsnell February 12, 2024 21:59

westonruter approved these changes Feb 12, 2024

View reviewed changes

westonruter requested changes Feb 12, 2024

View reviewed changes

src/wp-content/fonts/OpenSans-Regular.ttf Outdated Show resolved Hide resolved

Remove unwanted font

5cff5e6

cbravobernal requested a review from westonruter February 12, 2024 22:39

westonruter approved these changes Feb 12, 2024

View reviewed changes

dmsnell reviewed Feb 13, 2024

View reviewed changes

Use different function instead, add tests

e3b5349

westonruter reviewed Feb 13, 2024

View reviewed changes

cbravobernal requested a review from westonruter February 13, 2024 20:55

westonruter requested changes Feb 13, 2024

View reviewed changes

Address test changes

23f25c1

westonruter reviewed Feb 14, 2024

View reviewed changes

westonruter approved these changes Feb 14, 2024

View reviewed changes

Adress nitpicks

8ea4a69

swissspidy closed this Feb 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactivity API: Skip instead of bail out if HTML contains SVG or MATH #6091

Interactivity API: Skip instead of bail out if HTML contains SVG or MATH #6091

cbravobernal commented Feb 12, 2024

github-actions bot commented Feb 12, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024

westonruter left a comment

dmsnell left a comment

cbravobernal commented Feb 13, 2024

westonruter Feb 13, 2024 •

edited

Loading

dmsnell Feb 13, 2024

dmsnell Feb 13, 2024

cbravobernal commented Feb 13, 2024

westonruter Feb 13, 2024

westonruter Feb 13, 2024

dmsnell Feb 13, 2024

cbravobernal Feb 14, 2024

dmsnell Feb 14, 2024

cbravobernal Feb 14, 2024

westonruter Feb 13, 2024

cbravobernal Feb 14, 2024

westonruter Feb 13, 2024

cbravobernal commented Feb 14, 2024

westonruter Feb 14, 2024

cbravobernal Feb 14, 2024

westonruter Feb 14, 2024

westonruter left a comment •

edited

Loading

cbravobernal commented Feb 14, 2024

swissspidy commented Feb 17, 2024

cbravobernal commented Feb 17, 2024

swissspidy commented Feb 17, 2024

cbravobernal commented Feb 18, 2024

sirreal commented Feb 19, 2024

dmsnell commented Feb 20, 2024

	<svg height="100">
	<svg height="100">
	<title>Red Circle</title>

	<div data-wp-bind--id="myPlugin::state.id"></div>
	<g data-wp-bind--id="myPlugin::state.id" />

	<div data-wp-bind--id="myPlugin::state.id"></div>
	<mrow data-wp-bind--id="myPlugin::state.id" />

	public function jump_to_tag_closer(): bool {
	public function skip_to_tag_closer(): bool {

Interactivity API: Skip instead of bail out if HTML contains SVG or MATH #6091

Interactivity API: Skip instead of bail out if HTML contains SVG or MATH #6091

Conversation

cbravobernal commented Feb 12, 2024

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

github-actions bot commented Feb 12, 2024

Test using WordPress Playground

Some things to be aware of

westonruter left a comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

cbravobernal commented Feb 13, 2024

westonruter Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbravobernal commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbravobernal commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter left a comment • edited Loading

Choose a reason for hiding this comment

cbravobernal commented Feb 14, 2024

swissspidy commented Feb 17, 2024

cbravobernal commented Feb 17, 2024

swissspidy commented Feb 17, 2024

cbravobernal commented Feb 18, 2024

sirreal commented Feb 19, 2024

dmsnell commented Feb 20, 2024

github-actions bot commented Feb 12, 2024 •

edited

Loading

westonruter Feb 13, 2024 •

edited

Loading

westonruter left a comment •

edited

Loading