Summary: | REGRESSION (r73818): range.cloneContents() ignores end offset | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | HTML Editing | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, adele, aroben, darin, inferno, jberlin, mitz, rniwa | ||||
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | OS X 10.5 | ||||||
Bug Depends on: | 50854 | ||||||
Bug Blocks: | 52524 | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2011-01-15 11:27:42 PST
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(); [...] } [...] } 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!
Committed r75882: <http://trac.webkit.org/changeset/75882> (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 (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. (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. (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> (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> *** Bug 54861 has been marked as a duplicate of this bug. *** |