All Selection methods should refuse to add range that have a different root (collapse, extend, selectAllChildren, etc.)

RESOLVED FIXED in Firefox 57

Status

()

Core
Selection
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

This is per spec.  Bug 1341137 fixed addRange(), but it should be true for anything.  Chrome and Edge appear to match the spec here (testing with collapse()), but WebKit does not.  We should certainly be consistent with our own behavior for addRange().
Comment hidden (mozreview-request)
Suggested reviewers for Selection stuff?  (Note, I will be mostly vanishing after tomorrow, so if these aren't reviewed quickly then they'll probably have to wait to August 2, unless they can be landed without any changes.)
Flags: needinfo?(bugs)
Try mats or masayuki? (or sneak this into my queue when it is open)
Flags: needinfo?(bugs)
Attachment #8861452 - Flags: review?(masayuki)
Attachment #8861452 - Flags: review?(masayuki) → review?(bugs)
Comment on attachment 8861452 [details]
Bug 1359397 - Don't allow Selection in nodes not in the document

https://reviewboard.mozilla.org/r/133444/#review136308

::: layout/generic/nsSelection.cpp:5201
(Diff revision 1)
> +  if (aParentNode.NodeType() == nsIDOMNode::DOCUMENT_TYPE_NODE) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_NODE_TYPE_ERR);
> +    return;
> +  }

I'm curious, where does this define in Selection API spec?

