HTML API: set_modifiable_text should fail on foregin element tags by sirreal · Pull Request #11083 · WordPress/wordpress-develop

sirreal · 2026-02-27T17:39:41Z

Ensure the the HTML API does not attempt to set modifiable text on a foreign element tag. This could happen in foreign content when the tag name matches a special "atomic" HTML element, like SCRIPT or TEXTAREA.

::set_modifiable_text() includes special handling to allow setting the text of certain special "atomic" elements. These elements all have special parsing rules in the HTML standard.

The special handling should only apply to elements in the HTML namespace. The special handling only checks the tag names.

For example, this is correct:
$p = WP_HTML_Processor::create_fragment('<textarea></textarea>');
$p->next_tag();
echo "{$p->get_namespace()}:{$p->get_tag()}\n";
assert( $p->set_modifiable_text('hello') );
echo $p->get_updated_html();
Prints:
html:TEXTAREA
<textarea>hello</textarea>
The following attempts to set the text on svg:textarea. This should fail and return false:
$p = WP_HTML_Processor::create_fragment('<svg><textarea></textarea></svg>');
$p->next_tag();
$p->next_tag();
echo "{$p->get_namespace()}:{$p->get_tag()}\n";
assert( $p->set_modifiable_text('whoops!') ); // this assertion should fail!
echo $p->get_updated_html();
Instead, it adds the text to the beginning of the HTML:
svg:TEXTAREA
whoops!<svg><textarea></textarea></svg>
::get_modifiable_text() should behave similarly. It does not appear to have any issues although it does not explicitly check the HTML namespace.

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

Use of AI Tools

AI assisted with the creation of tests, as well as expanding and improving the base test set.

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 · 2026-02-27T17:39:51Z

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

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

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

Copilot

Pull request overview

This PR fixes an HTML API edge case where set_modifiable_text() could incorrectly apply special “atomic element” handling to foreign (SVG/MathML) elements whose tag names match atomic HTML elements, by ensuring the special handling only runs in the HTML namespace.

Changes:

Restrict WP_HTML_Tag_Processor::set_modifiable_text() atomic-element handling to the HTML namespace.
Add PHPUnit coverage to ensure set_modifiable_text() rejects non-atomic HTML tags and atomic-like foreign tags (SVG/MathML) without mutating the input HTML.
Add parallel coverage in both WP_HTML_Processor and WP_HTML_Tag_Processor test suites.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`src/wp-includes/html-api/class-wp-html-tag-processor.php`	Adds an HTML-namespace guard before applying atomic-tag modifiable-text logic.
`tests/phpunit/tests/html-api/wpHtmlProcessorModifiableText.php`	Adds fragment-level tests ensuring `set_modifiable_text()` fails (and doesn’t mutate HTML) for non-atomic/foreign tags.
`tests/phpunit/tests/html-api/wpHtmlTagProcessorModifiableText.php`	Adds tag-processor tests ensuring `set_modifiable_text()` fails for non-atomic and foreign atomic-like tags.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/phpunit/tests/html-api/wpHtmlProcessorModifiableText.php

github-actions · 2026-02-27T17:51:25Z

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

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.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

tests/phpunit/tests/html-api/wpHtmlProcessorModifiableText.php

dmsnell · 2026-02-27T18:25:34Z

tests/phpunit/tests/html-api/wpHtmlTagProcessorModifiableText.php

