Implement setBaseAndExtent

RESOLVED FIXED in Firefox 53

Status

()

Core
Selection
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: overholt, Assigned: mats)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla53
dev-doc-complete
Points:
---

Firefox Tracking Flags

(platform-rel +, firefox53 fixed)

Details

(Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleDocs] , URL)

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
It seems Chrome and Edge have this but we don't (and neither does IE).

https://w3c.github.io/selection-api/#widl-Selection-setBaseAndExtent-void-Node-anchorNode-unsigned-long-anchorOffset-Node-focusNode-unsigned-long-focusOffset
(Reporter)

Updated

6 months ago
Blocks: 1307024
(Assignee)

Comment 1

5 months ago
Created attachment 8819682 [details] [diff] [review]
fix
Assignee: nobody → mats
Attachment #8819682 - Flags: review?(bugs)
(Assignee)

Comment 2

5 months ago
Created attachment 8819683 [details] [diff] [review]
test

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69d34c014e23453426dbd17c60fbd08bdd8b20ab&exclusion_profile=false
Attachment #8819683 - Flags: review?(bugs)

Comment 3

5 months ago
Comment on attachment 8819682 [details] [diff] [review]
fix


>+++ b/dom/base/nsISelection.idl
I don't see any need to make changes to .idl file. .webidl should be enough, that is after all
what is exposed to JS
>+  void SetBaseAndExtent(nsINode& aAnchorNode, uint32_t anchorOffset,
>+                        nsINode& aFocusNode, uint32_t focusOffset,
>+                        mozilla::ErrorResult& aRv);
Please be consistent with argument naming. aName


>+NS_IMETHODIMP
>+Selection::SetBaseAndExtent(nsIDOMNode* aAnchorNode, uint32_t aAnchorOffset,
>+                            nsIDOMNode* aFocusNode, uint32_t aFocusOffset)
>+{
>+  ErrorResult result;
>+  nsCOMPtr<nsINode> anchor = do_QueryInterface(aAnchorNode);
>+  if (!anchor) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+  nsCOMPtr<nsINode> focus = do_QueryInterface(aFocusNode);
>+  if (!focus) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+  SetBaseAndExtent(*anchor, aAnchorOffset, *focus, aFocusOffset, result);
>+  return result.StealNSResult();
>+}
>+
If the change to .idl isn't done, this isn't needed either.


