Bug 1359780 - Always remove duplicates/whitespace in DOMTokenList methods draft
authorAryeh Gregor <ayg@aryeh.name>
Wed, 26 Apr 2017 15:13:44 +0300
changeset 568680 71ed9611d4fd5b6e8802348d0baad76aef8828ec
parent 568655 6817694cc4a84a383b7eb8e7b00612ac5ece3f5b
child 626006 5b5616394b4d44fa1bf82efb1fd1e51796473ed3
push id55957
push userbmo:ayg@aryeh.name
push dateWed, 26 Apr 2017 13:45:24 +0000
bugs1359780
milestone55.0a1
Bug 1359780 - Always remove duplicates/whitespace in DOMTokenList methods Previously, replace() and toggle() would not always remove duplicates and whitespace from the DOM attribute in the case where they were no-ops (replacing a nonexistent token, force-toggling a token to its current state). Now they do. This matches the behavior of add() and remove(), and also replace() in one case (replacing an existing token with itself). This follows a spec change: https://github.com/whatwg/dom/pull/444 MozReview-Commit-ID: 7lDEQxOGxPV
dom/base/nsDOMTokenList.cpp
testing/web-platform/mozilla/tests/dom/classList.html
testing/web-platform/tests/dom/nodes/Element-classlist.html
testing/web-platform/tests/dom/nodes/MutationObserver-attributes.html
--- a/dom/base/nsDOMTokenList.cpp
+++ b/dom/base/nsDOMTokenList.cpp
@@ -295,26 +295,39 @@ nsDOMTokenList::Toggle(const nsAString& 
   const nsAttrValue* attr = GetParsedAttr();
   const bool forceOn = aForce.WasPassed() && aForce.Value();
   const bool forceOff = aForce.WasPassed() && !aForce.Value();
 
   bool isPresent = attr && attr->Contains(aToken);
   AutoTArray<nsString, 1> tokens;
   (*tokens.AppendElement()).Rebind(aToken.Data(), aToken.Length());
 
-  if (isPresent) {
-    if (!forceOn) {
-      RemoveInternal(attr, tokens);
-      isPresent = false;
+  if (isPresent && !forceOn) {
+    RemoveInternal(attr, tokens);
+    return false;
+  }
+
+  if (!isPresent && !forceOff) {
+    AddInternal(attr, tokens);
+    return true;
+  }
+
+  if (attr) {
+    // Remove duplicates and whitespace from attr
+    RemoveDuplicates(attr);
+
+    nsAutoString resultStr;
+    for (uint32_t i = 0; i < attr->GetAtomCount(); i++) {
+      if (!resultStr.IsEmpty()) {
+        resultStr.AppendLiteral(" ");
+      }
+      resultStr.Append(nsDependentAtomString(attr->AtomAt(i)));
     }
-  } else {
-    if (!forceOff) {
-      AddInternal(attr, tokens);
-      isPresent = true;
-    }
+
+    mElement->SetAttr(kNameSpaceID_None, mAttrAtom, resultStr, true);
   }
 
   return isPresent;
 }
 
 void
 nsDOMTokenList::Replace(const nsAString& aToken,
                         const nsAString& aNewToken,
@@ -370,19 +383,17 @@ nsDOMTokenList::ReplaceInternal(const ns
       continue;
     }
     if (!resultStr.IsEmpty()) {
       resultStr.AppendLiteral(" ");
     }
     resultStr.Append(nsDependentAtomString(aAttr->AtomAt(i)));
   }
 
-  if (sawIt) {
-    mElement->SetAttr(kNameSpaceID_None, mAttrAtom, resultStr, true);
-  }
+  mElement->SetAttr(kNameSpaceID_None, mAttrAtom, resultStr, true);
 }
 
 bool
 nsDOMTokenList::Supports(const nsAString& aToken,
                          ErrorResult& aError)
 {
   if (!mSupportedTokens) {
     aError.ThrowTypeError<MSG_TOKENLIST_NO_SUPPORTED_TOKENS>(
--- a/testing/web-platform/mozilla/tests/dom/classList.html
+++ b/testing/web-platform/mozilla/tests/dom/classList.html
@@ -249,22 +249,20 @@ function testClassList(e, desc) {
   checkContains("null undefined", [null, undefined], true);
   checkContains("\t\n\f\r a\t\n\f\r b\t\n\f\r ", ["a", "b"], true);
 
   // add() method
 
   function checkAdd(before, argument, after, expectedException) {
     checkModification(e, "add", argument, undefined, before, after,
                       expectedException, desc);
-    // Also check force toggle
-    // XXX https://github.com/whatwg/dom/issues/443
-    //if (!Array.isArray(argument)) {
-    //  checkModification(e, "toggle", [argument, true], true, before, after,
-    //                    expectedException);
-    //}
+    if (!Array.isArray(argument)) {
+      checkModification(e, "toggle", [argument, true], true, before, after,
+                        expectedException, desc);
+    }
   }
 
   checkAdd(null, "", null, "SyntaxError");
   checkAdd(null, ["a", ""], null, "SyntaxError");
   checkAdd(null, " ", null, "InvalidCharacterError");
   checkAdd(null, "\ta", null, "InvalidCharacterError");
   checkAdd(null, "a\t", null, "InvalidCharacterError");
   checkAdd(null, "\na", null, "InvalidCharacterError");
@@ -304,22 +302,20 @@ function testClassList(e, desc) {
   checkAdd(null, null, "null");
   checkAdd(null, undefined, "undefined");
 
   // remove() method
 
   function checkRemove(before, argument, after, expectedException) {
     checkModification(e, "remove", argument, undefined, before, after,
                       expectedException, desc);
-    // Also check force toggle
-    // XXX https://github.com/whatwg/dom/issues/443
-    //if (!Array.isArray(argument)) {
-    //  checkModification(e, "toggle", [argument, false], false, before, after,
-    //                    expectedException);
-    //}
+    if (!Array.isArray(argument)) {
+      checkModification(e, "toggle", [argument, false], false, before, after,
+                        expectedException, desc);
+    }
   }
 
   checkRemove(null, "", null, "SyntaxError");
   checkRemove(null, " ", null, "InvalidCharacterError");
   checkRemove("\ta", "\ta", "\ta", "InvalidCharacterError");
   checkRemove("a\t", "a\t", "a\t", "InvalidCharacterError");
   checkRemove("\na", "\na", "\na", "InvalidCharacterError");
   checkRemove("a\n", "a\n", "a\n", "InvalidCharacterError");
@@ -402,34 +398,16 @@ function testClassList(e, desc) {
   checkToggle("\t\n\f\r a\t\n\f\r b\t\n\f\r ", "c", true, "a b c");
 
   checkToggle("null", null, false, "");
   checkToggle("", null, true, "null");
   checkToggle("undefined", undefined, false, "");
   checkToggle("", undefined, true, "undefined");
 
 
-  // tests for the force argument handling
-  // XXX Remove these if https://github.com/whatwg/dom/issues/443 is fixed
-
-  function checkForceToggle(before, argument, force, expectedRes, after, expectedException) {
-    checkModification(e, "toggle", [argument, force], expectedRes, before,
-                      after, expectedException, desc);
-  }
-
-  checkForceToggle("", "a", true, true, "a");
-  checkForceToggle("a", "a", true, true, "a");
-  checkForceToggle("a", "b", true, true, "a b");
-  checkForceToggle("a b", "b", true, true, "a b");
-  checkForceToggle("", "a", false, false, "");
-  checkForceToggle("a", "a", false, false, "");
-  checkForceToggle("a", "b", false, false, "a");
-  checkForceToggle("a b", "b", false, false, "a");
-
-
   // replace() method
   function checkReplace(before, token, newToken, after, expectedException) {
     checkModification(e, "replace", [token, newToken], undefined, before,
                       after, expectedException, desc);
   }
 
   checkReplace(null, "", "a", null, "SyntaxError");
   checkReplace(null, "", " ", null, "SyntaxError");
@@ -459,29 +437,25 @@ function testClassList(e, desc) {
   checkReplace(null, "b", " a", null, "InvalidCharacterError");
   checkReplace(null, "b", "a ", null, "InvalidCharacterError");
 
   checkReplace("a", "a", "a", "a");
   checkReplace("a", "a", "b", "b");
   checkReplace("a", "A", "b", "a");
   checkReplace("a b", "b", "A", "a A");
   checkReplace("a b c", "d", "e", "a b c");
-  // https://github.com/whatwg/dom/issues/443
   checkReplace("a a a  b", "a", "a", "a b");
-  checkReplace("a a a  b", "c", "d", "a a a  b");
+  checkReplace("a a a  b", "c", "d", "a b");
   checkReplace(null, "a", "b", null);
   checkReplace("", "a", "b", "");
-  checkReplace(" ", "a", "b", " ");
+  checkReplace(" ", "a", "b", "");
   checkReplace(" a  \f", "a", "b", "b");
   checkReplace("a b c", "b", "d", "a d c");
-  // https://github.com/whatwg/dom/issues/442
-  // Implementations agree on the first one here, so I test it, but disagree on
-  // the second, so no test until the spec decides what to say.
   checkReplace("a b c", "c", "a", "a b");
-  //checkReplace("c b a", "c", "a", ???);
+  checkReplace("c b a", "c", "a", "a b");
   checkReplace("a b a", "a", "c", "c b");
   checkReplace("a b a", "b", "c", "a c");
   checkReplace("   a  a b", "a", "c", "c b");
   checkReplace("   a  a b", "b", "c", "a c");
   checkReplace("\t\n\f\r a\t\n\f\r b\t\n\f\r ", "a", "c", "c b");
   checkReplace("\t\n\f\r a\t\n\f\r b\t\n\f\r ", "b", "c", "a c");
 
   checkReplace("a null", null, "b", "a b");
--- a/testing/web-platform/tests/dom/nodes/Element-classlist.html
+++ b/testing/web-platform/tests/dom/nodes/Element-classlist.html
@@ -197,22 +197,20 @@ function testClassList(e, desc) {
   checkContains("null undefined", [null, undefined], true);
   checkContains("\t\n\f\r a\t\n\f\r b\t\n\f\r ", ["a", "b"], true);
 
   // add() method
 
   function checkAdd(before, argument, after, expectedException) {
     checkModification(e, "add", argument, undefined, before, after,
                       expectedException, desc);
-    // Also check force toggle
-    // XXX https://github.com/whatwg/dom/issues/443
-    //if (!Array.isArray(argument)) {
-    //  checkModification(e, "toggle", [argument, true], true, before, after,
-    //                    expectedException);
-    //}
+    if (!Array.isArray(argument)) {
+      checkModification(e, "toggle", [argument, true], true, before, after,
+                        expectedException, desc);
+    }
   }
 
   checkAdd(null, "", null, "SyntaxError");
   checkAdd(null, ["a", ""], null, "SyntaxError");
   checkAdd(null, " ", null, "InvalidCharacterError");
   checkAdd(null, "\ta", null, "InvalidCharacterError");
   checkAdd(null, "a\t", null, "InvalidCharacterError");
   checkAdd(null, "\na", null, "InvalidCharacterError");
@@ -252,22 +250,20 @@ function testClassList(e, desc) {
   checkAdd(null, null, "null");
   checkAdd(null, undefined, "undefined");
 
   // remove() method
 
   function checkRemove(before, argument, after, expectedException) {
     checkModification(e, "remove", argument, undefined, before, after,
                       expectedException, desc);
-    // Also check force toggle
-    // XXX https://github.com/whatwg/dom/issues/443
-    //if (!Array.isArray(argument)) {
-    //  checkModification(e, "toggle", [argument, false], false, before, after,
-    //                    expectedException);
-    //}
+    if (!Array.isArray(argument)) {
+      checkModification(e, "toggle", [argument, false], false, before, after,
+                        expectedException, desc);
+    }
   }
 
   checkRemove(null, "", null, "SyntaxError");
   checkRemove(null, " ", null, "InvalidCharacterError");
   checkRemove("\ta", "\ta", "\ta", "InvalidCharacterError");
   checkRemove("a\t", "a\t", "a\t", "InvalidCharacterError");
   checkRemove("\na", "\na", "\na", "InvalidCharacterError");
   checkRemove("a\n", "a\n", "a\n", "InvalidCharacterError");
@@ -350,34 +346,16 @@ function testClassList(e, desc) {
   checkToggle("\t\n\f\r a\t\n\f\r b\t\n\f\r ", "c", true, "a b c");
 
   checkToggle("null", null, false, "");
   checkToggle("", null, true, "null");
   checkToggle("undefined", undefined, false, "");
   checkToggle("", undefined, true, "undefined");
 
 
-  // tests for the force argument handling
-  // XXX Remove these if https://github.com/whatwg/dom/issues/443 is fixed
-
-  function checkForceToggle(before, argument, force, expectedRes, after, expectedException) {
-    checkModification(e, "toggle", [argument, force], expectedRes, before,
-                      after, expectedException, desc);
-  }
-
-  checkForceToggle("", "a", true, true, "a");
-  checkForceToggle("a", "a", true, true, "a");
-  checkForceToggle("a", "b", true, true, "a b");
-  checkForceToggle("a b", "b", true, true, "a b");
-  checkForceToggle("", "a", false, false, "");
-  checkForceToggle("a", "a", false, false, "");
-  checkForceToggle("a", "b", false, false, "a");
-  checkForceToggle("a b", "b", false, false, "a");
-
-
   // replace() method
   function checkReplace(before, token, newToken, after, expectedException) {
     checkModification(e, "replace", [token, newToken], undefined, before,
                       after, expectedException, desc);
   }
 
   checkReplace(null, "", "a", null, "SyntaxError");
   checkReplace(null, "", " ", null, "SyntaxError");
@@ -407,29 +385,25 @@ function testClassList(e, desc) {
   checkReplace(null, "b", " a", null, "InvalidCharacterError");
   checkReplace(null, "b", "a ", null, "InvalidCharacterError");
 
   checkReplace("a", "a", "a", "a");
   checkReplace("a", "a", "b", "b");
   checkReplace("a", "A", "b", "a");
   checkReplace("a b", "b", "A", "a A");
   checkReplace("a b c", "d", "e", "a b c");
-  // https://github.com/whatwg/dom/issues/443
   checkReplace("a a a  b", "a", "a", "a b");
-  checkReplace("a a a  b", "c", "d", "a a a  b");
+  checkReplace("a a a  b", "c", "d", "a b");
   checkReplace(null, "a", "b", null);
   checkReplace("", "a", "b", "");
-  checkReplace(" ", "a", "b", " ");
+  checkReplace(" ", "a", "b", "");
   checkReplace(" a  \f", "a", "b", "b");
   checkReplace("a b c", "b", "d", "a d c");
-  // https://github.com/whatwg/dom/issues/442
-  // Implementations agree on the first one here, so I test it, but disagree on
-  // the second, so no test until the spec decides what to say.
   checkReplace("a b c", "c", "a", "a b");
-  //checkReplace("c b a", "c", "a", ???);
+  checkReplace("c b a", "c", "a", "a b");
   checkReplace("a b a", "a", "c", "c b");
   checkReplace("a b a", "b", "c", "a c");
   checkReplace("   a  a b", "a", "c", "c b");
   checkReplace("   a  a b", "b", "c", "a c");
   checkReplace("\t\n\f\r a\t\n\f\r b\t\n\f\r ", "a", "c", "c b");
   checkReplace("\t\n\f\r a\t\n\f\r b\t\n\f\r ", "b", "c", "a c");
 
   checkReplace("a null", null, "b", "a b");
--- a/testing/web-platform/tests/dom/nodes/MutationObserver-attributes.html
+++ b/testing/web-platform/tests/dom/nodes/MutationObserver-attributes.html
@@ -216,26 +216,28 @@ runMutationTest(n42,
                   {"attributes":true, "attributeOldValue": true},
                   [{type: "attributes", oldValue: "c01 c02", attributeName: "class"}],
                   function() { n42.classList.toggle("c01", false);},
                   "attributes Element.classList.toggle: forced token removal mutation");
 
   var n43 = document.getElementById('n43');
 runMutationTest(n43,
                   {"attributes":true, "attributeOldValue": true},
-                  [{type: "attributes", oldValue: "n43", attributeName: "id"}],
+                  [{type: "attributes", oldValue: "c01 c02", attributeName: "class"},
+                   {type: "attributes", oldValue: "n43", attributeName: "id"}],
                   function() { n43.classList.toggle("c03", false); n43.id = "n430"; },
-                  "attributes Element.classList.toggle: forced missing token removal no mutation");
+                  "attributes Element.classList.toggle: forced missing token removal no-op");
 
   var n44 = document.getElementById('n44');
 runMutationTest(n44,
                   {"attributes":true, "attributeOldValue": true},
-                  [{type: "attributes", oldValue: "n44", attributeName: "id"}],
+                  [{type: "attributes", oldValue: "c01 c02", attributeName: "class"},
+                   {type: "attributes", oldValue: "n44", attributeName: "id"}],
                   function() { n44.classList.toggle("c01", true); n44.id = "n440"; },
-                  "attributes Element.classList.toggle: forced existing token addition no mutation");
+                  "attributes Element.classList.toggle: forced existing token addition no-op");
 
   var n45 = document.getElementById('n45');
 runMutationTest(n45,
                   {"attributes":true, "attributeOldValue": true},
                   [{type: "attributes", oldValue: "c01 c02", attributeName: "class"}],
                   function() { n45.classList.toggle("c03", true);},
                   "attributes Element.classList.toggle: forced token addition mutation");