+		string $parsing_namespace,
+		string $expected_tag
+	) {
+		$processor = new WP_HTML_Tag_Processor( $html );


this is ultimately a test of the Tag Processor and its behavior, no? as in, a failure here is a bug, but a failure in the HTML Processor is a bug in detecting namespace changes.

Does it make sense to run the test in both places? If so, wouldn’t it be quite easy for the tests to diverge given that they are in separate files?

If we want to ensure that the behavior is locked, why not run both classes from this same test runner? Since we have tests already asserting that the HTML Processor detects the namespace changes, we should only need to test the modifiable text behavior here, and therefore we should be fine reading the namespace from the HTML Processor rather than supplying it.

I don't know what you have in mind here, perhaps you could clarify. I'm open to changing or moving things, there is some duplication between these tests.

I will elaborate that the HTML Processor tests have added value. Subtle issues can appear in the interaction between the "smart" HTML processor and the "syntax-only" tag processor.

For example, this change appears equivalent, but it would make the HTML processor tests fail, detecting an issue with svg:TITLE elements:

diff --git c/src/wp-includes/html-api/class-wp-html-tag-processor.php i/src/wp-includes/html-api/class-wp-html-tag-processor.php index d544f0f49b..cba78fdb51 100644 --- c/src/wp-includes/html-api/class-wp-html-tag-processor.php +++ i/src/wp-includes/html-api/class-wp-html-tag-processor.php @@ -3813,7 +3813,7 @@ class WP_HTML_Tag_Processor { */ if ( self::STATE_MATCHED_TAG !== $this->parser_state || - 'html' !== $this->get_namespace() + 'html' !== $this->parsing_namespace ) { return false; }

That's because svg:title is an HTML integration point. When the svg:title element is put on the stack of open elements, the parsing namespace changes to HTML (this is correct, its children will be HTML elements), but the element's namespace is svg. It's important to use $this->get_namespace() to detect the current namespace and not the namespace to be used for ongoing parsing.

right, but wouldn’t or shouldn’t this test case be caught by those tests covering the integration point logic, and not slipped under the rug in a seemingly unrelated test?

on the contrary, if we introduced this bug, I would want that to cause a failure in detecting the svg:TITLE namespace, not in some set_modifiable_text() assertion which is testing html:TITLE

The bug in the diff I shared would be in the implementation of the ::set_modifiable_text() function. I didn't introduce that bug, but could have. I had to consider carefully which was correct and whether there was a difference and I'm intimately familiar with this code.

I'm not sure how else that particular issue could be detected. It's not a problem in any of the other systems. It would be a bug with ::set_modifiable_text(). It would be easy to introduce by assuming that a property check can be trivially swapped for a getter as a micro-optimization. It can't, that would be a mistake.

tests/phpunit/tests/html-api/wpHtmlTagProcessorModifiableText.php

tests/phpunit/tests/html-api/wpHtmlProcessorModifiableText.php

westonruter · 2026-02-27T19:04:13Z

I see my suggestions would also apply to r61754.

Co-authored-by: Weston Ruter <westonruter@gmail.com>

tests/phpunit/tests/html-api/wpHtmlProcessorModifiableText.php

Co-authored-by: Weston Ruter <westonruter@gmail.com>

Improve some test types from [61754]

sirreal · 2026-03-02T09:42:32Z

I see my suggestions would also apply to r61754.

I pushed dc65bb3 to update those test types in this PR.

dmsnell · 2026-03-02T19:52:15Z

tests/phpunit/tests/html-api/wpHtmlProcessorModifiableText.php

+		string $target_tag
+	): void {
+		$processor = WP_HTML_Processor::create_fragment( $html );
+		$this->assertInstanceOf( WP_HTML_Tag_Processor::class, $processor );


I’ve been trying to leave helpful notes to people for cases like this where the failure should be unrelated to the unit under test. This is not meant to assert behaviors about creating Tag Processor.

$this->assertInstanceOf( …, $processor, 'Failed to initialize HTML Processor: check test setup.' );

on that note, why are we asserting that the HTML Processor is a Tag Processor?

In a few places I've used assert() to differentiate between test invariants and the unit under test. I don't think that's a common pattern but I do think it has some merits.

I've updated the test to be a non-null assertion with an descriptive message.

dmsnell

while I do not love everything about the tests, the fix is great, and the tests at least cover the change. thanks @sirreal

The method should only apply to special "atomic" HTML tags like `SCRIPT` or `TEXTAREA`. `::set_modifiable_text()` should not apply to tags with the same name in other namespaces. Developed in #11083. Props jonsurrell, dmsnell, westonruter. Fixes #64751. git-svn-id: https://develop.svn.wordpress.org/trunk@61796 602fd350-edb4-49c9-b593-d223f7449a82

github-actions · 2026-03-03T14:59:15Z

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 61796
GitHub commit: 4129a3a

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

The method should only apply to special "atomic" HTML tags like `SCRIPT` or `TEXTAREA`. `::set_modifiable_text()` should not apply to tags with the same name in other namespaces. Developed in WordPress/wordpress-develop#11083. Props jonsurrell, dmsnell, westonruter. Fixes #64751. Built from https://develop.svn.wordpress.org/trunk@61796 git-svn-id: http://core.svn.wordpress.org/trunk@61102 1a063a9b-81f0-0310-95a4-ce76da25c4cd

sirreal added 4 commits February 27, 2026 17:32

add tests

091d72b

Improve and expand html mod text tests

7babedf

fix up tag proc mod text tests

23c15f5

Fix issue

34925d5

sirreal requested review from Copilot and dmsnell February 27, 2026 17:40

Copilot started reviewing on behalf of sirreal February 27, 2026 17:41 View session

sirreal commented Feb 27, 2026

View reviewed changes

src/wp-includes/html-api/class-wp-html-tag-processor.php Outdated Show resolved Hide resolved

Remove excessively specific tag name documentation

3e88f34

sirreal mentioned this pull request Feb 27, 2026

HTML API: Ensure set_modifiable_text includes TEXTAREA leading newlines. #11062

Closed

Copilot AI reviewed Feb 27, 2026

View reviewed changes

tests/phpunit/tests/html-api/wpHtmlProcessorModifiableText.php Outdated Show resolved Hide resolved

sirreal requested review from ockham and westonruter February 27, 2026 18:18

Fix typos in code comment

c5ef32a

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

sirreal requested a review from Copilot February 27, 2026 18:19

Copilot started reviewing on behalf of sirreal February 27, 2026 18:19 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

dmsnell reviewed Feb 27, 2026

View reviewed changes

westonruter reviewed Feb 27, 2026

View reviewed changes

Improve types in tests

cfb6a53

Co-authored-by: Weston Ruter <westonruter@gmail.com>

westonruter reviewed Feb 28, 2026

View reviewed changes

tests/phpunit/tests/html-api/wpHtmlProcessorModifiableText.php Outdated Show resolved Hide resolved

sirreal and others added 3 commits February 28, 2026 08:42

Clean up whitespace

893c28d

Co-authored-by: Weston Ruter <westonruter@gmail.com>

fix lint

92a127b

Improve test types

dc65bb3

Improve some test types from [61754]

sirreal added 2 commits March 2, 2026 10:50

search for target tag directly

2b443a8

Remove namespace checking from set_modifiable_text tests

bfae594

Merge branch 'trunk' into trac-64751/set-mod-text-foreign-elements

768310e

sirreal requested a review from dmsnell March 2, 2026 09:58

dmsnell reviewed Mar 2, 2026

View reviewed changes

Assert processor is not null, add debug note

dd8b09e

westonruter approved these changes Mar 2, 2026

View reviewed changes

dmsnell approved these changes Mar 2, 2026

View reviewed changes

sirreal added 2 commits March 3, 2026 09:56

Merge branch 'trunk' into trac-64751/set-mod-text-foreign-elements

f35efcf

Merge branch 'trunk' into trac-64751/set-mod-text-foreign-elements

ae8da66

github-actions bot closed this Mar 3, 2026

sirreal deleted the trac-64751/set-mod-text-foreign-elements branch March 3, 2026 14:59

Conversation

sirreal commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Use of AI Tools

Uh oh!

github-actions bot commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

github-actions bot commented Feb 27, 2026

Test using WordPress Playground

Some things to be aware of

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmsnell Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

sirreal Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

dmsnell Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

sirreal Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonruter commented Feb 27, 2026

Uh oh!

Uh oh!

sirreal commented Mar 2, 2026

Uh oh!

dmsnell Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

sirreal Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

dmsnell left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sirreal commented Feb 27, 2026 •

edited

Loading

github-actions bot commented Feb 27, 2026 •

edited

Loading