>+void
>+Selection::SetBaseAndExtent(nsINode& aAnchorNode, uint32_t aAnchorOffset,
>+                            nsINode& aFocusNode, uint32_t aFocusOffset,
>+                            ErrorResult& aRv)
>+{
>+  if (!mFrameSelection) {
>+    return;
>+  }

Does this miss the first step
"If anchorOffset is longer than anchorNode's length or if focusOffset is longer than focusNode's length, throw an IndexSizeError exception and abort these steps."
or is that hidden underneath nsRange::CreateRange? If the latter, please add a comment, otherwise add a check.
(if I read the code right, it is about that latter that nsRange::CreateRange throws)
Attachment #8819682 - Flags: review?(bugs) → review+

Comment 4

5 months ago
Comment on attachment 8819683 [details] [diff] [review]
test

>+<div id=log></div>
>+<script src=/resources/testharness.js></script>
>+<script src=/resources/testharnessreport.js></script>
>+<script src=common.js></script>
oh, common.js has testRanges defined.
Took a bit time to figure out where that was coming from.
Attachment #8819683 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

5 months ago
(In reply to Olli Pettay [:smaug] from comment #3)
> >+++ b/dom/base/nsISelection.idl
> I don't see any need to make changes to .idl file. .webidl should be enough,
> that is after all what is exposed to JS

OK, I skipped that part then.

> ... or is that hidden underneath nsRange::CreateRange?

Yes, nsRange::CreateRange returns IndexSizeError when that happens.
I added a comment to that effect.

And fixed the other nits.

Comment 6

5 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c389436be671
Implement DOM Selection.setBaseAndExtent().  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddfcb1823adc
WPT tests for DOM Selection.setBaseAndExtent().  r=smaug
What's wrong with assert_throws()?

Comment 8

5 months ago
wpt being a bit badly documented and having many helper methods which only a few people know about? Other than that, probably nothing. But doesn't really matter either.
(Assignee)

Comment 9

5 months ago
Do we need to send an intent-to-ship about this?
Flags: needinfo?(bugs)

Comment 10

5 months ago
I would say no given that this is just a simple method implemented already in other browsers.
Of course intent-to-ship doesn't harm.
Flags: needinfo?(bugs)
(Assignee)

Comment 11

5 months ago
OK, I'll skip it then.

Comment 12

5 months ago
(I hope others don't disagree too much. Our intent-to-ship process is still too vague.)

Updated

5 months ago
Keywords: dev-doc-needed
Does this patch move the needle on any of our GSuites tests?
Flags: needinfo?(mats)
(Reporter)

Comment 14

5 months ago
This is probably obvious but I'll say it anyyway: we'd have to spoof the UA to be Chrome for that to make a difference, I think. AFAIK they don't feature-detect this but rather do it based on UA string. See https://bugzilla.mozilla.org/show_bug.cgi?id=1307024#c15 for more info.
(In reply to Andrew Overholt [:overholt] from comment #14)
> This is probably obvious but I'll say it anyyway: we'd have to spoof the UA
> to be Chrome for that to make a difference, I think. AFAIK they don't
> feature-detect this but rather do it based on UA string. See
> https://bugzilla.mozilla.org/show_bug.cgi?id=1307024#c15 for more info.

If I read that correctly, that's a "no" response to comment 13, which sucks.  We should contact the GSuites team and ask them to switch to feature detecting this if we're going to see any benefits from this for GSuites...
https://hg.mozilla.org/mozilla-central/rev/c389436be671
https://hg.mozilla.org/mozilla-central/rev/ddfcb1823adc
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Updated

5 months ago
platform-rel: --- → +
Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleDocs]
(Assignee)

Comment 17

5 months ago
This is the first time I've heard of "GSuites tests". :-)
Anyway, it looks like you got your answer from someone else.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #17)
> This is the first time I've heard of "GSuites tests". :-)

This is about a project run by the Hasal team to test our performance on Google Docs, Sheets and Slides.  Bug 1307024 was found and filed during that testing, so we are doing this in the hopes of asking the Google Docs team to serve us code similar to what they serve to Chrome using this API to potentially improve our performance here.
I have created a page for the setBaseAndExtent() method:

https://developer.mozilla.org/en-US/docs/Web/API/Selection/setBaseAndExtent

I've also updated support info in other places as appropriate:

https://developer.mozilla.org/en-US/docs/Web/API/Selection
https://developer.mozilla.org/en-US/docs/Web/API/Selection_API

And I've added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM_HTML_DOM

https://developer.mozilla.org/en-US/docs/Web/API/Selection

Let me know if this all looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 20

3 months ago
There's a typo in the Syntax "containsNode" should be "setBaseAndExtent":
https://developer.mozilla.org/en-US/docs/Web/API/Selection/setBaseAndExtent

Perhaps you should mention in the Note that the direction is affected
by anchor/focus order?  The direction basically says which side
the caret is on, which matters for any keyboard command that might
follow (Shift+Left_Arrow etc).  (Not super-important though if you
think it's too much detail and prefer to skip it...)

The "Browser compatibility" sections should probably say "Yes" in
the Chrome/Safari/Edge columns.  I don't know versions though.
I suspect Chrome/Safari have had it for a long time.

In https://developer.mozilla.org/en-US/docs/Web/API/Selection
I'm pretty sure "modify()" is also a Yes for Chrome/Safari at least.

Looks good otherwise, thanks for documenting it!
(In reply to Mats Palmgren (:mats) from comment #20)
> There's a typo in the Syntax "containsNode" should be "setBaseAndExtent":
> https://developer.mozilla.org/en-US/docs/Web/API/Selection/setBaseAndExtent
> 
> Perhaps you should mention in the Note that the direction is affected
> by anchor/focus order?  The direction basically says which side
> the caret is on, which matters for any keyboard command that might
> follow (Shift+Left_Arrow etc).  (Not super-important though if you
> think it's too much detail and prefer to skip it...)
> 
> The "Browser compatibility" sections should probably say "Yes" in
> the Chrome/Safari/Edge columns.  I don't know versions though.
> I suspect Chrome/Safari have had it for a long time.
> 
> In https://developer.mozilla.org/en-US/docs/Web/API/Selection
> I'm pretty sure "modify()" is also a Yes for Chrome/Safari at least.
> 
> Looks good otherwise, thanks for documenting it!

Thanks for the review Mats - I've fixed all your points. I added a bit to the note - can you have a read and tell me whether I'm on the right track with this?
(Assignee)

Comment 22

3 months ago
The "move in the opposite way to what you intended" seems a little confusing.

The direction in this case works the same as if you drag-select using
the mouse - dragging from right-to-left creates a "backwards" selection
and leaves the caret on the left side, so Shift+Left_Arrow will extend
the selection left-ward, whereas drag-selecting from left-to-right
leaves the caret at the right side and Shift+Left_Arrow will narrow
the selection.  I'm not sure how to best describe that in fewer words
though.  Perhaps we could simply say which side the caret is on?
But that assumes the reader knows about what a caret is...
(In reply to Mats Palmgren (:mats) from comment #22)
> The "move in the opposite way to what you intended" seems a little confusing.
> 
> The direction in this case works the same as if you drag-select using
> the mouse - dragging from right-to-left creates a "backwards" selection
> and leaves the caret on the left side, so Shift+Left_Arrow will extend
> the selection left-ward, whereas drag-selecting from left-to-right
> leaves the caret at the right side and Shift+Left_Arrow will narrow
> the selection.  I'm not sure how to best describe that in fewer words
> though.  Perhaps we could simply say which side the caret is on?
> But that assumes the reader knows about what a caret is...

Ah ha, I get what you mean now. I've tried rewriting the note again ;-)
(Assignee)

Comment 24

3 months ago
That's excellent, thanks.
You need to log in before you can comment on or make changes to this bug.