Closed Bug 1359387 Opened 6 years ago Closed 6 years ago

Support Selection.empty(), Selection.setPosition()

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 1359387 - Support Selection.empty()/setPosition()
59 bytes, text/x-review-board-request
smaug
: review+
Details 
https://github.com/w3c/selection-api/issues/36
https://github.com/w3c/selection-api/issues/38

These are from WebKit/Blink.  They're just aliases for removeAllRanges() and collapse().  They aren't good for much, but they are used in web content, so Blink wanted them added to the spec.  I don't see any reason to not remove them.  Edge reportedly supports empty().
Tests submitted upstream: https://github.com/w3c/web-platform-tests/pull/5684

Blink, WebKit, and Edge all support both of these methods.
If you don't want to review yourself, could you pick someone else to review?
Flags: needinfo?(bugs)
I'm trying to reduce the very high reviewing throughput, so I'm experimenting keeping queue open only when I'm not working, and then every morning closing the queue and going through the existing requests.

mats and masayuki are other possible reviewers for this code, I think.

(r+ for the .webidl change)
Flags: needinfo?(bugs)
Attachment #8861400 - Flags: review?(masayuki)
Attachment #8861400 - Flags: review?(masayuki) → review?(bugs)
Comment on attachment 8861400 [details]
Bug 1359387 - Support Selection.empty()/setPosition()

https://reviewboard.mozilla.org/r/133394/#review136294

::: dom/webidl/Selection.webidl:29
(Diff revision 1)
> -    //void      empty();
> +		[Throws]
> +    void      empty();
>  		[Throws, BinaryName="collapseJS"]
>      void      collapse(Node? node, optional unsigned long offset = 0);
> -    //void      setPosition(Node? node, optional unsigned long offset = 0);
> +		[Throws]
> +    void      setPosition(Node? node, optional unsigned long offset = 0);

Although, this needs additional review of DOM peer.

::: dom/webidl/Selection.webidl:29
(Diff revision 1)
> -    //void      empty();
> +		[Throws]
> +    void      empty();
>  		[Throws, BinaryName="collapseJS"]
>      void      collapse(Node? node, optional unsigned long offset = 0);
> -    //void      setPosition(Node? node, optional unsigned long offset = 0);
> +		[Throws]
> +    void      setPosition(Node? node, optional unsigned long offset = 0);

Please replace tabs with whitespaces.

And cannot define this them as:

> [Throws, BinaryName="RemoveAllRanges"]
> void empty();

and

> [Throws, BinaryName="collapseJS"]
> void setPoisition(...);

?  Otherwise, r+ for the C++ part, but still needs DOM peer's review for webidl change.
Attachment #8861400 - Flags: review+
Comment on attachment 8861400 [details]
Bug 1359387 - Support Selection.empty()/setPosition()

https://reviewboard.mozilla.org/r/133394/#review136324

Ah, masayuki is right. Using BinaryName would be quite handy here, especially since the spec explicitly talks about methods being aliases.
And don't use tabs but spaces.
Attachment #8861400 - Flags: review?(bugs) → review+
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/24b122474ff5
Support Selection.empty()/setPosition() r=masayuki,smaug
https://hg.mozilla.org/mozilla-central/rev/24b122474ff5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I've documented this by adding details on the aliases to (and updating browser compat info on):

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 Fx55 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 #10)
> I've documented this by adding details on the aliases to (and updating
> browser compat info on):
> 
> 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 Fx55 rel notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM

Thanks, looks fine to me. Although, it might be better to separate the row of the compat table for each method.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #11)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #10)
> > I've documented this by adding details on the aliases to (and updating
> > browser compat info on):
> > 
> > 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 Fx55 rel notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM
> 
> Thanks, looks fine to me. Although, it might be better to separate the row
> of the compat table for each method.

OK, done.
You need to log in before you can comment on or make changes to this bug.