Closed
Bug 1359453
Opened 7 years ago
Closed 7 years ago
Consider whether to match Chrome/wpt/spec for Selection.removeRange()
Categories
(Core :: DOM: Selection, enhancement)
Core
DOM: Selection
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ayg, Assigned: ayg)
Details
Attachments
(1 file)
Spec: "The method must make the context object empty by disassociating its range if the context object's range is range. Otherwise, it must do nothing." http://w3c.github.io/selection-api/#dom-selection-removerange This sounds like it only removes the range if the same exact object is already in the range. Indeed this is what Chrome does, and the wpt test checks for that: http://w3c-test.org/selection/removeRange.html The test was contributed by Chrome when they added removeRange in February: https://bugs.chromium.org/p/chromium/issues/detail?id=692881 WebKit doesn't have the removeRange method at all. Edge does, but it seems to be a no-op (based on the wpt test). This is not what we do, but what do we do? If this is in the spec at all, it should match us, because we were the only working implementation until this past February. But I can't easily understand from the code what we actually do. Should we ask for the spec to be changed, and if so, to what? Or should the spec just drop the method? It's clearly not necessary for web-compat, and is useless if you don't support multi-range selections.
Flags: needinfo?(bugs)
Comment 1•7 years ago
|
||
Matching the spec to what we do makes sense. We do pointer comparison too, and then update the other ranges as needed.
Flags: needinfo?(bugs)
Assignee | ||
Comment 2•7 years ago
|
||
Could you clarify what our algorithm is? It seems we throw NS_ERROR_INVALID_ARG (== NS_ERROR_ILLEGAL_VALUE) when the range is not in the selection. Is that the only change we should request to the spec (obviously, changing the exception type to NotFoundError or something)?
Flags: needinfo?(bugs)
Comment 3•7 years ago
|
||
Oh, I guess we could just not throw anything in that case. Is that what Chrome does these days? Then the spec doesn't sounds too bad (except that it doesn't support multiple ranges)
Flags: needinfo?(bugs)
Assignee | ||
Comment 4•7 years ago
|
||
If I change us to not throw, we pass all the tests. But shouldn't we change the spec to match us instead of changing to match the spec, since we were the original implementation of this feature and nobody else implemented it until Chrome did just now? We could change to not throw, I just don't see why we'd want to change our long-standing behavior.
Flags: needinfo?(bugs)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bugs)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8861929 -
Flags: review?(masayuki)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8861929 [details] Bug 1359453 - Use standard exception for selection.removeRange() https://reviewboard.mozilla.org/r/133944/#review136832 Nice!
Attachment #8861929 -
Flags: review?(masayuki) → review+
Pushed by ayg@aryeh.name: https://hg.mozilla.org/integration/autoland/rev/8422cd52edf7 Use standard exception for selection.removeRange() r=masayuki
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8422cd52edf7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•