Bug 16765 – Range does not adjust endContainer and endOffset correctly after insertNode (Acid3 bug)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andrew Wellington]

Acid3 test 15 failure:
Test 15: FAIL (range didn't start with the expected text; range stringified to '')

      // test 15: Ranges under mutations: insertion into text nodes
      var doc = getTestDocument();
      var p = doc.createElement('p');
      var t1 = doc.createTextNode('12345');
      p.appendChild(t1);
      var t2 = doc.createTextNode('ABCDE');
      p.appendChild(t2);
      doc.body.appendChild(p);
      var r = doc.createRange();
      r.setStart(p.firstChild, 2);
      r.setEnd(p.firstChild, 3);
      assert(!r.collapsed, "collapsed is wrong at start");
      assertEquals(r.commonAncestorContainer, p.firstChild, "commonAncestorContainer is wrong at start");
      assertEquals(r.startContainer, p.firstChild, "startContainer is wrong at start");
      assertEquals(r.startOffset, 2, "startOffset is wrong at start");
      assertEquals(r.endContainer, p.firstChild, "endContainer is wrong at start");
      assertEquals(r.endOffset, 3, "endOffset is wrong at start");
      assertEquals(r.toString(), "3", "range in text node stringification failed");
      r.insertNode(p.lastChild);
      assertEquals(p.childNodes.length, 3, "insertion of node made wrong number of child nodes");
      assertEquals(p.childNodes[0], t1, "unexpected first text node");
      assertEquals(p.childNodes[0].data, "12", "unexpected first text node contents");
      assertEquals(p.childNodes[1], t2, "unexpected second text node");
      assertEquals(p.childNodes[1].data, "ABCDE", "unexpected second text node");
      assertEquals(p.childNodes[2].data, "345", "unexpected third text node contents");
      // The spec is very vague about what exactly should be in the range afterwards:
      // the insertion results in a splitText(), which it says is equivalent to a truncation
      // followed by an insertion, but it doesn't say what to do when you have a truncation,
      // so we don't know where either the start or the end boundary points end up.
      // The spec really should be clarified for how to handle splitText() and
      // text node truncation in general
      // The only thing that seems very clear is that the inserted text node should
      // be in the range, and it has to be at the start, since insertion always puts it at
      // the start.
      assert(!r.collapsed, "collapsed is wrong after insertion");
      //assertEquals(r.commonAncestorContainer, p, "commonAncestorContainer is wrong after insertion");
      //assertEquals(r.startContainer, p, "startContainer is wrong after insertion");
      //assertEquals(r.startOffset, 1, "startOffset is wrong after insertion");
      //assertEquals(r.endContainer, p.lastChild, "endContainer is wrong after insertion");
      //assertEquals(r.endOffset, 1, "endOffset is wrong after insertion");
      assert(r.toString().match(/^ABCDE/), "range didn't start with the expected text; range stringified to '" + r.toString() + "'");
      return 1;

[Eric Seidel (no email)]

Ok, I understand what's going on here, and I plan to fix it.  I need to make a few test cases first, just to make sure.

Hixie is right, this is way under-specified, but I think Firefox's behavior is correct here:

<p>12345</p>

Range:
startContainer: Text<12345>
startOffset: 2
endContainer: Text<12345>
endOffset: 3

range.toString() -> "3" // we get this part right.

range.insertNode(Text<ABCDE>)

FireFox does:
startContainer: Text<12>
startOffset: 2
endContainer: Text<345>
endOffset: 1

Safari does:
startContainer: Text<12>
startOffset: 2
endContainer: Text<12>
endOffset: 4

Again, the spec is not at all clear here, but I think FF's behavior makes more sense.  Especially since our endOffset is off the end of the endContainer.

[Eric Seidel (no email)]

Created attachment 18419 [details]
Fix Range.insertNode for the textSplit case

 LayoutTests/ChangeLog                              |   16 ++++++++
 ...e-insertNode-separate-endContainer-expected.txt |   22 +++++++++++
 .../range-insertNode-separate-endContainer.html    |   13 +++++++
 .../Range/range-insertNode-splittext-expected.txt  |   22 +++++++++++
 .../fast/dom/Range/range-insertNode-splittext.html |   13 +++++++
 .../range-insertNode-separate-endContainer.js      |   38 ++++++++++++++++++++
 .../Range/resources/range-insertNode-splittext.js  |   33 +++++++++++++++++
 WebCore/ChangeLog                                  |   14 +++++++
 WebCore/dom/Range.cpp                              |   36 +++++++++++-------
 9 files changed, 193 insertions(+), 14 deletions(-)

[Eric Seidel (no email)]

Created attachment 18420 [details]
Fix Range.insertNode for the textSplit case

 LayoutTests/ChangeLog                              |   16 ++++++++
 ...e-insertNode-separate-endContainer-expected.txt |   22 +++++++++++
 .../range-insertNode-separate-endContainer.html    |   13 +++++++
 .../Range/range-insertNode-splittext-expected.txt  |   22 +++++++++++
 .../fast/dom/Range/range-insertNode-splittext.html |   13 +++++++
 .../range-insertNode-separate-endContainer.js      |   38 ++++++++++++++++++++
 .../Range/resources/range-insertNode-splittext.js  |   33 +++++++++++++++++
 WebCore/ChangeLog                                  |   14 +++++++
 WebCore/dom/Range.cpp                              |   36 +++++++++++-------
 9 files changed, 193 insertions(+), 14 deletions(-)

[Oliver Hunt]

Comment on attachment 18420 [details]
Fix Range.insertNode for the textSplit case

This looks fine to me, but i'd rather someone more familiar with this area (justin :D ) review it

[Eric Seidel (no email)]

This is the bug I figured Justin might be interested in seeing.

Will this work correctly when the inserted node is already in the range?

[Darin Adler]

Comment on attachment 18420 [details]
Fix Range.insertNode for the textSplit case

This is OK as far as it goes. But long term we need to adjust ranges for any changes to the DOM, not just changes performed by the range code itself. We need to register the ranges with the document and update them as the various elements and text nodes are mutated. So more patches like this one are not a step in the right direction.

Nonetheless, r=me

[Alexey Proskuryakov]

(See other bugs blocking bug 11997, some of which also include custom fixes that we will want to remove).

[Eric Seidel (no email)]

@darin: So now that I look closer (and realize that we don't support Range objects tracking DOM mutations), yes, this is "the wrong" fix.  The test case still remains useful.  I think I'll land it anyway, and then when we move Range off of this custom code, onto a more unified DOM mutation tracking infrastructure, this code will disappear (and the test case will stick around).

@ap: Yeah, this code will be removed in the end, but it won't require any special effort to do so. :)

[Eric Seidel (no email)]

(In reply to comment #6)
> Will this work correctly when the inserted node is already in the range?
> 

I made some additional test cases, and found at least one (existing) problem in our insertNode code.  FireFox and WebKit both have (differing) bad results for my test case.  I'll file a follow-up bug with that new test case shortly.

[Eric Seidel (no email)]

r29456