1359780 - Update DOMTokenList for spec changes

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://github.com/whatwg/dom/pull/444

There's only one change relevant to us: .toggle() and .replace() always remove duplicates and whitespace from the DOM attribute, even if they're no-ops.  This matches .add() and .remove(), and sometimes .replace().  Per spec, .replace() used to rewrite the attribute if you replaced a token with itself, but not if the token to replace isn't in the set.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: 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

Review commit: https://reviewboard.mozilla.org/r/133932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133932/

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8861918 [details]
Bug 1359780 - Always remove duplicates/whitespace in DOMTokenList methods

https://reviewboard.mozilla.org/r/133932/#review136816

::: dom/base/nsDOMTokenList.cpp:305
(Diff revision 1)
>    (*tokens.AppendElement()).Rebind(aToken.Data(), aToken.Length());
>  
> -  if (isPresent) {
> +  if (isPresent && !forceOn) {
> -    if (!forceOn) {
> -      RemoveInternal(attr, tokens);
> +    RemoveInternal(attr, tokens);
> -      isPresent = false;
> +    isPresent = false;

I think that use |return false;| is simpler.

::: dom/base/nsDOMTokenList.cpp:306
(Diff revision 1)
>  
> -  if (isPresent) {
> +  if (isPresent && !forceOn) {
> -    if (!forceOn) {
> -      RemoveInternal(attr, tokens);
> +    RemoveInternal(attr, tokens);
> -      isPresent = false;
> +    isPresent = false;
> -    }
> +  } else if (!isPresent && !forceOff) {

Then, this can be |if (!isPresent && !forceOff) {| (ommitting else).

::: dom/base/nsDOMTokenList.cpp:308
(Diff revision 1)
> -      isPresent = false;
> +    isPresent = false;
> -    }
> +  } else if (!isPresent && !forceOff) {
> -  } else {
> -    if (!forceOff) {
> -      AddInternal(attr, tokens);
> +    AddInternal(attr, tokens);
> -      isPresent = true;
> +    isPresent = true;

So, this should |return true;|.

::: dom/base/nsDOMTokenList.cpp:309
(Diff revision 1)
> -    }
> +  } else if (!isPresent && !forceOff) {
> -  } else {
> -    if (!forceOff) {
> -      AddInternal(attr, tokens);
> +    AddInternal(attr, tokens);
> -      isPresent = true;
> +    isPresent = true;
> +  } else if (attr) {

Then,

if (!attr) {
  return isPresent;
}

And reduce the following block's indent.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8861918 [details]
Bug 1359780 - Always remove duplicates/whitespace in DOMTokenList methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133932/diff/1-2/

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8861918 [details]
Bug 1359780 - Always remove duplicates/whitespace in DOMTokenList methods

https://reviewboard.mozilla.org/r/133932/#review136816

> Then,
> 
> if (!attr) {
>   return isPresent;
> }
> 
> And reduce the following block's indent.

We need to return isPresent whether attr is true or false, so the if (attr) { block needs to stay as it is.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8861918 [details]
Bug 1359780 - Always remove duplicates/whitespace in DOMTokenList methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133932/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47b53e3d4b27
Always remove duplicates/whitespace in DOMTokenList methods r=masayuki

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47b53e3d4b27

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6518bd1e2c8cc39fc0bbe6b174e7758059c0bd38

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This spec change was reverted: https://github.com/whatwg/dom/issues/451

Comment 10

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.