Bug 108274

Summary: compareDocumentPosition reports disconnected nodes as following each other
Product: WebKit Reporter: Richard Gibson <richard.gibson>
Component: DOMAssignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Minor CC: ap, cdumez, dglazkov, mkwst, nbarth, ojan.autocc, ojan, syoichi, tasak, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://jsfiddle.net/JUDGk/
Bug Depends on:    
Bug Blocks: 110007    
Attachments:
Description Flags
Patch none

Description Richard Gibson 2013-01-29 18:03:16 PST
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.
Comment 2 Mike West 2013-02-18 07:05:36 PST
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".
Comment 3 Mike West 2013-02-18 07:14:41 PST
Created attachment 188880 [details]
Patch
Comment 4 Mike West 2013-02-18 07:16:16 PST
Let's see what the bots say.
Comment 5 Mike West 2013-02-18 07:52:10 PST
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?
Comment 6 Dimitri Glazkov (Google) 2013-02-18 09:14:58 PST
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?
Comment 7 Mike West 2013-02-18 09:29:46 PST
(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.
Comment 8 Dimitri Glazkov (Google) 2013-02-18 09:31:58 PST
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.
Comment 9 Mike West 2013-02-18 09:55:24 PST
Comment on attachment 188880 [details]
Patch

Thanks Dimitri! Throwing this into the CQ.
Comment 10 WebKit Review Bot 2013-02-18 10:23:40 PST
Comment on attachment 188880 [details]
Patch

Clearing flags on attachment: 188880

Committed r143239: <http://trac.webkit.org/changeset/143239>
Comment 11 WebKit Review Bot 2013-02-18 10:23:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Alexey Proskuryakov 2014-02-28 22:41:14 PST
Pointer comparison is crazy talk. Undoing this in bug 120244.
Comment 13 Lucas Forschler 2019-02-06 09:03:26 PST
Mass moving XML DOM bugs to the "DOM" Component.