Return boolean from DOMTokenList's replace() method

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: code, Assigned: bzbarsky)

Tracking

({dev-doc-complete})

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

a year ago
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
Thank you for filing this and for writing the tests!
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 1443998
This fixes upcoming tests that will check when we actually mutate the attribute.

MozReview-Commit-ID: 7r5ErK9wvir
Attachment #8958706 - Flags: review?(kyle)
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
Attachment #8958707 - Flags: review?(kyle)
Attachment #8958706 - Flags: review?(kyle) → review+
Attachment #8958707 - Flags: review?(kyle) → review+
Attachment #8958708 - Flags: review?(kyle) → review+

Comment 5

a year ago
Pushed by bzbarsky@mozilla.com:
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
Blocks: 1443998
James, any idea why the test changes here did not get upstreamed to wpt?
Flags: needinfo?(james)
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).
Flags: needinfo?(james)
Where "these changes" is "some but not all of the changes from this bug", right?
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10183 for changes under testing/web-platform/tests
Upstream PR merged
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!
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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.
Looks great, thank you!
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.