Implement setBaseAndExtent

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: overholt, Assigned: mats)

Tracking

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

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel +, firefox53 fixed)

Details

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

Attachments

(2 attachments)

Reporter

Updated

3 years ago
Blocks: 1307024
Assignee

Comment 1

3 years ago
Posted patch fixSplinter Review
Assignee: nobody → mats
Attachment #8819682 - Flags: review?(bugs)
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 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

3 years 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

3 years 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()?
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

2 years ago
Do we need to send an intent-to-ship about this?
Flags: needinfo?(bugs)
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

2 years ago
OK, I'll skip it then.
(I hope others don't disagree too much. Our intent-to-ship process is still too vague.)

Comment 13

2 years ago
Does this patch move the needle on any of our GSuites tests?
Flags: needinfo?(mats)
Reporter

Comment 14

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

Comment 15

2 years ago
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c389436be671
https://hg.mozilla.org/mozilla-central/rev/ddfcb1823adc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Reporter

Updated

2 years ago
platform-rel: --- → +
Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleDocs]
Assignee

Comment 17

2 years 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)

Comment 18

2 years ago
(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.
Assignee

Comment 20

2 years 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

2 years 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

2 years ago
That's excellent, thanks.
Depends on: 1388075
You need to log in before you can comment on or make changes to this bug.