setStart, setEnd don't check container nodes for documents




DOM: Core & HTML
12 years ago
5 years ago


(Reporter: WeirdAl, Assigned: WeirdAl)



Windows XP

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



12 years ago
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

12 years ago
Created attachment 212445 [details]

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

Comment 2

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

Comment 3

12 years ago
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.
Assignee: traversal-range → ajvincent
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

12 years ago
*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.
No longer blocks: 319768
Last Resolved: 12 years ago
Resolution: --- → INVALID

Comment 6

12 years ago
Created attachment 212488 [details]
testcase showing correct behavior
Attachment #212445 - Attachment is obsolete: true
Er... the order in which you set the endpoints should not matter.  If it does, we have a bug.
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.