433662 - make range.insertNode insert the node into the range even when the range is collapsed

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

ACID3 test7 relies on DOM Range insertNode to move the end boundary point
also when used with collapsed range. This contradicts with DOM Range 2.12.1 [1], which says that
"Note that when content is inserted at a boundary-point, it is ambiguous as to where the boundary-point should be repositioned if its relative position is to be maintained. There are two possibilities: at the start or at the end of the newly inserted content. We have chosen that in this case neither the container nor offset of the boundary-point is changed. As a result, the boundary-point will be positioned at the start of the newly inserted content."

2.9 says that "insertNode() method inserts the specified node into the Range's context tree." Context tree isn't the same thing as range, but 
"The tree rooted by the root container is known as the Range's context tree."
And in the ACID3 test7 the root container is #document, so inserting
#comment to #document without modifying range is valid behavior.

[1] http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/ranges.html#Level-2-Range-Insertions

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

FYI, this has been discussed on WebAPI mailing list. Just look at
messages with insertNode in the subject.
http://lists.w3.org/Archives/Public/public-webapi/2008May/
A change was proposed to the DOM 2 Range spec, but IMHO, that doesn't really
make sense - changing the spec so that a testcase becomes valid.

FF2, FF3, Opera9.2x and Opera9.5b have the behavior, which DOM 2 Range defines
(although the wording in the spec could be better for sure).
Safari3.1 doesn't have that behavior nor does "ACID3'ed-Opera".

I've proposed that ACID3 should be fixed, but so far no success in that.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

#developers
Hixie>	i've added a hack in acid3

So ACID3 was changed and this can be closed.

Comment 3

[Hixie (not reading bugmail)]

This is still a bug. It's just not blocking acid3 anymore.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

This can be reopened if range spec is changed ;)

Comment 5

[Martijn Wargers (dead)]

This bug was about Mozilla failing this particular ACID3 test. Since the ACID3
test was changed, Mozilla is succeeding now, right?
In that case, this bug should be marked FIXED.

Comment 6

[Hixie (not reading bugmail)]

(In reply to comment #4)
> This can be reopened if range spec is changed ;)

The test was changed on the understanding that this bug would be fixed. If you still don't agree it's a bug, then I should change Acid3 back -- the whole point was that I didn't want to change the test because then people would just ignore the issue, which is exactly what you're now doing.

Comment 7

[Martijn Wargers (dead)]

This bug is fixed, because Mozilla doesn't fail on this particular ACID3 test anymore.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #6)
> (In reply to comment #4)
> > This can be reopened if range spec is changed ;)
> 
> The test was changed on the understanding that this bug would be fixed. If you
> still don't agree it's a bug, then I should change Acid3 back -- the whole
> point was that I didn't want to change the test because then people would just
> ignore the issue, which is exactly what you're now doing.

It is a bug in Range spec or (was) in ACID3.
Well, ok this can be open until range spec is fixed and implementation changed to follow the new errata.

Comment 9

[Martijn Wargers (dead)]

So why change ACID3 then?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Because we do what the spec says currently, but one may argue that
the spec can be interpreted in a different way.
So the test must handle both interpretations.

Comment 11

[Hixie (not reading bugmail)]

