Update DOMTokenList for spec changes

RESOLVED INVALID

Status

()

RESOLVED INVALID
2 years ago
10 months ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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 hidden (mozreview-request)
Attachment #8861918 - Flags: review?(michael)
Attachment #8861918 - Flags: review?(michael) → review?(masayuki)

Comment 2

2 years ago
mozreview-review
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.
Attachment #8861918 - Flags: review?(masayuki) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 6

2 years ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/47b53e3d4b27
Always remove duplicates/whitespace in DOMTokenList methods r=masayuki

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/47b53e3d4b27
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Keywords: leave-open
status-firefox55: fixed → ---
Target Milestone: mozilla55 → ---
This spec change was reverted: https://github.com/whatwg/dom/issues/451
Status: REOPENED → RESOLVED
Last Resolved: 2 years agoa year ago
Resolution: --- → INVALID
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.