Update Selection.webidl to match spec

RESOLVED FIXED in Firefox 55

Status

()

Core
Selection
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

({dev-doc-complete})

unspecified
mozilla55
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Comment hidden (mozreview-request)
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 5

a year ago
mozreview-review
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 6

a year ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/447626910bd6
Update Selection.webidl to match spec r=smaug

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/447626910bd6
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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.