Bug 108274 – compareDocumentPosition reports disconnected nodes as following each other

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Richard Gibson]

Distinct from but probably related to #77882, compareDocumentPosition always returns 4 when comparing disconnected nodes, giving the bizarre situation where (a !== b && a.compareDocumentPosition( b ) === b.compareDocumentPosition( a )).

Expected Results: I'm not 100% certain, but probably 1. At any rate, if one comparison yields 4 (or 5) then the other should yield 2 (or 3) for symmetry.

[Richard Gibson]

jQuery ticket: http://bugs.jquery.com/ticket/13331 
Workaround: https://github.com/jquery/sizzle/commit/1c8aec91284af8d8c14447976235d5dd72b0d75e

[Mike West]

http://dom.spec.whatwg.org/#dom-node-comparedocumentposition says that nodes that aren't in the same tree ought to return "the result of adding DOCUMENT_POSITION_DISCONNECTED, DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC, and either DOCUMENT_POSITION_PRECEDING or DOCUMENT_POSITION_FOLLOWING, with the constraint that this is to be consistent, together".

[Mike West]

Created attachment 188880 [details]
Patch

[Mike West]

Let's see what the bots say.

[Mike West]

Hey Dimitri! After https://bugs.webkit.org/show_bug.cgi?id=103502 it seems like you might be in a good position to review this change, which seems to be addressing a similar problem.

Would you mind taking a look?

[Dimitri Glazkov (Google)]

Comment on attachment 188880 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:1792
> +    // If the two elements don't have a common root, they're not in the same tree.

You can do this much earlier by simply comparing treeScope() values. No need to calculate common ancestors :) Maybe that's better and clearer?

[Mike West]

(In reply to comment #6)
> (From update of attachment 188880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188880&action=review
> 
> > Source/WebCore/dom/Node.cpp:1792
> > +    // If the two elements don't have a common root, they're not in the same tree.
> 
> You can do this much earlier by simply comparing treeScope() values. No need to calculate common ancestors :) Maybe that's better and clearer?

Those are compared, like two if-blocks up (line 1179). It didn't seem to have the semantics I needed since it didn't trigger in this case, but its quite possible I misunderstood the code.

Since the ancestor chain is generated regardless, this seemed like a fairly low impact mechanism.

[Dimitri Glazkov (Google)]

Comment on attachment 188880 [details]
Patch

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

>>> Source/WebCore/dom/Node.cpp:1792
>>> +    // If the two elements don't have a common root, they're not in the same tree.
>> 
>> You can do this much earlier by simply comparing treeScope() values. No need to calculate common ancestors :) Maybe that's better and clearer?
> 
> Those are compared, like two if-blocks up (line 1179). It didn't seem to have the semantics I needed since it didn't trigger in this case, but its quite possible I misunderstood the code.
> 
> Since the ancestor chain is generated regardless, this seemed like a fairly low impact mechanism.

You're right. A Document vs. DocumentFragment case will pass treescope check.

[Mike West]

Comment on attachment 188880 [details]
Patch

Thanks Dimitri! Throwing this into the CQ.

[WebKit Review Bot]

Comment on attachment 188880 [details]
Patch

Clearing flags on attachment: 188880

Committed r143239: <http://trac.webkit.org/changeset/143239>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Pointer comparison is crazy talk. Undoing this in bug 120244.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.