Closed
Bug 1444909
Opened 7 years ago
Closed 7 years ago
Return boolean from DOMTokenList's replace() method
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: code, Assigned: bzbarsky)
References
Details
(Keywords: dev-doc-complete)
Attachments
(3 files)
2.16 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
20.61 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
4.13 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
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
![]() |
Assignee | |
Comment 1•7 years ago
|
||
Thank you for filing this and for writing the tests!
![]() |
Assignee | |
Comment 2•7 years ago
|
||
This fixes upcoming tests that will check when we actually mutate the attribute.
MozReview-Commit-ID: 7r5ErK9wvir
Attachment #8958706 -
Flags: review?(kyle)
![]() |
Assignee | |
Comment 3•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 4•7 years ago
|
||
MozReview-Commit-ID: AL1rU7AQlFb
Attachment #8958708 -
Flags: review?(kyle)
Updated•7 years ago
|
Attachment #8958706 -
Flags: review?(kyle) → review+
Updated•7 years ago
|
Attachment #8958707 -
Flags: review?(kyle) → review+
Updated•7 years ago
|
Attachment #8958708 -
Flags: review?(kyle) → review+
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
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7702f7024085
https://hg.mozilla.org/mozilla-central/rev/c97167cf4845
https://hg.mozilla.org/mozilla-central/rev/bb405d71ba67
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
![]() |
Assignee | |
Comment 7•7 years ago
|
||
James, any idea why the test changes here did not get upstreamed to wpt?
Flags: needinfo?(james)
Comment 8•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 9•7 years ago
|
||
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
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 12•7 years ago
|
||
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)
Keywords: dev-doc-needed → dev-doc-complete
![]() |
Assignee | |
Comment 13•7 years ago
|
||
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)
Comment 14•7 years ago
|
||
(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.
![]() |
Assignee | |
Comment 15•7 years ago
|
||
Looks great, thank you!
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•