Bug 148733 – Range.comparePoint shouldn't throw an exception if the range and the node are in the same detached tree

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 260467 [details]
Test case that pass in Firefox and fail in WebKit

comparePoint currently throws WrongDocumentError if the node passed in the first argument is not in the document.
However, DOM4 spec (http://www.w3.org/TR/dom/#dom-range-comparepoint) allows it as long as the range's boundary point in the same detached tree.

Firefox follows the spec.

[Radar WebKit Bug Importer]

<rdar://problem/22551214>

[Ryosuke Niwa]

Created attachment 260474 [details]
Fixes the bug

[Chris Dumez]

Regressions: Unexpected text-only failures (2)
  http/tests/w3c/dom/ranges/Range-comparePoint.html [ Failure ]
  http/tests/w3c/dom/ranges/Range-set.html [ Failure ]

[Chris Dumez]

Comment on attachment 260474 [details]
Fixes the bug

r- due to w3c tests failures (may need a simple rebaseline)

[Ryosuke Niwa]

(In reply to comment #4)
> Comment on attachment 260474 [details]
> Fixes the bug
> 
> r- due to w3c tests failures (may need a simple rebaseline)

Oh oops, I fixed those tests and forgot to rebase :(

[Ryosuke Niwa]

Created attachment 260478 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 260479 [details]
Fixed a typo

[Chris Dumez]

Comment on attachment 260479 [details]
Fixed a typo

View in context: https://bugs.webkit.org/attachment.cgi?id=260479&action=review

> Source/WebCore/dom/Range.cpp:266
> +        if (!refNode->inDocument() && !commonAncestorContainer(refNode, &startContainer()))

This looks odd. Why would we do this check *after* the checkNodeWOffset() call? The spec says:
1. If node’s root is different from the context object’s root, throw a WrongDocumentError exception. (Isn't this the &refNode->document() != &ownerDocument() check above)?
2. If node is a doctype, throw an InvalidNodeTypeError exception. (done in checkNodeWOffset)
3. If offset is greater than node’s length, throw an IndexSizeError exception. (done in checkNodeWOffset)

[Ryosuke Niwa]

Comment on attachment 260479 [details]
Fixed a typo

View in context: https://bugs.webkit.org/attachment.cgi?id=260479&action=review

>> Source/WebCore/dom/Range.cpp:266
>> +        if (!refNode->inDocument() && !commonAncestorContainer(refNode, &startContainer()))
> 
> This looks odd. Why would we do this check *after* the checkNodeWOffset() call? The spec says:
> 1. If node’s root is different from the context object’s root, throw a WrongDocumentError exception. (Isn't this the &refNode->document() != &ownerDocument() check above)?
> 2. If node is a doctype, throw an InvalidNodeTypeError exception. (done in checkNodeWOffset)
> 3. If offset is greater than node’s length, throw an IndexSizeError exception. (done in checkNodeWOffset)

Because checkNodeWOffset is O(1) for text nodes, etc... but commonAncestorContainer is always O(n).

[Chris Dumez]

Comment on attachment 260479 [details]
Fixed a typo

View in context: https://bugs.webkit.org/attachment.cgi?id=260479&action=review

>>> Source/WebCore/dom/Range.cpp:266
>>> +        if (!refNode->inDocument() && !commonAncestorContainer(refNode, &startContainer()))
>> 
>> This looks odd. Why would we do this check *after* the checkNodeWOffset() call? The spec says:
>> 1. If node’s root is different from the context object’s root, throw a WrongDocumentError exception. (Isn't this the &refNode->document() != &ownerDocument() check above)?
>> 2. If node is a doctype, throw an InvalidNodeTypeError exception. (done in checkNodeWOffset)
>> 3. If offset is greater than node’s length, throw an IndexSizeError exception. (done in checkNodeWOffset)
> 
> Because checkNodeWOffset is O(1) for text nodes, etc... but commonAncestorContainer is always O(n).

Doing it in this order means you're going to throw the wrong exception if the offset is wrong AND the nodes don't have the same root (you will throw an index error instead of a wrong document error).

[Chris Dumez]

Comment on attachment 260479 [details]
Fixed a typo

View in context: https://bugs.webkit.org/attachment.cgi?id=260479&action=review

>>>> Source/WebCore/dom/Range.cpp:266
>>>> +        if (!refNode->inDocument() && !commonAncestorContainer(refNode, &startContainer()))
>>> 
>>> This looks odd. Why would we do this check *after* the checkNodeWOffset() call? The spec says:
>>> 1. If node’s root is different from the context object’s root, throw a WrongDocumentError exception. (Isn't this the &refNode->document() != &ownerDocument() check above)?
>>> 2. If node is a doctype, throw an InvalidNodeTypeError exception. (done in checkNodeWOffset)
>>> 3. If offset is greater than node’s length, throw an IndexSizeError exception. (done in checkNodeWOffset)
>> 
>> Because checkNodeWOffset is O(1) for text nodes, etc... but commonAncestorContainer is always O(n).
> 
> Doing it in this order means you're going to throw the wrong exception if the offset is wrong AND the nodes don't have the same root (you will throw an index error instead of a wrong document error).

Wait, I misread. Doesn't this mean you'll throw the WRONG_DOCUMENT_ERR *only* if the offset is also wrong? What if the nodes are detached, don't have the same root and the offset is right?

[Ryosuke Niwa]

(In reply to comment #11)
> Comment on attachment 260479 [details]
> Fixed a typo
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260479&action=review
> 
> >>>> Source/WebCore/dom/Range.cpp:266
> >>>> +        if (!refNode->inDocument() && !commonAncestorContainer(refNode, &startContainer()))
> >>> 
> >>> This looks odd. Why would we do this check *after* the checkNodeWOffset() call? The spec says:
> >>> 1. If node’s root is different from the context object’s root, throw a WrongDocumentError exception. (Isn't this the &refNode->document() != &ownerDocument() check above)?
> >>> 2. If node is a doctype, throw an InvalidNodeTypeError exception. (done in checkNodeWOffset)
> >>> 3. If offset is greater than node’s length, throw an IndexSizeError exception. (done in checkNodeWOffset)
> >> 
> >> Because checkNodeWOffset is O(1) for text nodes, etc... but commonAncestorContainer is always O(n).
> > 
> > Doing it in this order means you're going to throw the wrong exception if the offset is wrong AND the nodes don't have the same root (you will throw an index error instead of a wrong document error).
> 
> Wait, I misread. Doesn't this mean you'll throw the WRONG_DOCUMENT_ERR
> *only* if the offset is also wrong? What if the nodes are detached, don't
> have the same root and the offset is right?

In that case,compareBoundaryPoints will throw WRONG_DOCUMENT_ERR.  I explained all of this in the change log.

[Chris Dumez]

Comment on attachment 260479 [details]
Fixed a typo

View in context: https://bugs.webkit.org/attachment.cgi?id=260479&action=review

This makes sense now, sorry for not reading the changelog XD r=me.

> Source/WebCore/ChangeLog:11
> +        WRONG_DOCUMENT_ERR is continued to be thrown by compareBoundaryPoints later in the function when the root

s/continued to be/still/ ?

> Source/WebCore/ChangeLog:14
> +        There is one subtly here that we need to throw WRONG_DOCUMENT_ERR instead of INDEX_SIZE_ERR when refNode

"subtlety"

> LayoutTests/fast/dom/Range/range-comparePoint-detached-nodes-expected.txt:3
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

Where is my "TEST COMPLETE"? :)

> LayoutTests/fast/dom/Range/range-comparePoint-detached-nodes.html:25
> +shouldThrow("var range = new Range; range.setStart(detachedDiv, 0); range.comparePoint(detachedP, 0);");

I think we should specify the exception string as second parameter since we really want to make sure a WrongDocumentError is thrown.

> LayoutTests/fast/dom/Range/range-comparePoint-detached-nodes.html:26
> +shouldThrow("var range = new Range; range.setStart(spanInDetachedDiv1, 0); range.comparePoint(spanInDetachedP, 0);");

ditto.

> LayoutTests/fast/dom/Range/range-comparePoint-detached-nodes.html:28
> +</script>

You're missing the js-test-post.js

[Ryosuke Niwa]

Committed r189327: <http://trac.webkit.org/changeset/189327>