diff --git a/src/wp-includes/html-api/class-wp-html-tag-processor.php b/src/wp-includes/html-api/class-wp-html-tag-processor.php index 88487fb068d11..25dd7ef2f60bb 100644 --- a/src/wp-includes/html-api/class-wp-html-tag-processor.php +++ b/src/wp-includes/html-api/class-wp-html-tag-processor.php @@ -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 `` 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; @@ -4786,24 +4799,86 @@ public function remove_attribute( $name ): bool { * * Result:
*/ - $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: + * + * + * ^----^ + * start end + * replacement: `` + * + * Result: + * + * Had only `attr` been removed the result would have been ``, 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: + * + * + * ^--------^ + * start end + * replacement: ` ` + * + * Result: + * + * Had the `/` been removed outright the result would have been ``, 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. ``. 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. ``. + */ + $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. * diff --git a/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php b/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php index 7f59239608d6b..6c0ea4e6dba26 100644 --- a/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php +++ b/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php @@ -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. `` becoming ``. Conversely, removing too much can merge the + * tag name or a neighboring attribute into whatever follows, e.g. `` + * becoming ``. + * + * @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 + */ + public static function data_remove_attribute_preserves_tag_semantics(): array { + return array( + 'Slash before attribute' => array( 'ok', 'attr', 'ok' ), + 'Slash before attribute, no whitespace' => array( 'ok', 'attr', 'ok' ), + 'Multiple slashes before attribute' => array( 'ok', 'attr', 'ok' ), + 'Slash separating attributes, remove last' => array( 'ok', 'b', 'ok' ), + 'Slash separating attributes, remove first' => array( 'ok', 'a', 'ok' ), + 'Slash after quoted value, remove last' => array( 'ok', 'b', 'ok' ), + 'Slash before duplicated attributes' => array( 'ok', 'a', 'ok' ), + 'Slash and whitespace before attribute' => array( 'ok', 'attr', 'ok' ), + 'Slash inside unquoted value' => array( 'ok', 'attr', 'ok' ), + 'Slash ending unquoted value' => array( 'ok', 'attr', 'ok' ), + 'Slash ending unquoted value, remove other' => array( 'ok', 'b', 'ok' ), + 'Self-closing flag is preserved' => array( 'ok', 'attr', 'ok' ), + 'Self-closing flag after quoted value' => array( 'ok', 'attr', 'ok' ), + 'Self-closing flag, slash before attribute' => array( 'ok', 'attr', 'ok' ), + 'Attribute directly after quoted value' => array( 'ok', 'a', 'ok' ), + 'Attribute directly after quoted value, middle' => array( 'ok', 'a', 'ok' ), + 'Equals directly after quoted value' => array( 'ok', 'a', 'ok' ), + 'Quote directly after quoted value' => array( 'ok', 'a', 'ok' ), + 'Slash directly after quoted value' => array( 'ok', 'a', 'ok' ), + 'Whitespace after quoted value' => array( 'ok', 'a', '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. ``. + * + * @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 $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}> + */ + public static function data_set_and_remove_attribute_at_tag_name_end(): array { + return array( + 'Set then remove' => array( + 'ok', + array( array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ) ), + 'ok', + ), + 'Remove then set' => array( + 'ok', + array( array( 'remove_attribute', 'a' ), array( 'set_attribute', 'b', 'x' ) ), + 'ok', + ), + 'Set two then remove' => array( + 'ok', + array( array( 'set_attribute', 'c', 'y' ), array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ) ), + 'ok', + ), + 'Add class then remove' => array( + 'ok', + array( array( 'add_class', 'c' ), array( 'remove_attribute', 'a' ) ), + 'ok', + ), + 'Set then remove, self-closing' => array( + 'ok', + array( array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ) ), + 'ok', + ), + 'Set then remove, attribute follows' => array( + 'ok', + array( array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ) ), + 'ok', + ), + 'Set, remove both' => array( + 'ok', + array( array( 'set_attribute', 'b', 'x' ), array( 'remove_attribute', 'a' ), array( 'remove_attribute', 'c' ) ), + 'ok', + ), + ); + } + /** * @ticket 56299 *