The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: smaug, Assigned: ayg)

Tracking

({dev-doc-complete})

Trunk
mozilla14
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
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.
(Reporter)

Comment 2

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

So ACID3 was changed and this can be closed.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
This is still a bug. It's just not blocking acid3 anymore.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 4

9 years ago
This can be reopened if range spec is changed ;)
No longer blocks: 410460
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This bug is fixed, because Mozilla doesn't fail on this particular ACID3 test anymore.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 8

9 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Either fix ACID3, or DOM Range spec or make range.insertNode to work in a way which ACID3 assumes → make range.insertNode insert the node into the range even when the range is collapsed
So why change ACID3 then?
(Reporter)

Comment 10

9 years ago
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.
I changed Acid3 as an act of good faith. (I disagree that Smaug's interpretation is reasonable.)
Well, fwiw, I don't think you should change Acid3 if you think it's a bug.
(Reporter)

Comment 13

9 years ago
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.
(Reporter)

Comment 14

6 years ago
As of now this is invalid.
If the spec changes, this may need to be reopened.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago6 years ago
Resolution: --- → INVALID
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Created attachment 608008 [details] [diff] [review]
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).
Assignee: nobody → ayg
Status: REOPENED → ASSIGNED
Attachment #608008 - Flags: review?(bugs)
Created attachment 608010 [details] [diff] [review]
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.
Attachment #608010 - Flags: review?(bugs)
Try: https://tbpl.mozilla.org/?tree=Try&rev=9cda0a2a75ad

Comment 19

5 years ago
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/ayg@aryeh.name-f07188fdfcf5
(ignore that, it was a bad try push that I canceled)

Comment 21

5 years ago
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/ayg@aryeh.name-f1fa94fa8421
(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

5 years ago
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/ayg@aryeh.name-9cda0a2a75ad
 Timed out after 12 hours without completing.

Comment 24

5 years ago
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/ayg@aryeh.name-69721f440558
 Timed out after 12 hours without completing.
Created attachment 608344 [details] [diff] [review]
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.
Attachment #608010 - Attachment is obsolete: true
Attachment #608344 - Flags: review?(bugs)
Attachment #608010 - Flags: review?(bugs)
Whiteboard: [autoland-try:-u all]

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 26

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

5 years ago
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/autolanduser@mozilla.com-4fffd1e1fd5f

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Reporter)

Comment 28

5 years ago
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)
Attachment #608008 - Flags: review?(bugs) → review+
(Reporter)

Comment 29

5 years ago
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.
Attachment #608344 - Flags: review?(bugs) → review-
Created attachment 611264 [details] [diff] [review]
Patch part 1, v2 -- Clean up nsRange::InsertNode
Attachment #608008 - Attachment is obsolete: true
Attachment #611264 - Flags: review+
Created attachment 611265 [details] [diff] [review]
Patch part 2, v3 -- Make Range.insertNode insert the node into the range even when the range is collapsed
Attachment #608344 - Attachment is obsolete: true
Attachment #611265 - Flags: review?(bugs)
Whiteboard: [autoland-try:-u all]

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 32

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

5 years ago
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/autolanduser@mozilla.com-3e9043417b7f

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(Reporter)

Updated

5 years ago
Attachment #611265 - Flags: review?(bugs) → review+
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
Flags: in-testsuite+
Hardware: x86 → All
Whiteboard: [autoland-try:-u all]
Target Milestone: --- → mozilla14

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 35

5 years ago
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.

Updated

5 years ago
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/mozilla-central/rev/244c18f9b577
https://hg.mozilla.org/mozilla-central/rev/fcf55cbcd2fb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Keywords: dev-doc-needed
Resolution: --- → FIXED
Docs updated:

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

Mentioned on Firefox 14 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 years ago
Depends on: 765799
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.