Closed
Bug 1444909
Opened 5 years ago
Closed 5 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•5 years ago
|
||
Thank you for filing this and for writing the tests!
![]() |
Assignee | |
Comment 2•5 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•5 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•5 years ago
|
||
MozReview-Commit-ID: AL1rU7AQlFb
Attachment #8958708 -
Flags: review?(kyle)
Updated•5 years ago
|
Attachment #8958706 -
Flags: review?(kyle) → review+
Updated•5 years ago
|
Attachment #8958707 -
Flags: review?(kyle) → review+
Updated•5 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•5 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: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
![]() |
Assignee | |
Comment 7•5 years ago
|
||
James, any idea why the test changes here did not get upstreamed to wpt?
Flags: needinfo?(james)
Comment 8•5 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•5 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•5 years ago
|
Keywords: dev-doc-needed
Comment 12•5 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•5 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•5 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•5 years ago
|
||
Looks great, thank you!
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•