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.
<rdar://problem/22551214>
Created attachment 260474 [details] Fixes the bug
Regressions: Unexpected text-only failures (2) http/tests/w3c/dom/ranges/Range-comparePoint.html [ Failure ] http/tests/w3c/dom/ranges/Range-set.html [ Failure ]
Comment on attachment 260474 [details] Fixes the bug r- due to w3c tests failures (may need a simple rebaseline)
(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 :(
Created attachment 260478 [details] Fixes the bug
Created attachment 260479 [details] Fixed a typo
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)
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).
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).
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 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.
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
Committed r189327: <http://trac.webkit.org/changeset/189327>