Bug 52512 – REGRESSION (r73818): range.cloneContents() ignores end offset

• Component: HTML Editing
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Created attachment 79068 [details]
Patch

Reviewed by NOBODY (OOPS!).

WebCore:

The fix for Bug 50710 in r73799 introduced an off-by-one error
when copying nodes to a local NodeVector for processing.  A fix
was attempted for Bug 50854 in r73818, but instead of stopping
at the end offset, it iterates through all the sibling nodes
because the loop variable (i) is never incremented.  To clean
this up, revert back to the code in r73799 and fix the
off-by-one error.

Test: fast/dom/Range/range-clone-contents.html

* dom/Range.cpp:
(WebCore::Range::processContents): Fix the loop that copies
nodes to a local NodeVector by restoring the code from r73799
and fixing the off-by-one error.

LayoutTests:

* fast/dom/Range/range-clone-contents-expected.txt: Added.
* fast/dom/Range/range-clone-contents.html: Added.
---
 5 files changed, 124 insertions(+), 6 deletions(-)

[David Kilzer (:ddkilzer)]

Note to reviewers:  Please verify the new algorithm matches the behavior of the original code.  I've gone through the new algorithm, but this code has had bugs introduced through two rewrites and two reviews, so please study it carefully before giving an r+.

This is the pre-r73799 code in processContents() in WebCore/dom/Range.cpp:

            Node* n = m_end.container()->firstChild();
            if (n && m_end.offset()) {
                for (int i = 0; i + 1 < m_end.offset(); i++) { // skip to end.offset() 
                    Node* next = n->nextSibling(); 
                    if (!next) 
                        break; 
                    n = next; 
                } 
                Node* prev; 
                for (; n; n = prev) { 
                    prev = n->previousSibling(); 
                    [...]
                } 
                [...]
            }

The fix is this code:

            Node* n = m_end.container()->firstChild();
            if (n && m_end.offset()) {
                NodeVector nodes;
                for (int i = 0; i < m_end.offset() && n; i++, n = n->nextSibling())
                    nodes.append(n);
                for (int i = nodes.size() - 1; i >= 0; i--) {
                    n = nodes[i].get();
                    [...]
                } 
                [...]
            }

[David Kilzer (:ddkilzer)]

<rdar://problem/8789045>

[Adele Peterson]

Comment on attachment 79068 [details]
Patch

I'm not sure I understand the intermediate change before this very well, but this code looks right.  You're still going to always append at least the end container's first child, since m_end.offset() won't be 0 in the inner loop.

The new test looks good too, and Dave says this also passes the other editing tests, including the one that was failing for one of the earlier patches.

r=me!

[David Kilzer (:ddkilzer)]

Committed r75882: <http://trac.webkit.org/changeset/75882>

[Jessie Berlin]

(In reply to comment #4)
> Committed r75882: <http://trac.webkit.org/changeset/75882>

fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots:

http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (In reply to comment #4)
> > Committed r75882: <http://trac.webkit.org/changeset/75882>
> 
> fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots:
> 
> http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html

This is the exact erroneous behavior that the test is trying to fix (where the <img> tag is left in the range instead of being removed).  It's as if Range.cpp wasn't compiled on the Windows 7 bot.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (In reply to comment #4)
> > Committed r75882: <http://trac.webkit.org/changeset/75882>
> 
> fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots:
> 
> http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r75882%20(8257)/fast/dom/Range/range-clone-contents-pretty-diff.html

Same issue on the Windows XP box:

http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r75882%20(24216)/fast/dom/Range/range-clone-contents-pretty-diff.html

Oddly enough, I don't see any other bots failing on this test.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Committed r75882: <http://trac.webkit.org/changeset/75882>
> > 
> > fast/dom/Range/range-clone-contents.html appears to be failing on the Windows 7 Release Test bots:
> > [...]
> Same issue on the Windows XP box:
> [...]
> Oddly enough, I don't see any other bots failing on this test.

Filed Bug 52524 to cover this.  Added the test to the Windows skip list:

Committed r75889.  <http://trac.webkit.org/changeset/75889>

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Filed Bug 52524 to cover this.  Added the test to the Windows skip list:
> 
> Committed r75889.  <http://trac.webkit.org/changeset/75889>

Turns out the buildbots just needed to do a clean build.  Removed the test from the skip list:

Committed r75970: <http://trac.webkit.org/changeset/75970>

[Ryosuke Niwa]

*** Bug 54861 has been marked as a duplicate of this bug. ***