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.

Sign up for GitHub

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: Skip instead of bail out if HTML contains SVG or MATH #6091

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cd9aa70
Skip instead of bail out if HTML contains SVG or MATH
cbravobernal Feb 12, 2024
4f4e576
Make sure that directives after SVG or MATH are proccessed
cbravobernal Feb 12, 2024
5cff5e6
Remove unwanted font
cbravobernal Feb 12, 2024
e3b5349
Use different function instead, add tests
cbravobernal Feb 13, 2024
23f25c1
Address test changes
cbravobernal Feb 14, 2024
8ea4a69
Adress nitpicks
cbravobernal Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,39 @@ private function get_balanced_tag_bookmarks() {
return array( $opener_tag, $closer_tag );
}

/**
* Skips processing the content between tags.
*
* It positions the cursor in the closer tag of the foreign element, if it
* exists.
*
* This function is intended to skip processing SVG and MathML inner content
* instead of bailing out the whole processing.
*
* @since 6.5.0
*
* @access private
*
* @return bool Whether the foreign content was successfully skipped.
*/
public function jump_to_tag_closer(): bool {
Copy link
Member

Choose a reason for hiding this comment

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

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 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true.

$depth = 1;
$tag_name = $this->get_tag();
while ( $depth > 0 && $this->next_tag(
array(
'tag_name' => $tag_name,
'tag_closers' => 'visit',
)
) ) {
if ( $this->has_self_closing_flag() ) {
continue;
}
$depth += $this->is_tag_closer() ? -1 : 1;
Comment on lines +207 to +210
Copy link
Member

@westonruter westonruter Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

return 0 === $depth;
}

/**
* Finds the matching closing tag for an opening tag.
*
Expand Down
9 changes: 7 additions & 2 deletions src/wp-includes/interactivity-api/class-wp-interactivity-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,14 @@ private function process_directives_args( string $html, array &$context_stack, a
while ( $p->next_tag( array( 'tag_closers' => 'visit' ) ) ) {
$tag_name = $p->get_tag();

/*
* Directives inside SVG and MATH tags are not processed,
* as they are not compatible with the Tag Processor yet.
* We still process the rest of the HTML.
*/
if ( 'SVG' === $tag_name || 'MATH' === $tag_name ) {
$unbalanced = true;
break;
$p->jump_to_tag_closer();
continue;
}

if ( $p->is_tag_closer() ) {
Expand Down
119 changes: 100 additions & 19 deletions tests/phpunit/tests/interactivity-api/wpInteractivityAPI.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*
* @coversDefaultClass WP_Interactivity_API
*/
class Tests_WP_Interactivity_API extends WP_UnitTestCase {
class Tests_Interactivity_API_WpInteractivityAPI extends WP_UnitTestCase {
/**
* Instance of WP_Interactivity_API.
*
Expand Down Expand Up @@ -508,50 +508,131 @@ public function test_process_directives_doesnt_change_html_if_contains_unbalance
}

/**
* Tests that the `process_directives` returns the same HTML if it finds an
* SVG tag.
* Tests that the `process_directives` process the HTML outside a SVG tag.
*
* @ticket 60356
* @ticket 60517
*
* @covers ::process_directives
*/
public function test_process_directives_doesnt_change_html_if_contains_svgs() {
$this->interactivity->state( 'myPlugin', array( 'id' => 'some-id' ) );
public function test_process_directives_changes_html_if_contains_svgs() {
$this->interactivity->state(
'myPlugin',
array(
'id' => 'some-id',
'width' => '100',
)
);
$html = '
<div data-wp-bind--id="myPlugin::state.id">
<svg height="100" width="100">
<header>
<svg height="100" data-wp-bind--width="myPlugin::state.width">
<title>Red Circle</title>
<circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />
</svg>
</div>
</svg>
<div data-wp-bind--id="myPlugin::state.id"></div>
<div data-wp-bind--id="myPlugin::state.width"></div>
</header>
';
$processed_html = $this->interactivity->process_directives( $html );
$p = new WP_HTML_Tag_Processor( $processed_html );
$p->next_tag();
$p->next_tag( 'svg' );
$this->assertNull( $p->get_attribute( 'width' ) );
$p->next_tag( 'div' );
$this->assertEquals( 'some-id', $p->get_attribute( 'id' ) );
$p->next_tag( 'div' );
$this->assertEquals( '100', $p->get_attribute( 'id' ) );
}

/**
* Tests that the `process_directives` does not process the HTML
* inside SVG tags.
*
* @ticket 60517
*
* @covers ::process_directives
*/
public function test_process_directives_does_not_change_inner_html_in_svgs() {
$this->interactivity->state(
'myPlugin',
array(
'id' => 'some-id',
)
);
$html = '
<header>
<svg height="100">
Copy link
Member

Choose a reason for hiding this comment

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

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>

<circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />
<g data-wp-bind--id="myPlugin::state.id" />
</svg>
</header>
';
$processed_html = $this->interactivity->process_directives( $html );
$p = new WP_HTML_Tag_Processor( $processed_html );
$p->next_tag( 'div' );
$this->assertNull( $p->get_attribute( 'id' ) );
}

/**
* Tests that the `process_directives` returns the same HTML if it finds an
* Tests that the `process_directives` process the HTML outside the
* MathML tag.
*
* @ticket 60356
* @ticket 60517
*
* @covers ::process_directives
*/
public function test_process_directives_doesnt_change_html_if_contains_math() {
$this->interactivity->state( 'myPlugin', array( 'id' => 'some-id' ) );
public function test_process_directives_change_html_if_contains_math() {
$this->interactivity->state(
'myPlugin',
array(
'id' => 'some-id',
'math' => 'ml-id',
)
);
$html = '
<div data-wp-bind--id="myPlugin::state.id">
<math>
<header>
<math data-wp-bind--id="myPlugin::state.math">
<mi>x</mi>
<mo>=</mo>
<mi>1</mi>
</math>
</div>
<div data-wp-bind--id="myPlugin::state.id"></div>
</header>
';
$processed_html = $this->interactivity->process_directives( $html );
$p = new WP_HTML_Tag_Processor( $processed_html );
$p->next_tag();
$p->next_tag( 'math' );
$this->assertNull( $p->get_attribute( 'id' ) );
$p->next_tag( 'div' );
$this->assertEquals( 'some-id', $p->get_attribute( 'id' ) );
}

/**
* Tests that the `process_directives` does not process the HTML
* inside MathML tags.
*
* @ticket 60517
*
* @covers ::process_directives
*/
public function test_process_directives_does_not_change_inner_html_in_math() {
$this->interactivity->state(
'myPlugin',
array(
'id' => 'some-id',
)
);
$html = '
<header>
<math data-wp-bind--id="myPlugin::state.math">
<mrow data-wp-bind--id="myPlugin::state.id" />
Copy link
Member

Choose a reason for hiding this comment

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

Indentation nit.

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

<mi>x</mi>
<mo>=</mo>
<mi>1</mi>
</math>
</header>
';
$processed_html = $this->interactivity->process_directives( $html );
$p = new WP_HTML_Tag_Processor( $processed_html );
$p->next_tag( 'div' );
$this->assertNull( $p->get_attribute( 'id' ) );
}

Expand Down
49 changes: 48 additions & 1 deletion tests/phpunit/tests/interactivity-api/wpInteractivityAPIDirectivesProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*
* @coversDefaultClass WP_Interactivity_API_Directives_Processor
*/
class Tests_WP_Interactivity_API_Directives_Processor extends WP_UnitTestCase {
class Tests_Interactivity_API_WpInteractivityAPIDirectivesProcessor extends WP_UnitTestCase {
/**
* Tests the `get_content_between_balanced_template_tags` method on template
* tags.
Expand Down Expand Up @@ -778,4 +778,51 @@ public function test_next_balanced_tag_closer_tag_on_closing_tag() {
$p->next_tag( array( 'tag_closers' => 'visit' ) );
$this->assertFalse( $p->next_balanced_tag_closer_tag() );
}

/**
* Tests that jump_to_tag_closer skips to the next tag,
* independant of the content.
*
* @ticket 60517
*
* @covers ::jump_to_tag_closer
*/
public function test_jump_to_tag_closer() {
$content = '<div><span>Not closed</div>';
$p = new WP_Interactivity_API_Directives_Processor( $content );
$p->next_tag();
$this->assertTrue( $p->jump_to_tag_closer() );
$this->assertTrue( $p->is_tag_closer() );
$this->assertEquals( 'DIV', $p->get_tag() );
}

/**
* Tests that jump_to_tag_closer does not skip to the
* next tag if there is no closing tag.
*
* @ticket 60517
*
* @covers ::jump_to_tag_closer
*/
public function test_jump_to_tag_closer_bails_not_closed() {
$content = '<div>Not closed parent';
$p = new WP_Interactivity_API_Directives_Processor( $content );
$p->next_tag();
$this->assertFalse( $p->jump_to_tag_closer() );
}

/**
* Tests that jump_to_tag_closer does not skip to the next
* tag if the closing tag is different from the current tag.
*
* @ticket 60517
*
* @covers ::jump_to_tag_closer
*/
public function test_jump_to_tag_closer_bails_different_tags() {
$content = '<div></span>';
$p = new WP_Interactivity_API_Directives_Processor( $content );
$p->next_tag();
$this->assertFalse( $p->jump_to_tag_closer() );
}
}
Loading