Bug 1359780 - Always remove duplicates/whitespace in DOMTokenList methods r=masayuki
authorAryeh Gregor <ayg@aryeh.name>
Wed, 26 Apr 2017 15:13:44 +0300
changeset 355018 47b53e3d4b27c0b900c06180d857dac1de1d3241
parent 355017 e5073b47f5c8f7aa8fc2e5578d4fcc6a6c79deed
child 355019 93fac052231cf92e6baefba5b1675d65e3bba175
push id41526
push userayg@aryeh.name
push dateWed, 26 Apr 2017 13:46:43 +0000
treeherderautoland@47b53e3d4b27 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmasayuki
bugs1359780
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1359780 - Always remove duplicates/whitespace in DOMTokenList methods r=masayuki 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");