1321623 - Implement setBaseAndExtent

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Andrew Overholt [:overholt]]

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

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

Attachment: test

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69d34c014e23453426dbd17c60fbd08bdd8b20ab&exclusion_profile=false

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Mats Palmgren (inactive)]

(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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

What's wrong with assert_throws()?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Mats Palmgren (inactive)]

Do we need to send an intent-to-ship about this?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Mats Palmgren (inactive)]

OK, I'll skip it then.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 13

[(no longer active)]

Does this patch move the needle on any of our GSuites tests?

Comment 14

[Andrew Overholt [:overholt]]

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.

Comment 15

[(no longer active)]

(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...

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c389436be671
https://hg.mozilla.org/mozilla-central/rev/ddfcb1823adc

Comment 17

[Mats Palmgren (inactive)]

This is the first time I've heard of "GSuites tests". :-)
Anyway, it looks like you got your answer from someone else.

Comment 18

[(no longer active)]

(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.

Comment 19

[Chris Mills [:cmills]]

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!

Comment 20

[Mats Palmgren (inactive)]

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!

Comment 21

[Chris Mills [:cmills]]

(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?

Comment 22

[Mats Palmgren (inactive)]

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...

Comment 23

[Chris Mills [:cmills]]

(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 ;-)

Comment 24

[Mats Palmgren (inactive)]

That's excellent, thanks.