I changed Acid3 as an act of good faith. (I disagree that Smaug's interpretation is reasonable.)

Comment 12

[Martijn Wargers (dead)]

Well, fwiw, I don't think you should change Acid3 if you think it's a bug.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

And I (and some others) disagree with Hixie's interpretation - so the 
spec should be clarified for sure.
But thank you Hixie for handling both cases in ACID3.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

As of now this is invalid.
If the spec changes, this may need to be reopened.

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The current spec (DOM4) is clear:

"""
7. Let new offset be the index of reference node, or parent's length if reference node is null.

8. Add node's length to new offset, if node is a DocumentFragment. Otherwise add one to new offset.

9. Pre-insert node into parent before reference node.

10. If start and end are the same, set end to (parent, new offset).
"""
http://dvcs.w3.org/hg/domcore/raw-file/tip/dom-core.html#dom-range-insertnode

Test-case:

data:text/html,<!DOCTYPE html>
<script>
var range = document.createRange();
range.setStart(document.head, 0);
range.insertNode(document.createComment("abc"));
document.documentElement.textContent = range.endOffset;
</script>

Firefox outputs 0, against the spec.  IE10 Developer Preview, Chrome 19 dev, and Opera Next 12.00 alpha all output 1, following the spec.  This causes Firefox to fail official W3C tests at <http://w3c-test.org/webapps/DOMCore/tests/approved/Range-insertNode.html>.  I'm willing to try writing a patch if you'll accept it.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v1 -- Clean up nsRange::InsertNode

The first patch just cleans up InsertNode.  There was a bunch of unnecessary code here, like code to set the end offset properly if the text node was split (no longer needed due to bug 191864), and code that fetched tChildListLength but then never used it.  I also unified the text/non-text paths so there's only one InsertBefore, because the next patch will add extra code that has to be used by both paths and it shouldn't be copy-pasted.

This shouldn't change the effect of anything -- the score on the Range-insertNode.html test suite is the same.  It reduces LOC (20 insertions, 36 deletions).

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v1 -- Make Range.insertNode insert the node into the range even when the range is collapsed

This fixes the actual bug.  With this patch, together with bug 737612 (already landed on -inbound), Gecko passes all the Range-insertNode.html tests.

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=9cda0a2a75ad

Comment 19

[Mozilla RelEng Bot]

Try run for f07188fdfcf5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f07188fdfcf5
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f07188fdfcf5

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(ignore that, it was a bad try push that I canceled)

Comment 21

[Mozilla RelEng Bot]

Try run for f1fa94fa8421 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f1fa94fa8421
Results (out of 86 total builds):
    exception: 27
    success: 16
    warnings: 6
    failure: 37
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f1fa94fa8421

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(ignore that one too, I gave it the wrong --post-to-bugzilla parameter; still waiting for the actual try push to finish before I upload a new patch)

Comment 23

[Mozilla RelEng Bot]

Try run for 9cda0a2a75ad is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9cda0a2a75ad
Results (out of 225 total builds):
    exception: 1
    success: 171
    warnings: 38
    failure: 14
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9cda0a2a75ad
 Timed out after 12 hours without completing.

Comment 24

[Mozilla RelEng Bot]

Try run for 69721f440558 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=69721f440558
Results (out of 218 total builds):
    exception: 2
    success: 164
    warnings: 37
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-69721f440558
 Timed out after 12 hours without completing.

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v2 -- Make Range.insertNode insert the node into the range even when the range is collapsed

This fixes the test failures from the try run.  I'll do another try run to be sure, but it should be safe to review now.

Comment 26

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 608008, 608344
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=4fffd1e1fd5f
Try run started, revision 4fffd1e1fd5f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=4fffd1e1fd5f

Comment 27

[Mozilla RelEng Bot]

Try run for 4fffd1e1fd5f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4fffd1e1fd5f
Results (out of 255 total builds):
    success: 196
    warnings: 43
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4fffd1e1fd5f

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 608008 [details] [diff] [review]
Patch part 1, v1 -- Clean up nsRange::InsertNode


> NS_IMETHODIMP
>-nsRange::InsertNode(nsIDOMNode* aN)
>+nsRange::InsertNode(nsIDOMNode* aNode)
> {
>-  VALIDATE_ACCESS(aN);
>+  VALIDATE_ACCESS(aNode);
>   
>   nsresult res;
>   PRInt32 tStartOffset;
>   this->GetStartOffset(&tStartOffset);
> 
>   nsCOMPtr<nsIDOMNode> tStartContainer;
>   res = this->GetStartContainer(getter_AddRefs(tStartContainer));
>   if(NS_FAILED(res)) return res;
> 
>+  // This is the node we'll be inserting before, and its parent
>+  nsCOMPtr<nsIDOMNode>referenceNode;
>+  nsCOMPtr<nsIDOMNode>referenceParentNode = tStartContainer;
space after >


>+    referenceNode = secondPart;
>+  } else {
>+    nsCOMPtr<nsIDOMNodeList>tChildList;
>+    res = tStartContainer->GetChildNodes(getter_AddRefs(tChildList));
>+    if(NS_FAILED(res)) return res;
if (expr) {
  stmt;
}
Or NS_ENSURE_SUCCESS(res, res);
same also elsewhere.
(and I know, the old code had bad style)

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 608344 [details] [diff] [review]
Patch part 2, v2 -- Make Range.insertNode insert the node into the range even when the range is collapsed

>+  // We might need to update the end to include the new node (bug 433662)
>+  PRInt32 newOffset;
>+
>   nsCOMPtr<nsIDOMText> startTextNode(do_QueryInterface(tStartContainer));
>   if (startTextNode) {
>     res = tStartContainer->GetParentNode(getter_AddRefs(referenceParentNode));
>     if(NS_FAILED(res)) return res;
>     NS_ENSURE_TRUE(referenceParentNode, NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
> 
>     nsCOMPtr<nsIDOMText> secondPart;
>     res = startTextNode->SplitText(tStartOffset, getter_AddRefs(secondPart));
>     if (NS_FAILED(res)) return res;
> 
>     referenceNode = secondPart;
>+    newOffset = IndexOf(referenceNode);
>   } else {
>     nsCOMPtr<nsIDOMNodeList>tChildList;
>     res = tStartContainer->GetChildNodes(getter_AddRefs(tChildList));
>     if(NS_FAILED(res)) return res;
> 
>     // find the insertion point in the DOM and insert the Node
>     res = tChildList->Item(tStartOffset, getter_AddRefs(referenceNode));
>     if(NS_FAILED(res)) return res;
>+
>+    if (referenceNode) {
>+      newOffset = IndexOf(referenceNode);
>+    } else {
>+      res = tChildList->GetLength((PRUint32*)&newOffset);
This looks ugly. Use a separate variable and assign to newOffset or something.


>+
>   nsCOMPtr<nsIDOMNode> tResultNode;
>-  return referenceParentNode->InsertBefore(aNode, referenceNode, getter_AddRefs(tResultNode));
>+  res = referenceParentNode->InsertBefore(aNode, referenceNode, getter_AddRefs(tResultNode));
>+  if (NS_FAILED(res)) return res;
>+
>+  if (Collapsed()) {
>+    return SetEnd(referenceParentNode, newOffset);
>+  }
>+  return NS_OK;
We should count newOffset only if Collapsed(). IndexOf() isn't superfast.

Comment 30

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v2 -- Clean up nsRange::InsertNode

Comment 31

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v3 -- Make Range.insertNode insert the node into the range even when the range is collapsed

Comment 32

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 611264, 611265
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=3e9043417b7f
Try run started, revision 3e9043417b7f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=3e9043417b7f

Comment 33

[Mozilla RelEng Bot]

Try run for 3e9043417b7f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3e9043417b7f
Results (out of 219 total builds):
    exception: 2
    success: 186
    warnings: 31
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3e9043417b7f

Comment 34

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The tryserver results seem to have vanished.  I'm pretty sure they were fine, so I'll risk pushing to inbound anyway.

https://hg.mozilla.org/integration/mozilla-inbound/rev/244c18f9b577
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf55cbcd2fb

Comment 35

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 611264, 611265
	Branch: mozilla-central => try
Patch 611265 could not be applied to mozilla-central.
patching file content/base/test/Makefile.in
Hunk #1 FAILED at 573
1 out of 1 hunks FAILED -- saving rejects to file content/base/test/Makefile.in.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/244c18f9b577
https://hg.mozilla.org/mozilla-central/rev/fcf55cbcd2fb

Comment 37

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/en/DOM/range.insertNode

Mentioned on Firefox 14 for developers.