Summary: | REGRESSION: Increasing indent deletes text. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Thomas <michaelthomas> | ||||||||||
Component: | HTML Editing | Assignee: | Enrica Casucci <enrica> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, enrica, eric, jparent, rniwa, sullivan, webkit.review.bot | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Michael Thomas
2009-12-07 11:38:37 PST
Found in latest version of Chrome on LInux. Reproduces in WebKit nightly. Does not reproduce in Sarari 4.0.4 (5531.21.10). I'm working on a fix. Created attachment 44445 [details]
Patch
Attachment 44445 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/CompositeEditCommand.cpp:774: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/editing/CompositeEditCommand.cpp:774: Use 0 instead of NULL. [readability/null] [5]
Total errors found: 2
Created attachment 44446 [details]
Patch2
Fixed to make review bot happy.
style-queue ran check-webkit-style on attachment 44446 [details] without any errors.
Comment on attachment 44446 [details] Patch2 > + for (Node* n = start.node()->traverseNextSibling(); n; n = n->nextSibling()) { What is the rationale for using traverseNextSibling here. Do we really want the parent's next sibling? This seems like it could easily walk entirely out of the part of the subtree we are trying to work on. > + lastNode = topNode.get()->firstChild(); No get() needed here. (In reply to comment #7) > (From update of attachment 44446 [details]) > > + for (Node* n = start.node()->traverseNextSibling(); n; n = n->nextSibling()) { > > What is the rationale for using traverseNextSibling here. Do we really want the > parent's next sibling? This seems like it could easily walk entirely out of the > part of the subtree we are trying to work on. This is exactly the bug I had. Imagine this markup DIV SPAN #text1 SPAN #text2 When trying to clone this paragraph, the old code was looking for the sibling of #text1 which is NULL and the following SPAN was lost. We had a test case that was covering various inlne combinations (execCommand/indent-paragraphs.html) but did not cover this one that I've now added with the patch. Let me know if I'm missing something here. > > > + lastNode = topNode.get()->firstChild(); > > No get() needed here. Fixed. Created attachment 44449 [details]
Patch3
style-queue ran check-webkit-style on attachment 44449 [details] without any errors.
(In reply to comment #8) > Let me know if I'm missing something here. Well, traverseNextSibling, if not passed an argument to limit how far it can iterate, can walk all the way out of a content editable area. Is there some guarantee that won't happen here? (In reply to comment #11) > (In reply to comment #8) > > Let me know if I'm missing something here. > > Well, traverseNextSibling, if not passed an argument to limit how far it can > iterate, can walk all the way out of a content editable area. Is there some > guarantee that won't happen here? Probably not. I can call it passing outerNode to make sure. I think that would work. Created attachment 44450 [details]
Patch4
Limiting traverseNextSibling to outerNode.
style-queue ran check-webkit-style on attachment 44450 [details] without any errors.
Comment on attachment 44450 [details] Patch4 > + if (start.node() != end.node() && !start.node()->isDescendantOf(end.node())) { Such things would be so much easier to read if we had the flavor of isDescendantOf that includes the node itself. r=me Committed revision 51819. |