User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/419.3 (KHTML, like Gecko) Safari/419.3 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2) Gecko/2007121014 Firefox/3.0b2 The following JS: var element, range; element = document.createElement('div'); range = element.ownerDocument.createRange(); range.selectNodeContents(element); thows: NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR: The container of an boundary-point of a range is being set to either a node of an invalid type or a node with an ancestor of an invalid type.([Exception... "The container of an boundary-point of a range is being set to either a node of an invalid type or a node with an ancestor of an invalid type." code: "2" nsresult: "0x805c0002 (NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR)" at: range.selectNodeContents(element); Reproducible: Always Steps to Reproduce: 1. run the above mentioned JS Actual Results: throws an NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR error. Expected Results: This shouldn't throw an error. It's a regression issue in FF3b2.

I've confirmed this issue - and it's a serious regression, it prevents all forms of DOM insertion from working in the popular Prototype JavaScript Library. Additionally, the following test case also fails: var element, range; element = document.createElement('div'); range = document.createRange(); range.selectNodeContents(element); I've attached a test case of the original submission to verify against.

Also Opera throws an exception when an element, which is not in a document, is tried to be used in a range. (This was changed in bug 336381) For some reason bug 358106 changed the exception type from NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR to NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR.

And creating range using element which isn't in document or document fragment isn't allowed per DOM 2 Range recommendation: "The boundary-points of a Range must have a common ancestor container which is either a Document, DocumentFragment or Attr node."

"The boundary-points of a Range must have a common ancestor container which is either a Document, DocumentFragment or Attr node." that's the case here.

What you mean? If you create an element using document.createElement, the created element doesn't have any ancestors before it is somehow attached to dom (for example using insertBefore or appendChild). Or do you just mean that "that's the case here, that is why this doesn't work anymore. FF3 implements the W3C recommendation more strictly and correctly than FF2." :)

The definition of common ancestor container is rather vague in the specs. I initially read it as meaning that it needed to share the same document - but I think you might be right and in saying that it must actually be appended to the document.

The following (from http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Creating) seems to imply that sharing the same ownerDocument is enough: "Like some objects created using methods in the Document interface (such as Nodes and DocumentFragments), Ranges created via a particular document instance can select only content associated with that Document, or with DocumentFragments and Attrs for which that Document is the ownerDocument."

Well, this is quite clear "the content of a Range must be entirely within the subtree rooted by a single Document, DocumentFragment or Attr Node."

The only thing that seems clear to me is that that setting ranges on newly created nodes wasn't thought about when the specs were designed: - there's no relevant error for that particular case (BAD_BOUNDARYPOINTS_ERR doesn't seem appropriate, INVALID_NODE_TYPE_ERR is plain wrong), - as pointed above, there are some blatant contradictions in the specs themselves (is sharing the same ownerDocument sufficient, or must the selected node be appended to it?). Furthermore, API-wise, I don't see any reason why a newly created node couldn't be targeted by document.selectNodeContents. Previous implementations of FF understood the specs in the broad sense (i.e. sharing ownerDocument is enough). WebKit still does. Restricting the scope of range.selectNodeContent to appended nodes only is a serious regression issue and will notably break websites using any current version of Prototype. Given the lack of clarity of the specs in that area, and given the obvious use cases for selecting unappended nodes (I'm not going to pull the "don't break the web" argument here ;) ) it would seem unreasonable imho to dismiss this bug as being due to non spec-conforming code.

To me it is very clear what the spec says but because of backward compatibility supporting the old behavior is one option. (If there is an exception, it should be BAD_BOUNDARYPOINTS_ERR.)

(In reply to comment #10) > (If there is an exception, it should be BAD_BOUNDARYPOINTS_ERR.) > Actually, INVALID_NODE_TYPE_ERR is possible too.

(In reply to comment #9) > - as pointed above, there are some blatant contradictions in the specs > themselves (is sharing the same ownerDocument sufficient, or must the selected > node be appended to it?). There's really no discussion about this, the spec is clear that the "boundary-points of a Range must have a common ancestor container which is either a Document, DocumentFragment or Attr node." A node that is not appended to anything does not have an ancestor.

Agreed. Can we simply mark this as INVALID, or are we throwing the wrong exception?

(In reply to comment #13) > Agreed. Can we simply mark this as INVALID, or are we throwing the wrong > exception? > Do you realize that making selectNodeContents throw an error is a regression and will break every website using Prototype ?

Smaug, Jonas: Is there tangible benefits to leaving the code this way (throwing an exception when an element is outside of the document)? Or was this change made to become more spec compliant? On one hand: If the change was strictly for compliance, then we should fight to keep more web sites working. On the other hand: If this was done with real performance/operational benefits (and citing the fact that Opera already does this) then we might have to swallow the bullet. Andrew, Tobie: Does the Prototype test suite not test for regressions to the DOM insertion code? Nothing was triggered by this change to selectNodeContents so that leads me to believe that it isn't in there (or, at least, our version of the suite isn't current).

What would be broken in our browser if we returned to the FF2 behaviour here? Or, in other words, if we hadn't made this behavioural change, would we want it for FF3 on its own merits? It looks to me that we chose this path in 2006 because it simplified a fix for a crash case, and believed that Opera's behaviour here would be sufficient to indicate compatibility. Smaug expressed concerns back then about regressions, and I think they've come to pass: Prototype is quite widely used (though it might not have been in 2006, and their lack of support for Opera has protected the code in question from needing to cope with a strict interpretation of the standard until they started testing against FF3b2) and we shouldn't break those sites lightly. It's unfortunate that the Prototype test suite in our ajax tests didn't pick this up, since it's apparently a critical issue for their toolkit; we should make sure that we're running the test suite correctly in our harness, since this is basically exactly the sort of case that our ajax test collection is meant to catch. :/

(In reply to comment #16) > What would be broken in our browser if we returned to the FF2 behaviour here? Nothing > Or, in other words, if we hadn't made this behavioural change, would we want it > for FF3 on its own merits? It wouldn't be a blocker, I guess, but a wanted-1.9. Similar to firing capturing event listeners @target - doing something which the spec doesn't allow but changing implementation to follow the spec possibly breaks some sites. > It looks to me that we chose this path in 2006 > because it simplified a fix for a crash case, and believed that Opera's > behaviour here would be sufficient to indicate compatibility. Right. What is surprising that the this bug was filed 1½ years after the change. Maybe there aren't so many alpha users or maybe this feature isn't used too often in Prototype.

Jonas, since you've made some quite big clean-ups to nsRange code, what do you think about making it support the old (un-specified/un-defined) behavior? Would it be lots of work? Any guesses?

This issue showed up in our unit tests for version 1.6 of Prototype; before then we didn't write any code around element creation, so we had no test coverage thereof. The Ajax tests were added before 1.6 was released. So it sounds like the problem is keeping the test suite up to date. I'll talk to John Resig soon about how we can make this as easy as possible for you guys. We test against Opera, albeit unofficially; the relevant test passes in Opera because we circumvent the DOM range approach altogether, instead relying on Opera's support for the non-standard IE API. (In reply to comment #17) > Maybe there aren't so many alpha users or maybe this feature isn't used > too often in Prototype. We've since determined that this won't break all versions of Prototype — only version 1.6. That's still a substantial portion of the Web, and this is a very common use case.

Btw, would be great to know what is the real use case for range.selectNodeContents(element) where element isn't attached to DOM tree. Or any other similar case where element isn't in DOM tree. Not that it matters much to whether this bug should be fixed or not, but just to understand why it is used in such way.

I wouldn't be entirely opposed to reverting this change no. I do recall being nervous about it when the change was originally made. My memory regarding all of this is quite hazy though, as it all happened quite a long time ago. However we should all be aware that if we revert this we would be choosing website compatibility over spec compatibility. I have no faith at all that we can get the spec changed. So there is no "fight to keep more web sites working", we'd simply be giving up on following the spec. We've had this argument with W3C multiple times, and their stance is basically that the DOM is used for more things than the web so we can't change the spec since we'd be making other things out there non-compliant. There is one problem with reverting the fix. There is not really any logical behavior to what happens to a range that lives inside an orphaned node, when that node is inserted in a Document. A range is always "rooted" such that if an endpoint lives in a node, and the node is removed from the tree, the endpoint is moved up closer to the root of that tree. In the spec the root of such a tree is always a node that can't be inserted elsewhere. I.e. Documents, DocFragments and Attrs can't ever have their parent set to be another node. So what should happen if an orphaned element that contains a range is inserted into a document and then removed from the document? What should the range span? I'd argue that we should "reroot" the range when the element is inserted into the document. This isn't ideal, but neither is any other solution.

There is a growing feeling that we should have a "Web DOM Core" spec, we could similarly have a "Web DOM Range" spec. Right now, while the spec does indeed require that ranges be rooted in documents or documentFragments (or attrs), it doesn't actually say what should happen if you selectNode() something that isn't in one of those. The spec needs to be fixed either way.

This should really be reverted until a final decision is made, as it causes web content to break (anything that uses Prototype DOM Insertion, for example).

Ok, so I just talked with Tobie some more and, apparently, this will only effect Prototype 1.6.0/1.6.0.1. If this fix is easy, then I think we should go for it, to avoid any potential problems.

Another web compatibility change that was apparently done primarily to match a spec, but breaks existing code. I vote for reverting this change. Smaug, any chance you could have a look a this?

I guess I need to look at this, though there is the problem Jonas described: (In reply to comment #21) > There is one problem with reverting the fix. There is not really any logical > behavior to what happens to a range that lives inside an orphaned node, when > that node is inserted in a Document.

(In reply to comment #20) > Btw, would be great to know what is the real use case for > range.selectNodeContents(element) where element isn't attached to DOM tree. > Or any other similar case where element isn't in DOM tree. Could any Prototype developer answer to this? I'd like to know what kind of behavior is expected and how the not-in-dom range is used. The problem is that in FF3 some range bugs have been fixed and if someone is relying on the old, wrong, behavior (well, like this one), it may be hard to inject back those bugs without breaking the fixed things.

(In reply to comment #27) > Could any Prototype developer answer to this? They told me, at least, that this was only an issue with the 1.6.0 release and was fixed in subsequent versions. As I mentioned above, it would be ideal if this could be resolved, if there were no ramifications, but that does not appear to be the case. They were fine with having it slip by if it was going to be a problem.

Smaug: To make a long story short: range.selectNodeContents was used to insert HTML before/above/below/after an element in Prototype 1.5. In 1.6, we refactored that old code into a new API which handles both inserting content as a HTML string or a DOM node. People used it to populate newly created elements with pure text (instead of creating a text node and appending it - probably because the syntax is shorter). We've fixed that issue in 1.6.0.2 (changeset is here: http://dev.rubyonrails.org/changeset/8537/spinoffs/prototype/trunk/src/dom.js) , which is a drop-in replacement for 1.6.0, so I think that's no longer an issue. BTW, sorry if I initially overreacted, I thought the modifications in the insertion API were pre 1.6, so I was worried that a lot of users might be affected by it. Turns out it wasn't the case... and that inserting HTML *before* a detached element doesn't make much sense indeed. I hope this clarifies the issue. Thanks again for your help.

They're cool, we're cool. Removing the block and closing the bug.

Well, are we sure this issue isn't affecting more things?

I'm still trying to fix this, or as much as is possibly without regressing other things.

FWIW, I think the old code mostly ended up behaving as if you did "reroot" a range to the new top node if the range was part of an orphaned tree that was inserted into another tree.

Tried to keep changes pretty simple. The change to nsContentUtils::ComparePoints isn't perhaps the nicest one, but that way the behavior in other cases doesn't change. And I *really* want to try to avoid all regressions atm. Jonas, comments?

Comment on attachment 304524 [details] [diff] [review] something like this >+++ content/base/src/nsRange.cpp 20 Feb 2008 17:36:35 -0000 >+ if (disconnected) { >+ return NS_ERROR_FAILURE; >+ } Can we come up with a better error code than this? NS_ERROR_FAILURE is very generic. >- if (!nsContentUtils::ContentIsDescendantOf(parent, mRoot)) { >+ if (!parent->HasSameOwnerDoc(mRoot)) { I wonder about this change. In particular, a document fragment from a document, compared against the same document. (Then again, reading the bug summary, this might be the point.) >+ return disconnected ? NS_ERROR_FAILURE : NS_OK; Again, generic error code. >@@ -1265,23 +1291,27 @@ nsRange::CompareBoundaryPoints(PRUint16 >+ if (!mRoot->HasSameOwnerDoc(otherRoot)) Again, wondering about document fragments or detached nodes that may have the same owner document.

You're right. I'll change those error codes to NS_ERROR_DOM_WRONG_DOCUMENT_ERR. That is at least closest to what the spec says here: "WRONG_DOCUMENT_ERR: Raised if the two Ranges are not in the same Document or DocumentFragment." And so I can put back !nsContentUtils::ContentIsDescendantOfs. Will re-run tests and upload the new patch.

Comment on attachment 304540 [details] [diff] [review] better error code You shouldn't >@@ -1517,18 +1518,22 @@ nsContentUtils::ComparePoints(nsINode* a > do { > parents2.AppendElement(node2); > node2 = node2->GetNodeParent(); > } while (node2); > > PRUint32 pos1 = parents1.Length() - 1; > PRUint32 pos2 = parents2.Length() - 1; > >- NS_ASSERTION(parents1.ElementAt(pos1) == parents2.ElementAt(pos2), >- "disconnected nodes"); >+ if (aDisconnected) { >+ *aDisconnected = (parents1.ElementAt(pos1) != parents2.ElementAt(pos2)); >+ } else { >+ NS_ASSERTION(parents1.ElementAt(pos1) == parents2.ElementAt(pos2), >+ "disconnected nodes"); >+ } This'll make the non-debug code look weird. Do NS_ASSERTION(parents1.ElementAt(pos1) == parents2.ElementAt(pos2) || aDisconnected); if (aDisconnected) ... Set *aDisconnected to PR_FALSE at the top to handle the early-returns. >Index: content/base/src/nsRange.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/base/src/nsRange.cpp,v >retrieving revision 1.226 >diff -u -8 -p -r1.226 nsRange.cpp >--- content/base/src/nsRange.cpp 10 Feb 2008 13:04:19 -0000 1.226 >+++ content/base/src/nsRange.cpp 20 Feb 2008 19:11:57 -0000 >@@ -98,16 +98,17 @@ nsRange::CompareNodeToRange(nsIContent* > return CompareNodeToRange(aNode, range, outNodeBefore, outNodeAfter); > } > > // static > nsresult > nsRange::CompareNodeToRange(nsIContent* aNode, nsIRange* aRange, > PRBool *outNodeBefore, PRBool *outNodeAfter) > { >+ NS_ENSURE_STATE(aNode); > // create a pair of dom points that expresses location of node: > // NODE(start), NODE(end) > // Let incoming range be: > // {RANGE(start), RANGE(end)} > // if (RANGE(start) <= NODE(start)) and (RANGE(end) => NODE(end)) > // then the Node is contained (completely) by the Range. > > nsresult rv; >@@ -137,24 +138,33 @@ nsRange::CompareNodeToRange(nsIContent* > } > > nsINode* rangeStartParent = range->GetStartParent(); > nsINode* rangeEndParent = range->GetEndParent(); > PRInt32 rangeStartOffset = range->StartOffset(); > PRInt32 rangeEndOffset = range->EndOffset(); > > // is RANGE(start) <= NODE(start) ? >+ PRBool disconnected = PR_FALSE; > *outNodeBefore = nsContentUtils::ComparePoints(rangeStartParent, > rangeStartOffset, >- parent, nodeStart) > 0; >+ parent, nodeStart, >+ &disconnected) > 0; >+ if (disconnected) { >+ return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; >+ } >+ > // is RANGE(end) >= NODE(end) ? > *outNodeAfter = nsContentUtils::ComparePoints(rangeEndParent, > rangeEndOffset, >- parent, nodeEnd) < 0; >- >+ parent, nodeEnd, >+ &disconnected) < 0; >+ if (disconnected) { >+ return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; >+ } > return NS_OK; > } > > /****************************************************** > * non members > ******************************************************/ > > nsresult >@@ -330,16 +340,29 @@ nsRange::NodeWillBeDestroyed(const nsINo > NS_ASSERTION(mIsPositioned, "shouldn't be notified if not positioned"); > > // No need to detach, but reset positions so that the endpoints don't > // end up disconnected from each other. > // An alternative solution would be to make mRoot a strong pointer. > DoSetRange(nsnull, 0, nsnull, 0, nsnull); > } > >+void >+nsRange::ParentChainChanged(nsIContent *aContent) >+{ >+ if (mRoot == aContent) { >+ nsINode* newRoot = mRoot; >+ nsINode* tmp; >+ while ((tmp = newRoot->GetNodeParent())) { >+ newRoot = tmp; >+ } >+ DoSetRange(mStartParent, mStartOffset, mEndParent, mEndOffset, newRoot); >+ } >+} >+ > /******************************************************** > * Utilities for comparing points: API from nsIDOMNSRange > ********************************************************/ > NS_IMETHODIMP > nsRange::IsPointInRange(nsIDOMNode* aParent, PRInt32 aOffset, PRBool* aResult) > { > PRInt16 compareResult = 0; > nsresult rv = ComparePoint(aParent, aOffset, &compareResult); >@@ -367,32 +390,36 @@ nsRange::ComparePoint(nsIDOMNode* aParen > return NS_ERROR_NOT_INITIALIZED; > > nsCOMPtr<nsINode> parent = do_QueryInterface(aParent); > NS_ENSURE_TRUE(parent, NS_ERROR_DOM_HIERARCHY_REQUEST_ERR); > > if (!nsContentUtils::ContentIsDescendantOf(parent, mRoot)) { > return NS_ERROR_DOM_WRONG_DOCUMENT_ERR; > } >- >+ >+ PRBool disconnected = PR_FALSE; > PRInt32 cmp; > if ((cmp = nsContentUtils::ComparePoints(parent, aOffset, >- mStartParent, mStartOffset)) <= 0) { >+ mStartParent, mStartOffset, >+ &disconnected)) <= 0) { Don't you need to add |&& !disconnected| here? >- else if (nsContentUtils::ComparePoints(mEndParent, mEndOffset, >- parent, aOffset) == -1) { >+ else if (!disconnected && >+ nsContentUtils::ComparePoints(mEndParent, mEndOffset, >+ parent, aOffset, >+ &disconnected) == -1) { And technically you need to check for connected-ness here. It should be the same as from the first ComparePoints call. So leave it as null to enable the assertion. >@@ -592,46 +621,41 @@ nsINode* nsRange::IsValidBoundary(nsINod > NS_ASSERTION(!root->IsNodeOfType(nsINode::eDOCUMENT), > "GetCurrentDoc should have returned a doc"); > > if (root->IsNodeOfType(nsINode::eDOCUMENT_FRAGMENT) || > root->IsNodeOfType(nsINode::eATTRIBUTE)) { > return root; > } > >-#ifdef DEBUG_smaug >- nsCOMPtr<nsIContent> cont = do_QueryInterface(root); >- if (cont) { >- nsAutoString name; >- cont->Tag()->ToString(name); >- printf("nsRange::IsValidBoundary: node is not a valid boundary point [%s]\n", >- NS_ConvertUTF16toUTF8(name).get()); >- } >-#endif >- >- return nsnull; >+ NS_WARNING("Creating a DOM Range using root which isn't in DOM!"); >+ // But we allow this because of backward compatibility. >+ return root; No need to check for docfragment and attribute as only debug-only code differs. If you really do want to warn do something like #ifdef DEBUG if (DOC_FRAGMENT || ATTR) { NS_WARNING } #endif Though I sort of think that if we're going to allow ranges like this we should embrace it fully and not even warn. > nsresult nsRange::SetStart(nsIDOMNode* aParent, PRInt32 aOffset) > { > VALIDATE_ACCESS(aParent); > > nsCOMPtr<nsINode> parent = do_QueryInterface(aParent); > nsINode* newRoot = IsValidBoundary(parent); > NS_ENSURE_TRUE(newRoot, NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR); > > PRInt32 len = GetNodeLength(parent); > if (aOffset < 0 || aOffset > len) > return NS_ERROR_DOM_INDEX_SIZE_ERR; > > // Collapse if not positioned yet, if positioned in another doc or > // if the new start is after end. >+ PRBool disconnected = PR_FALSE; > if (!mIsPositioned || newRoot != mRoot || > nsContentUtils::ComparePoints(parent, aOffset, >- mEndParent, mEndOffset) == 1) { >+ mEndParent, mEndOffset, >+ &disconnected) == 1 || >+ disconnected) { The nodes can't be disconnected here can they, since newRoot == mRoot. So leave as is to get the assertion. Same in SetEnd and CompareBoundaryPoints. Actually, come to think of it. Why do you need to add the disconnected check anywhere? There were disconnected subtrees before too, just always rooted in docfragments. So the old code should have had all necessary checks, no? IsPointInRange checks nsContentUtils::ContentIsDescendantOf so that should be enough there. And the CompareNodeToRange callers should already be checking for disconnectedness.

Ok, disconnectivity check isn't needed when there is root check, but I think it is better to have the test in nsRange::CompareNodeToRange.

Comment on attachment 305308 [details] [diff] [review] v2 >+nsRange::ParentChainChanged(nsIContent *aContent) >+{ >+ if (mRoot == aContent) { Just assert that the above is true. The only node we observe is mRoot so if we get notified for any other something is very wrong. r/sr=me with that