327880 - setStart, setEnd don't check container nodes for documents

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Alex Vincent [:WeirdAl]]

In writing XPath Generator code, I came upon DOM ranges as a shortcut for a small piece of it - namely, to get a common ancestor for the target node and the ancestor node.

Unfortunately, when one of these nodes is a child node of the document, then the call to nsContentUtils::InSameDoc() results in a QI of the document node to nsIContent.  Since documents don't implement nsIContent, it means that the range gets incorrectly collapsed.

I'm willing to fix this.  The question I have is where should the fix go?  nsContentUtils::InSameDoc is only used by nsRange::SetStart and nsRange::SetEnd.

Testcase coming up.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

The basic failure condition is pretty easy to spot.  Look at the third startNode/endNode combination after clicking the button.

Comment 2

[Alex Vincent [:WeirdAl]]

bah, comment 0:  that should have been "to get a common ancestor for the target node and the context node"

Comment 3

[Alex Vincent [:WeirdAl]]

I asked sicking if we cared about nodes not attached to documents (say, nodes that are instead descending from a document fragment).  This was his answer:

<sicking> yes, we don't want nodes without a GetCurrentDoc to have ranges
<sicking> for now i don't think ranges can handle it
<sicking> if someone cleans up the implementation we can talk :)

Personally, I think that deserves a XXX comment in the patch.  I'm going to patch this in nsContentUtils, using nsINode::GetOwnerDoc().  It's the simplest solution.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well.  Do we want to be using GetOwnerDoc for the check?  Or GetCurrentDoc?  Or do we want some other check altogether (like "root of the node subtree is the same")?

For example, given a range with one endpoint in XBL bound to doc A and the other in XBL bound to doc B (but the same XBL, so the same owner doc), are we OK?  With XBL2, that situation cannot be prevented with a GetOwnerDoc() check.

Comment 5

[Alex Vincent [:WeirdAl]]

*sigh* Once again, I have forgotten that the end point of a range must be set first, and the start point of a range set second.

Comment 6

[Alex Vincent [:WeirdAl]]

Attachment: testcase showing correct behavior

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er... the order in which you set the endpoints should not matter.  If it does, we have a bug.