1444909 - Return boolean from DOMTokenList's replace() method

Status: RESOLVED FIXED

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

Description

[Magne Andersson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180311220116

Steps to reproduce:

A proposed change to the specification for DOMTokenList's replace() method which would make it return a boolean - indicating true if a token was replaced and false otherwise - has received positive responses from implementers.

Pull request and discussion: https://github.com/whatwg/dom/pull/582

Tests: https://github.com/w3c/web-platform-tests/pull/9920

Comment 1

[Boris Zbarsky [:bzbarsky]]

Thank you for filing this and for writing the tests!

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Fix DOMTokenList.replace() to not do the attr set when the old token was not found

This fixes upcoming tests that will check when we actually mutate the attribute.

MozReview-Commit-ID: 7r5ErK9wvir

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change DOMTokenList.replace() to return a boolean and pull in the corresponding web platform test updates

The wpt changes come from https://github.com/w3c/web-platform-tests/pull/9920
and are needed to keep Element-classlist.html passing.

The change to testing/web-platform/mozilla/tests/dom/classList.html is pulling
those changes into our weird forked version of that test...

MozReview-Commit-ID: CvQlBRuieUY

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add testing for what mutation observers happen when doing DOMTokenList.replace

MozReview-Commit-ID: AL1rU7AQlFb

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7702f7024085
part 1.  Fix DOMTokenList.replace() to not do the attr set when the old token was not found.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/c97167cf4845
part 2.  Change DOMTokenList.replace() to return a boolean and pull in the corresponding web platform test updates.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb405d71ba67
part 3.  Add testing for what mutation observers happen when doing DOMTokenList.replace.  r=qdot

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7702f7024085
https://hg.mozilla.org/mozilla-central/rev/c97167cf4845
https://hg.mozilla.org/mozilla-central/rev/bb405d71ba67

Comment 7

[Boris Zbarsky [:bzbarsky]]

James, any idea why the test changes here did not get upstreamed to wpt?

Comment 8

[James Graham [:jgraham]]

It seems like 5a1c088ef04548f623a2b6704466158eaf300c9f upstream already contains these changes, which was a situation that's badly handled (we error out rather than do something more reasonable like nothing).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Where "these changes" is "some but not all of the changes from this bug", right?

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10183 for changes under testing/web-platform/tests

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 12

[Chris Mills [:cmills]]

I have documented this:

Description of replace return value updated:
https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/replace

Note added to Fx61 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/61#DOM

PR for updating the browser compat data:
https://github.com/mdn/browser-compat-data/pull/2124

Let me know if that look OK, thanks!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Chris, the returned value is a boolean value, not a Boolean object.  The documentation all seems to claim a Boolean object is returned, which isn't right.

Other than that, looks good.

Comment 14

[Chris Mills [:cmills]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> Chris, the returned value is a boolean value, not a Boolean object.  The
> documentation all seems to claim a Boolean object is returned, which isn't
> right.
> 
> Other than that, looks good.

Ooops. I've fixed it now, thanks for reporting.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Looks great, thank you!