Bug 16765 - Range does not adjust endContainer and endOffset correctly after insertNode (Acid3 bug)
Summary: Range does not adjust endContainer and endOffset correctly after insertNode (...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-06 17:49 PST by Andrew Wellington
Modified: 2008-02-10 12:36 PST (History)
4 users (show)

See Also:


Attachments
Fix Range.insertNode for the textSplit case (10.48 KB, patch)
2008-01-13 02:12 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix Range.insertNode for the textSplit case (10.48 KB, patch)
2008-01-13 02:14 PST, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wellington 2008-01-06 17:49:52 PST
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;
Comment 1 Eric Seidel (no email) 2008-01-12 19:51:02 PST
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.
Comment 2 Eric Seidel (no email) 2008-01-13 02:12:18 PST
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(-)
Comment 3 Eric Seidel (no email) 2008-01-13 02:14:37 PST
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(-)
Comment 4 Oliver Hunt 2008-01-13 02:21:12 PST
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
Comment 5 Eric Seidel (no email) 2008-01-13 02:25:04 PST
This is the bug I figured Justin might be interested in seeing.
Comment 6 mitz 2008-01-13 02:35:02 PST
Will this work correctly when the inserted node is already in the range?
Comment 7 Darin Adler 2008-01-13 08:14:25 PST
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
Comment 8 Alexey Proskuryakov 2008-01-13 11:15:07 PST
(See other bugs blocking bug 11997, some of which also include custom fixes that we will want to remove).
Comment 9 Eric Seidel (no email) 2008-01-13 16:53:41 PST
@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. :)
Comment 10 Eric Seidel (no email) 2008-01-13 17:34:05 PST
(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.
Comment 11 Eric Seidel (no email) 2008-01-13 17:36:04 PST
r29456