<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[56684] trunk: HTML API: Remove all duplicate copies of an attribute when removing.</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { white-space: pre-line; overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta" style="font-size: 105%">
<dt style="float: left; width: 6em; font-weight: bold">Revision</dt> <dd><a style="font-weight: bold" href="https://core.trac.wordpress.org/changeset/56684">56684</a><script type="application/ld+json">{"@context":"http://schema.org","@type":"EmailMessage","description":"Review this Commit","action":{"@type":"ViewAction","url":"https://core.trac.wordpress.org/changeset/56684","name":"Review Commit"}}</script></dd>
<dt style="float: left; width: 6em; font-weight: bold">Author</dt> <dd>Bernhard Reiter</dd>
<dt style="float: left; width: 6em; font-weight: bold">Date</dt> <dd>2023-09-25 19:02:42 +0000 (Mon, 25 Sep 2023)</dd>
</dl>
<pre style='padding-left: 1em; margin: 2em 0; border-left: 2px solid #ccc; line-height: 1.25; font-size: 105%; font-family: sans-serif'>HTML API: Remove all duplicate copies of an attribute when removing.
When encountering an HTML tag with duplicate copies of an attribute the tag processor ignores the duplicate values, according to the specification. However, when removing an attribute it must remove all copies of that attribute lest one of the duplicates becomes the primary and it appears as if no attributes were removed.
In this patch we're adding tests that will be used to ensure that all attribute copies are removed from a tag when one is request to be removed.
**Before**
{{{#!php
<?php
$p = new WP_HTML_Tag_Processor( '<br id=one id="two" id='three' id>' );
$p->next_tag();
$p->remove_attribute( 'id' );
$p->get_updated_html();
// <br id="two" id='three' id>
}}}
**After**
{{{#!php
<?php
$p = new WP_HTML_Tag_Processor( '<br id=one id="two" id='three' id>' );
$p->next_tag();
$p->remove_attribute( 'id' );
$p->get_updated_html();
// <br>
}}}
Previously we have been overlooking duplicate attributes since they don't have an impact on what parses into the DOM. However, as one unit test affirmed (asserting the presence of the bug in the tag processor) when removing an attribute where duplicates exist this meant we ended up changing the value of an attribute instead of removing it.
In this patch we're tracking the text spans of the parsed duplicate attributes so that ''if'' we attempt to remove them then we'll have the appropriate information necessary to do so. When an attribute isn't removed we'll simply forget about the tracked duplicates. This involves some overhead for normal operation ''when'' in fact there are duplicate attributes on a tag, but that overhead is minimal in the form of integer pairs of indices for each duplicated attribute.
Props dmsnell, zieladam.
Fixes <a href="https://core.trac.wordpress.org/ticket/58119">#58119</a>.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunksrcwpincludeshtmlapiclasswphtmltagprocessorphp">trunk/src/wp-includes/html-api/class-wp-html-tag-processor.php</a></li>
<li><a href="#trunktestsphpunittestshtmlapiwpHtmlTagProcessorphp">trunk/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunksrcwpincludeshtmlapiclasswphtmltagprocessorphp"></a>
<div class="modfile"><h4 style="background-color: #eee; color: inherit; margin: 1em 0; padding: 1.3em; font-size: 115%">Modified: trunk/src/wp-includes/html-api/class-wp-html-tag-processor.php</h4>
<pre class="diff"><span>
<span class="info" style="display: block; padding: 0 10px; color: #888">--- trunk/src/wp-includes/html-api/class-wp-html-tag-processor.php 2023-09-25 17:47:27 UTC (rev 56683)
+++ trunk/src/wp-includes/html-api/class-wp-html-tag-processor.php 2023-09-25 19:02:42 UTC (rev 56684)
</span><span class="lines" style="display: block; padding: 0 10px; color: #888">@@ -407,6 +407,16 @@
</span><span class="cx" style="display: block; padding: 0 10px"> private $attributes = array();
</span><span class="cx" style="display: block; padding: 0 10px">
</span><span class="cx" style="display: block; padding: 0 10px"> /**
</span><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+ * Tracks spans of duplicate attributes on a given tag, used for removing
+ * all copies of an attribute when calling `remove_attribute()`.
+ *
+ * @since 6.3.2
+ *
+ * @var (WP_HTML_Span[])[]|null
+ */
+ private $duplicate_attributes = null;
+
+ /**
</ins><span class="cx" style="display: block; padding: 0 10px"> * Which class names to add or remove from a tag.
</span><span class="cx" style="display: block; padding: 0 10px"> *
</span><span class="cx" style="display: block; padding: 0 10px"> * These are tracked separately from attribute updates because they are
</span><span class="lines" style="display: block; padding: 0 10px; color: #888">@@ -1286,8 +1296,27 @@
</span><span class="cx" style="display: block; padding: 0 10px"> $attribute_end,
</span><span class="cx" style="display: block; padding: 0 10px"> ! $has_value
</span><span class="cx" style="display: block; padding: 0 10px"> );
</span><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+
+ return true;
</ins><span class="cx" style="display: block; padding: 0 10px"> }
</span><span class="cx" style="display: block; padding: 0 10px">
</span><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+ /*
+ * Track the duplicate attributes so if we remove it, all disappear together.
+ *
+ * While `$this->duplicated_attributes` could always be stored as an `array()`,
+ * which would simplify the logic here, storing a `null` and only allocating
+ * an array when encountering duplicates avoids needless allocations in the
+ * normative case of parsing tags with no duplicate attributes.
+ */
+ $duplicate_span = new WP_HTML_Span( $attribute_start, $attribute_end );
+ if ( null === $this->duplicate_attributes ) {
+ $this->duplicate_attributes = array( $comparable_name => array( $duplicate_span ) );
+ } elseif ( ! array_key_exists( $comparable_name, $this->duplicate_attributes ) ) {
+ $this->duplicate_attributes[ $comparable_name ] = array( $duplicate_span );
+ } else {
+ $this->duplicate_attributes[ $comparable_name ][] = $duplicate_span;
+ }
+
</ins><span class="cx" style="display: block; padding: 0 10px"> return true;
</span><span class="cx" style="display: block; padding: 0 10px"> }
</span><span class="cx" style="display: block; padding: 0 10px">
</span><span class="lines" style="display: block; padding: 0 10px; color: #888">@@ -1307,11 +1336,12 @@
</span><span class="cx" style="display: block; padding: 0 10px"> */
</span><span class="cx" style="display: block; padding: 0 10px"> private function after_tag() {
</span><span class="cx" style="display: block; padding: 0 10px"> $this->get_updated_html();
</span><del style="background-color: #fdd; text-decoration:none; display:block; padding: 0 10px">- $this->tag_name_starts_at = null;
- $this->tag_name_length = null;
- $this->tag_ends_at = null;
- $this->is_closing_tag = null;
- $this->attributes = array();
</del><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+ $this->tag_name_starts_at = null;
+ $this->tag_name_length = null;
+ $this->tag_ends_at = null;
+ $this->is_closing_tag = null;
+ $this->attributes = array();
+ $this->duplicate_attributes = null;
</ins><span class="cx" style="display: block; padding: 0 10px"> }
</span><span class="cx" style="display: block; padding: 0 10px">
</span><span class="cx" style="display: block; padding: 0 10px"> /**
</span><span class="lines" style="display: block; padding: 0 10px; color: #888">@@ -2080,6 +2110,17 @@
</span><span class="cx" style="display: block; padding: 0 10px"> ''
</span><span class="cx" style="display: block; padding: 0 10px"> );
</span><span class="cx" style="display: block; padding: 0 10px">
</span><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+ // Removes any duplicated attributes if they were also present.
+ if ( null !== $this->duplicate_attributes && array_key_exists( $name, $this->duplicate_attributes ) ) {
+ foreach ( $this->duplicate_attributes[ $name ] as $attribute_token ) {
+ $this->lexical_updates[] = new WP_HTML_Text_Replacement(
+ $attribute_token->start,
+ $attribute_token->end,
+ ''
+ );
+ }
+ }
+
</ins><span class="cx" style="display: block; padding: 0 10px"> return true;
</span><span class="cx" style="display: block; padding: 0 10px"> }
</span><span class="cx" style="display: block; padding: 0 10px">
</span></span></pre></div>
<a id="trunktestsphpunittestshtmlapiwpHtmlTagProcessorphp"></a>
<div class="modfile"><h4 style="background-color: #eee; color: inherit; margin: 1em 0; padding: 1.3em; font-size: 115%">Modified: trunk/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php</h4>
<pre class="diff"><span>
<span class="info" style="display: block; padding: 0 10px; color: #888">--- trunk/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php 2023-09-25 17:47:27 UTC (rev 56683)
+++ trunk/tests/phpunit/tests/html-api/wpHtmlTagProcessor.php 2023-09-25 19:02:42 UTC (rev 56684)
</span><span class="lines" style="display: block; padding: 0 10px; color: #888">@@ -1049,16 +1049,10 @@
</span><span class="cx" style="display: block; padding: 0 10px"> * Removing an attribute that's listed many times, e.g. `<div id="a" id="b" />` should remove
</span><span class="cx" style="display: block; padding: 0 10px"> * all its instances and output just `<div />`.
</span><span class="cx" style="display: block; padding: 0 10px"> *
</span><del style="background-color: #fdd; text-decoration:none; display:block; padding: 0 10px">- * Today, however, WP_HTML_Tag_Processor only removes the first such attribute. It seems like a corner case
- * and introducing additional complexity to correctly handle this scenario doesn't seem to be worth it.
- * Let's revisit if and when this becomes a problem.
</del><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+ * @since 6.3.2 Removes all duplicated attributes as expected.
</ins><span class="cx" style="display: block; padding: 0 10px"> *
</span><del style="background-color: #fdd; text-decoration:none; display:block; padding: 0 10px">- * This test is in place to confirm this behavior, which while incorrect, is well-defined.
- * A later fix introduced to the Tag Processor should update this test to reflect the
- * wanted and correct behavior.
</del><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+ * @ticket 58119
</ins><span class="cx" style="display: block; padding: 0 10px"> *
</span><del style="background-color: #fdd; text-decoration:none; display:block; padding: 0 10px">- * @ticket 56299
- *
</del><span class="cx" style="display: block; padding: 0 10px"> * @covers WP_HTML_Tag_Processor::remove_attribute
</span><span class="cx" style="display: block; padding: 0 10px"> */
</span><span class="cx" style="display: block; padding: 0 10px"> public function test_remove_first_when_duplicated_attribute() {
</span><span class="lines" style="display: block; padding: 0 10px; color: #888">@@ -1066,8 +1060,8 @@
</span><span class="cx" style="display: block; padding: 0 10px"> $p->next_tag();
</span><span class="cx" style="display: block; padding: 0 10px"> $p->remove_attribute( 'id' );
</span><span class="cx" style="display: block; padding: 0 10px">
</span><del style="background-color: #fdd; text-decoration:none; display:block; padding: 0 10px">- $this->assertSame(
- '<div id="ignored-id"><span id="second">Text</span></div>',
</del><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+ $this->assertStringNotContainsString(
+ 'update-me',
</ins><span class="cx" style="display: block; padding: 0 10px"> $p->get_updated_html(),
</span><span class="cx" style="display: block; padding: 0 10px"> 'First attribute (when duplicates exist) was not removed'
</span><span class="cx" style="display: block; padding: 0 10px"> );
</span><span class="lines" style="display: block; padding: 0 10px; color: #888">@@ -1091,6 +1085,61 @@
</span><span class="cx" style="display: block; padding: 0 10px"> }
</span><span class="cx" style="display: block; padding: 0 10px">
</span><span class="cx" style="display: block; padding: 0 10px"> /**
</span><ins style="background-color: #dfd; text-decoration:none; display:block; padding: 0 10px">+ * @ticket 58119
+ *
+ * @since 6.3.2 Removes all duplicated attributes as expected.
+ *
+ * @covers WP_HTML_Tag_Processor::remove_attribute
+ *
+ * @dataProvider data_html_with_duplicated_attributes
+ */
+ public function test_remove_attribute_with_duplicated_attributes_removes_all_of_them( $html_with_duplicate_attributes, $attribute_to_remove ) {
+ $p = new WP_HTML_Tag_Processor( $html_with_duplicate_attributes );
+ $p->next_tag();
+
+ $p->remove_attribute( $attribute_to_remove );
+ $this->assertNull( $p->get_attribute( $attribute_to_remove ), 'Failed to remove all copies of an attribute when duplicated in modified source.' );
+
+ // Recreate a tag processor with the updated HTML after removing the attribute.
+ $p = new WP_HTML_Tag_Processor( $p->get_updated_html() );
+ $p->next_tag();
+ $this->assertNull( $p->get_attribute( $attribute_to_remove ), 'Failed to remove all copies of duplicated attributes when getting updated HTML.' );
+ }
+
+ /**
+ * @ticket 58119
+ *
+ * @since 6.3.2 Removes all duplicated attributes as expected.
+ *
+ * @covers WP_HTML_Tag_Processor::remove_attribute
+ */
+ public function test_previous_duplicated_attributes_are_not_removed_on_successive_tag_removal() {
+ $p = new WP_HTML_Tag_Processor( '<span id=one id=two id=three><span id=four>' );
+ $p->next_tag();
+ $p->next_tag();
+ $p->remove_attribute( 'id' );
+
+ $this->assertSame( '<span id=one id=two id=three><span >', $p->get_updated_html() );
+ }
+
+ /**
+ * Data provider.
+ *
+ * @ticket 58119
+ *
+ * @return array[].
+ */
+ public function data_html_with_duplicated_attributes() {
+ return array(
+ 'Double attributes' => array( '<div id=one id=two>', 'id' ),
+ 'Triple attributes' => array( '<div id=one id=two id=three>', 'id' ),
+ 'Duplicates around another' => array( '<img src="test.png" alt="kites flying in the wind" src="kites.jpg">', 'src' ),
+ 'Case-variants of attribute' => array( '<button disabled inert DISABLED dISaBled INERT DisABleD>', 'disabled' ),
+ 'Case-variants of attribute name' => array( '<button disabled inert DISABLED dISaBled INERT DisABleD>', 'DISABLED' ),
+ );
+ }
+
+ /**
</ins><span class="cx" style="display: block; padding: 0 10px"> * @ticket 56299
</span><span class="cx" style="display: block; padding: 0 10px"> *
</span><span class="cx" style="display: block; padding: 0 10px"> * @covers WP_HTML_Tag_Processor::remove_attribute
</span></span></pre>
</div>
</div>
</body>
</html>