REGRESSION: Increasing indent deletes text.

[Michael Thomas]

Steps to Reproduce:

1. Go to midas demo: http://www.mozilla.org/editor/midasdemo/
2. Enter the following HTML:

<ul><li><b>foo</b>bar</li></ul>

3. Place your cursor before 'foobar'.
4. Press the 'indent' button.

Actual Result:

Line is indent, unbold test ('bar') is deleted.

Expected Result:

No text is deleted.

[Michael Thomas]

Found in latest version of Chrome on LInux. Reproduces in WebKit nightly. Does not reproduce in Sarari 4.0.4 (5531.21.10).

[Enrica Casucci]

I'm working on a fix.

[Enrica Casucci]

Created attachment 44445 [details]
Patch

[WebKit Review Bot]

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

[Enrica Casucci]

Created attachment 44446 [details]
Patch2

Fixed to make review bot happy.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44446 [details] without any errors.

[Darin Adler]

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.

[Enrica Casucci]

(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.

[Enrica Casucci]

Created attachment 44449 [details]
Patch3

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44449 [details] without any errors.

[Darin Adler]

(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?

[Enrica Casucci]

(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.

[Enrica Casucci]

Created attachment 44450 [details]
Patch4

Limiting traverseNextSibling to outerNode.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44450 [details] without any errors.

[Darin Adler]

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

[Enrica Casucci]

Committed revision 51819.