::: layout/generic/nsSelection.cpp:5583
(Diff revision 1)
>  Selection::Extend(nsINode& aParentNode, uint32_t aOffset, ErrorResult& aRv)
>  {
>    // First, find the range containing the old focus point:
>    if (!mAnchorFocusRange) {
>      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>      return;
>    }
>  
>    if (!mFrameSelection) {
>      aRv.Throw(NS_ERROR_NOT_INITIALIZED); // Can't do selection
>      return;
>    }
>  
> +  if (!HasSameRoot(aParentNode)) {
> +    return;
> +  }

Don't we need to check if aParentNode is a document node like Collapse()?

::: layout/generic/nsSelection.cpp:6716
(Diff revision 1)
>  Selection::SetBaseAndExtent(nsINode& aAnchorNode, uint32_t aAnchorOffset,
>                              nsINode& aFocusNode, uint32_t aFocusOffset,
>                              ErrorResult& aRv)
>  {
> -  if (!mFrameSelection) {
> +  if (!mFrameSelection || !HasSameRoot(aAnchorNode) ||
> +      !HasSameRoot(aFocusNode)) {

Same here, don't we need to check if aAnchorNode nor aFocusNode is a document node?

::: testing/web-platform/tests/selection/removeAllRanges.html:17
(Diff revision 1)
>  
>  var range = rangeFromEndpoints([paras[0].firstChild, 0, paras[0].firstChild, 1]);
>  
>  for (var i = 0; i < testRanges.length; i++) {
> +    var endpoints = eval(testRanges[i]);
> +    if (!isSelectableNode(endpoints[0]) || !isSelectableNode(endpoints[2])) {

If endpoints[0|2] is selectable, but endpoints[0|2].childNodes.item(endpoints[1|3]) is not selectable (when the node has children), this does not work fine. (Although, I guess there are no such testcases.)
Attachment #8861452 - Flags: review-
(FYI: This patch doesn't include changes in webidl. So, I can review this completely.)
Comment on attachment 8861452 [details]
Bug 1359397 - Don't allow Selection in nodes not in the document

indeed.
Attachment #8861452 - Flags: review?(bugs)
Comment hidden (mozreview-request)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4)
> I'm curious, where does this define in Selection API spec?

As it says in the commit message, this is not currently in the spec, but matches Edge and Chrome and our current behavior for this case, while the spec is only like WebKit (and only because WebKit never throws for a DocumentType range endpoint).  So I think the spec is wrong.  https://github.com/w3c/selection-api/pull/86

> Don't we need to check if aParentNode is a document node like Collapse()?
> . . .
> Same here, don't we need to check if aAnchorNode nor aFocusNode is a
> document node?

The spec and Chrome both don't throw in this case.  I guess this is a bug, though, you're right -- we should be consistent.  I'm not sure which way the spec should go.  I commented in the PR.

> If endpoints[0|2] is selectable, but
> endpoints[0|2].childNodes.item(endpoints[1|3]) is not selectable (when the
> node has children), this does not work fine. (Although, I guess there are no
> such testcases.)

Why doesn't it work?  (needinfo on this point)
Flags: needinfo?(masayuki)
>> If endpoints[0|2] is selectable, but
>> endpoints[0|2].childNodes.item(endpoints[1|3]) is not selectable (when the
>> node has children), this does not work fine. (Although, I guess there are no
>> such testcases.)
> 
> Why doesn't it work?  (needinfo on this point)

Ah, isSelection does NOT check if it's actually a selectable node. Okay, that's fine.
Flags: needinfo?(masayuki)
Comment on attachment 8861452 [details]
Bug 1359397 - Don't allow Selection in nodes not in the document

The exact details of when to throw what still need to be worked out, but Ryosuke has not replied on the spec issue.  Either way, I think this patch is still an improvement, so we should take it until the spec issue is worked out.

Just to be clear, it's never possible to set a range endpoint to inside a DocumentType node, and this always throws: https://dom.spec.whatwg.org/#concept-range-bp-set  The question here is only whether we should return silently because it's in a different tree before we throw for being a DocumentType node, and for collapse() also whether we throw IndexSizeError before returning silently.

I will presumably not be able to work on this further after Sunday, so if you'd like me to land this, I'll need review before then.
Attachment #8861452 - Flags: review?(masayuki)
Comment hidden (mozreview-request)
The new review request is just updated to apply cleanly to tip.  There's only one other change, which is an added "endpoints.length &&" to handle the case of no range properly, a bug I noticed when looking at the patch again.
Should I review only the latter one? I don't understand why you duplicated the review request with MozReview, though...
Flags: needinfo?(ayg)
If you want to clean them up, go to REVIEW SUMMARY of the review requests:
https://reviewboard.mozilla.org/r/133442/
Then, choose [Close] -> [Discarded]. Then, push only new patch from your local repository again.
FYI: I'll be offline tomorrow due to Japanese national holiday.
Attachment #8861452 - Attachment is obsolete: true
Attachment #8861452 - Flags: review?(masayuki)
I don't know why it did that.  It has the same MozReview-Commit-ID, so shouldn't it be the same review?  I did have to copy and paste the content of the old review to a new one, because I lost the contents of my hard drive and am working with a fresh install of everything, so presumably something didn't carry over.
Flags: needinfo?(ayg)
(In reply to Aryeh Gregor (:ayg) (working until August 13, then no longer with Mozilla) from comment #16)
> I don't know why it did that.  It has the same MozReview-Commit-ID, so
> shouldn't it be the same review?

Yeah, usually, it should be replaced with new one.

> I did have to copy and paste the content
> of the old review to a new one, because I lost the contents of my hard drive
> and am working with a fresh install of everything, so presumably something
> didn't carry over.

Ah, perhaps, the changeset ID becomes different by the accident. That caused this odd behavior.
Comment on attachment 8895791 [details]
Bug 1359397 - Don't allow Selection in nodes not in the document;

https://reviewboard.mozilla.org/r/167110/#review172288

::: dom/base/Selection.cpp:2523
(Diff revision 1)
> +  if (!HasSameRoot(aContainer)) {
> +    return;
> +  }

So, just do nothing is correct?

I don't have much time to check the spec due to too late time in JST now. Please check it and if you don't mind, I'd like you to add comment like "do nothing" here. Then, when other developers look for a bug, the comment explicitly tells them that not throwing error here is expected.

::: dom/base/Selection.cpp:2914
(Diff revision 1)
> +  if (!HasSameRoot(aContainer)) {
> +    return;
> +  }

same.

::: dom/base/Selection.cpp:3211
(Diff revision 1)
> +
> +  if (!HasSameRoot(aNode)) {
> +    return;
> +  }

Same.

::: dom/base/Selection.cpp:4014
(Diff revision 1)
> -  if (!mFrameSelection) {
> +  if (!mFrameSelection || !HasSameRoot(aAnchorNode) ||
> +      !HasSameRoot(aFocusNode)) {
>      return;
>    }

Same but for the comment, I'd like you to check HasSameRoot() with an independent |if|.
Attachment #8895791 - Flags: review?(masayuki) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
MozReview is giving me errors when submitting try pushes on this bug, so I did a new one manually:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77528c8d19576ef7ae5ca71e49bc5159e45602e0

Comment 22

9 months ago
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/0bda6393453e
Don't allow Selection in nodes not in the document; r=masayuki
https://hg.mozilla.org/mozilla-central/rev/0bda6393453e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1388075
Blocks: 1384161
You need to log in before you can comment on or make changes to this bug.