Closed Bug 1359371 Opened 7 years ago Closed 7 years ago

Update Selection.webidl to match spec

Categories

(Core :: DOM: Selection, enhancement)

Product:
Core
Component:
DOM: Selection
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ayg, Assigned: ayg)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Bug 1359371 - Update Selection.webidl to match spec
59 bytes, text/x-review-board-request
smaug
: review+
Details
Our IDL: https://hg.mozilla.org/mozilla-central/file/a30dc237c3a6/dom/webidl/Selection.webidl Spec: http://w3c.github.io/selection-api/#selection-interface Differences (other than added attributes/methods): * The first argument to collapse() in the spec is optional. If the node is null, it behaves like removeAllRanges(). This was done here: <https://github.com/w3c/selection-api/issues/64>. I think our current behavior is better, but the current spec matches Blink and WebKit, and Blink tried to change to our behavior but found it was incompatible. So I think we need to change. * The offset argument to collapse() and extend() is optional, and the boolean argument to containsNode(). This was done here: <https://github.com/w3c/selection-api/issues/30>. The spec matches WebKit and Blink. Blink requested the change because they don't want to risk compat fallout, and the defaults aren't so unreasonable. I think we should change. Those are all the differences I see. I'll file separate bugs for added methods.
Who would you like to review?
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
Attachment #8861382 - Flags: review?(masayuki)
Comment on attachment 8861382 [details] Bug 1359371 - Update Selection.webidl to match spec Changes of .webidl need DOM peer's review, but I'm not. Smaug, could you review this? Aryeh leaves soon. So, this request is urgent.
Flags: needinfo?(bugs)
Aryeh, want to put this to my queue tomorrow morning or so?
Flags: needinfo?(bugs)
Comment on attachment 8861382 [details] Bug 1359371 - Update Selection.webidl to match spec https://reviewboard.mozilla.org/r/133382/#review136296 ::: dom/webidl/Selection.webidl:21 (Diff revision 1) > - [Throws] > + [Throws] > - Range getRangeAt(unsigned long index); > + Range getRangeAt(unsigned long index); > - [Throws, BinaryName="addRangeJS"] > + [Throws, BinaryName="addRangeJS"] > - void addRange(Range range); > + void addRange(Range range); > - [Throws] > + [Throws] > - void removeRange(Range range); > + void removeRange(Range range); > - [Throws] > + [Throws] > - void removeAllRanges(); > - > - [Throws] > - boolean containsNode(Node node, boolean allowPartialContainment); > - > + void removeAllRanges(); > + //void empty(); > + [Throws, BinaryName="collapseJS"] > + void collapse(Node? node, optional unsigned long offset = 0); > + //void setPosition(Node? node, optional unsigned long offset = 0); > + [Throws, BinaryName="collapseToStartJS"] > + void collapseToStart(); > + [Throws, BinaryName="collapseToEndJS"] > + void collapseToEnd(); > + [Throws, BinaryName="extendJS"] > + void extend(Node node, optional unsigned long offset = 0); > - [Throws, BinaryName="setBaseAndExtentJS"] > + [Throws, BinaryName="setBaseAndExtentJS"] Oh, you shouldn't use tabs to indent. Plaese replace them with ASCII whitespaces.
Attachment #8861382 - Flags: review?(masayuki)
Attachment #8861382 - Flags: review?(bugs)
Comment on attachment 8861382 [details] Bug 1359371 - Update Selection.webidl to match spec https://reviewboard.mozilla.org/r/133382/#review136332 ::: commit-message-62a2d:3 (Diff revision 1) > +Bug 1359371 - Update Selection.webidl to match spec > + > +The only practical change is that some additional method arguments are not quite true. This adds for example one CEReactions
Attachment #8861382 - Flags: review?(bugs) → review+
Pushed by ayg@aryeh.name: https://hg.mozilla.org/integration/autoland/rev/447626910bd6 Update Selection.webidl to match spec r=smaug
https://hg.mozilla.org/mozilla-central/rev/447626910bd6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Selection is changed: * containsNode(Node node, boolean allowPartialContainment); -> containsNode(Node node, optional boolean allowPartialContainment = false); * collapse(Node node, unsigned long offset); -> collapse(Node? node, optional unsigned long offset = 0); * extend(Node node, unsigned long offset); -> extend(Node node, optional unsigned long offset = 0); * Added: deleteFromDocument();
Keywords: dev-doc-needed
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10) > Selection is changed: > > * containsNode(Node node, boolean allowPartialContainment); > -> containsNode(Node node, optional boolean allowPartialContainment = > false); > * collapse(Node node, unsigned long offset); > -> collapse(Node? node, optional unsigned long offset = 0); > * extend(Node node, unsigned long offset); > -> extend(Node node, optional unsigned long offset = 0); > * Added: deleteFromDocument(); Thanks for the really useful changes summary Masayuki! I have added these changes to the relevant pages, including making sure all the browser compat info is up-to-date: https://developer.mozilla.org/en-US/docs/Web/API/Selection/collapse https://developer.mozilla.org/en-US/docs/Web/API/Selection/extend https://developer.mozilla.org/en-US/docs/Web/API/Selection/containsNode https://developer.mozilla.org/en-US/docs/Web/API/Selection/deleteFromDocument https://developer.mozilla.org/en-US/docs/Web/API/Selection_API https://developer.mozilla.org/en-US/docs/Web/API/Selection And of course I've added a note to the Fx 55 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11) > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10) > > Selection is changed: > > > > * containsNode(Node node, boolean allowPartialContainment); > > -> containsNode(Node node, optional boolean allowPartialContainment = > > false); > > * collapse(Node node, unsigned long offset); > > -> collapse(Node? node, optional unsigned long offset = 0); > > * extend(Node node, unsigned long offset); > > -> extend(Node node, optional unsigned long offset = 0); > > * Added: deleteFromDocument(); > > Thanks for the really useful changes summary Masayuki! > > I have added these changes to the relevant pages, including making sure all > the browser compat info is up-to-date: > > https://developer.mozilla.org/en-US/docs/Web/API/Selection/collapse > https://developer.mozilla.org/en-US/docs/Web/API/Selection/extend > https://developer.mozilla.org/en-US/docs/Web/API/Selection/containsNode > https://developer.mozilla.org/en-US/docs/Web/API/Selection/deleteFromDocument Thank you for your work! > https://developer.mozilla.org/en-US/docs/Web/API/Selection_API > https://developer.mozilla.org/en-US/docs/Web/API/Selection Might be unnecessary the new row to the compat table of these document because it looks messy to me. How about to restrict only big changes like adding new method/attribute? The compat table becomes too big if each method's behavior changes would be added.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #12) > (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11) > > > https://developer.mozilla.org/en-US/docs/Web/API/Selection_API > > https://developer.mozilla.org/en-US/docs/Web/API/Selection > > Might be unnecessary the new row to the compat table of these document > because it looks messy to me. How about to restrict only big changes like > adding new method/attribute? The compat table becomes too big if each > method's behavior changes would be added. I think you've got a good point here. I've removed the rows about the method/property changes.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: