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)

enhancement
Not set
normal

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)
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)
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)
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)
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)
Flags: needinfo?(bugs)
Attachment #8861929 - Flags: review?(masayuki)
Assignee: nobody → ayg
Status: NEW → ASSIGNED
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
https://hg.mozilla.org/mozilla-central/rev/8422cd52edf7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: