Skip to content

HTML API: Prevent remove_attribute() from introducing a self-closing flag #13208

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

Draft
sirreal wants to merge 5 commits into
base: trunk
Choose a base branch
from
+242 −11
Draft

HTML API: Prevent remove_attribute() from introducing a self-closing flag #13208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c67e7b
HTML API: Add tests for remove_attribute() preserving the self-closin…
sirreal Aug 20, 2026
9b8a4bc
HTML API: Prevent remove_attribute() from introducing a self-closing …
sirreal Aug 20, 2026
9cf76f9
HTML API: Extend remove_attribute() semantics tests to neighboring sy…
sirreal Aug 21, 2026
6495adf
HTML API: Keep neighboring syntax intact when removing slashes with a…
sirreal Aug 21, 2026
0da5474
HTML API: Add return types and array shapes to new remove_attribute()…
sirreal Aug 21, 2026
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
97 changes: 86 additions & 11 deletions src/wp-includes/html-api/class-wp-html-tag-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2769,6 +2769,19 @@ private static function sort_start_ascending( WP_HTML_Text_Replacement $a, WP_HT
return $by_start;
}

/*
* An insertion (a replacement of zero length) must be applied before
* any replacement which starts at the same offset but consumes bytes.
* Otherwise the consuming replacement would advance the cursor past
* the insertion point before the insertion is applied.
*
* For example, new attributes are inserted directly after the tag name,
* and removing the attribute in `<g/a>` also starts there.
*/
if ( ( 0 === $a->length ) !== ( 0 === $b->length ) ) {
return 0 === $a->length ? -1 : 1;
}

$by_text = isset( $a->text, $b->text ) ? strcmp( $a->text, $b->text ) : 0;
if ( 0 !== $by_text ) {
return $by_text;
Expand Down Expand Up @@ -4786,24 +4799,86 @@ public function remove_attribute( $name ): bool {
*
* Result: <div />
*/
$this->lexical_updates[ $name ] = new WP_HTML_Text_Replacement(
$this->attributes[ $name ]->start,
$this->attributes[ $name ]->length,
''
);
$this->lexical_updates[ $name ] = $this->get_attribute_removal( $this->attributes[ $name ] );

// Removes any duplicated attributes if they were also present.
foreach ( $this->duplicate_attributes[ $name ] ?? array() as $attribute_token ) {
$this->lexical_updates[] = new WP_HTML_Text_Replacement(
$attribute_token->start,
$attribute_token->length,
''
);
foreach ( $this->duplicate_attributes[ $name ] ?? array() as $attribute_span ) {
$this->lexical_updates[] = $this->get_attribute_removal( $attribute_span );
}

return true;
}

/**
* Creates a text replacement which removes the given attribute from the document.
*
* Any `/` characters immediately preceding the attribute are removed along with it.
* Inside a tag, `/` is ignored unless it's immediately followed by `>`, in which case
* it's the self-closing flag. Leaving a `/` behind could therefore change the meaning
* of the tag if it ends up adjacent to the `>` after the attribute is removed.
*
* Example:
*
* <g /attr>
* ^----^
* start end
* replacement: ``
*
* Result: <g >
*
* Had only `attr` been removed the result would have been `<g />`, which has a
* self-closing flag that wasn't in the input.
*
* When those `/` characters are the only thing separating the attribute from what
* precedes it, and another attribute immediately follows, a space is left in their
* place so that the surrounding syntax doesn't merge.
*
* Example:
*
* <g/attr="x"b>
* ^--------^
* start end
* replacement: ` `
*
* Result: <g b>
*
* Had the `/` been removed outright the result would have been `<gb>`, changing
* the tag name.
*
* @since 7.2.0
*
* @param WP_HTML_Attribute_Token|WP_HTML_Span $attribute Attribute to remove, or the span it occupies.
* @return WP_HTML_Text_Replacement Replacement which removes the attribute.
*/
private function get_attribute_removal( $attribute ): WP_HTML_Text_Replacement {
$start = $attribute->start;
$end = $start + $attribute->length;

/*
* A `/` can only precede an attribute as a separator: tag names and attribute
* names end at `/`, and it's part of an unquoted attribute value when found
* there. It's safe to consume every `/` leading up to the attribute.
*/
while ( $start > 0 && '/' === $this->html[ $start - 1 ] ) {
--$start;
}

/*
* Only a quoted attribute value can be directly followed by another attribute,
* e.g. `<g/a="x"b>`. If the `/` separating the removed attribute from the tag
* name or from a preceding attribute is consumed, then a space must take its
* place to keep the tag name or preceding attribute from merging with the
* following one, e.g. `<gb>`.
*/
$needs_separator = (
$start !== $attribute->start &&
isset( $this->html[ $end ] ) &&
0 === strspn( $this->html, " \t\f\r\n/>", $end, 1 )
);

return new WP_HTML_Text_Replacement( $start, $end - $start, $needs_separator ? ' ' : '' );
}

/**
* Adds a new class name to the currently matched tag.
*
Expand Down
156 changes: 156 additions & 0 deletions tests/phpunit/tests/html-api/wpHtmlTagProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1595,6 +1595,162 @@ public static function data_html_with_duplicated_attributes() {
);
}

/**
* Ensures that removing an attribute does not change anything else about the tag.
*
* Inside a tag, `/` is ignored unless immediately followed by `>`, where it's the
* self-closing flag; otherwise it merely separates attributes. Removing only the
* attribute's own span can leave a stray `/` which changes the meaning of the tag,
* e.g. `<g /attr>` becoming `<g />`. Conversely, removing too much can merge the
* tag name or a neighboring attribute into whatever follows, e.g. `<g/a="b"c>`
* becoming `<gc>`.
*
* @covers WP_HTML_Tag_Processor::remove_attribute
*
* @dataProvider data_remove_attribute_preserves_tag_semantics
*
* @param string $html HTML containing a G tag whose attribute will be removed.
* @param string $attribute_to_remove Name of the attribute to remove.
* @param string $expected Expected HTML after removing the attribute.
*/
public function test_remove_attribute_preserves_tag_semantics( string $html, string $attribute_to_remove, string $expected ): void {
$processor = new WP_HTML_Tag_Processor( $html );
$processor->next_tag( 'G' );
$had_self_closing_flag = $processor->has_self_closing_flag();
$expected_attributes = array();
foreach ( $processor->get_attribute_names_with_prefix( '' ) as $name ) {
if ( strtolower( $attribute_to_remove ) !== $name ) {
$expected_attributes[ $name ] = $processor->get_attribute( $name );
}
}

$this->assertTrue( $processor->remove_attribute( $attribute_to_remove ), 'Failed to remove the attribute.' );
$updated_html = $processor->get_updated_html();
$this->assertSame( $expected, $updated_html, 'Updated HTML did not match the expected output.' );

// Re-parse the output to confirm the tag's semantics are unchanged.
$processor = new WP_HTML_Tag_Processor( $updated_html );
$processor->next_tag();
$processor->next_tag();
$this->assertSame( 'G', $processor->get_tag(), 'Removing the attribute changed the tag name.' );
$this->assertSame(
$had_self_closing_flag,
$processor->has_self_closing_flag(),
$had_self_closing_flag
? 'Removing the attribute dropped the self-closing flag.'
: 'Removing the attribute introduced a self-closing flag.'
);
$actual_attributes = array();
foreach ( $processor->get_attribute_names_with_prefix( '' ) as $name ) {
$actual_attributes[ $name ] = $processor->get_attribute( $name );
}
$this->assertSame( $expected_attributes, $actual_attributes, 'Removing the attribute changed the other attributes.' );
$processor->next_token();
$this->assertSame( 'ok', $processor->get_modifiable_text(), 'Removing the attribute changed the content following the tag.' );
}

/**
* Data provider.
*
* @return array<string, array{string, string, string}>
*/
public static function data_remove_attribute_preserves_tag_semantics(): array {
return array(
'Slash before attribute' => array( '<svg><g /attr>ok', 'attr', '<svg><g >ok' ),
'Slash before attribute, no whitespace' => array( '<svg><g/attr>ok', 'attr', '<svg><g>ok' ),
'Multiple slashes before attribute' => array( '<svg><g //attr>ok', 'attr', '<svg><g >ok' ),
'Slash separating attributes, remove last' => array( '<svg><g a/b>ok', 'b', '<svg><g a>ok' ),
'Slash separating attributes, remove first' => array( '<svg><g a/b>ok', 'a', '<svg><g /b>ok' ),
'Slash after quoted value, remove last' => array( '<svg><g a="x"/b>ok', 'b', '<svg><g a="x">ok' ),
'Slash before duplicated attributes' => array( '<svg><g /a /a>ok', 'a', '<svg><g >ok' ),
'Slash and whitespace before attribute' => array( '<svg><g / attr>ok', 'attr', '<svg><g / >ok' ),
'Slash inside unquoted value' => array( '<svg><g attr=/>ok', 'attr', '<svg><g >ok' ),
'Slash ending unquoted value' => array( '<svg><g attr=a/>ok', 'attr', '<svg><g >ok' ),
'Slash ending unquoted value, remove other' => array( '<svg><g a=x/ b>ok', 'b', '<svg><g a=x/ >ok' ),
'Self-closing flag is preserved' => array( '<svg><g attr/>ok', 'attr', '<svg><g />ok' ),
'Self-closing flag after quoted value' => array( '<svg><g attr="x"/>ok', 'attr', '<svg><g />ok' ),
'Self-closing flag, slash before attribute' => array( '<svg><g /attr/>ok', 'attr', '<svg><g />ok' ),
'Attribute directly after quoted value' => array( '<svg><g/a="x"b>ok', 'a', '<svg><g b>ok' ),
'Attribute directly after quoted value, middle' => array( '<svg><g c/a="x"b>ok', 'a', '<svg><g c b>ok' ),
'Equals directly after quoted value' => array( '<svg><g/a="x"=b>ok', 'a', '<svg><g =b>ok' ),
'Quote directly after quoted value' => array( '<svg><g/a="x""b">ok', 'a', '<svg><g "b">ok' ),
'Slash directly after quoted value' => array( '<svg><g/a="x"/b>ok', 'a', '<svg><g/b>ok' ),
'Whitespace after quoted value' => array( '<svg><g/a="x" b>ok', 'a', '<svg><g b>ok' ),
);
}

/**
* Ensures that attributes added and removed on the same tag don't interfere
* when the removal extends all the way back to the end of the tag name.
*
* New attributes are inserted directly after the tag name, which is also where
* a removal starts when the removed attribute directly follows the tag name
* with only `/` between them, e.g. `<g/attr>`.
*
* @covers WP_HTML_Tag_Processor::remove_attribute
* @covers WP_HTML_Tag_Processor::set_attribute
*
* @dataProvider data_set_and_remove_attribute_at_tag_name_end
*
* @param string $html HTML containing a G tag.
* @param list<array{0: 'set_attribute'|'remove_attribute'|'add_class', 1: string, 2?: string}> $ops Operations to apply, each a method name followed by its arguments.
* @param string $expected Expected HTML after applying the operations.
*/
public function test_set_and_remove_attribute_at_tag_name_end( string $html, array $ops, string $expected ): void {
$processor = new WP_HTML_Tag_Processor( $html );
$processor->next_tag( 'G' );
foreach ( $ops as $op ) {
$method = array_shift( $op );
$processor->$method( ...$op );
}
$this->assertSame( $expected, $processor->get_updated_html() );
}

/**
* Data provider.
*
* @return array<string, array{string, list<array{0: 'set_attribute'|'remove_attribute'|'add_class', 1: string, 2?: string}>, string}>
*/
public static function data_set_and_remove_attribute_at_tag_name_end(): array {
return array(
'Set then remove' => array(
'<svg><g/a>ok',
array( array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ) ),
'<svg><g b="x">ok',
),
'Remove then set' => array(
'<svg><g/a>ok',
array( array( 'remove_attribute', 'a' ), array( 'set_attribute', 'b', 'x' ) ),
'<svg><g b="x">ok',
),
'Set two then remove' => array(
'<svg><g/a>ok',
array( array( 'set_attribute', 'c', 'y' ), array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ) ),
'<svg><g b="x" c="y">ok',
),
'Add class then remove' => array(
'<svg><g/a>ok',
array( array( 'add_class', 'c' ), array( 'remove_attribute', 'a' ) ),
'<svg><g class="c">ok',
),
'Set then remove, self-closing' => array(
'<svg><g/a/>ok',
array( array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ) ),
'<svg><g b="x"/>ok',
),
'Set then remove, attribute follows' => array(
'<svg><g/a="1"c>ok',
array( array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ) ),
'<svg><g b="x" c>ok',
),
'Set, remove both' => array(
'<svg><g/a/c>ok',
array( array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ), array( 'remove_attribute', 'c' ) ),
'<svg><g b="x">ok',
),
);
}

/**
* @ticket 56299
*
Expand Down
Loading