Bug 33226 – Indent inside li with br's changes order and creates new lists

• Component: HTML Editing
• Status: NEW

[Julie Parent]

Repro steps:
1. Create a list, with br's inside the li, aka, html is <ul><li>one<br>two<br>three</li></ul>
2. Put cursor anywhere on the line with "three"
3. Indent (aka, execCommand('indent'))

Result: "three" moves above "one" in its own list (html is: <ul><ul><li>three</li></ul><li>one<br>two<br></li></ul>)
Expected Result: "three" is indented, and stays after "two"

Seen in Safari 4.0.4 and Chrome 4.0.266.0.

Plexode demo - just click "eval js" to see it execute the indent: http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cdiv%20contentEditable%3E%3Cul%3E%3Cli%20id%3D'li'%3Eone%3Cbr%3Etwo%3Cbr%3Ethree%3C%2Fli%3E%3C%2Ful%3E%3C%2Fdiv%3E&ohh=1&ohj=1&jt=%2F%2F%20Put%20cursor%20near%20%22three%22%20and%20indent%0Avar%20r%20%3D%20document.createRange()%3B%0Ar.setStart(li%2C%204)%3B%0Ar.setEnd(li%2C%204)%3B%0As%20%3D%20window.getSelection()%3B%0As.removeAllRanges()%3B%0As.addRange(r)%3B%0Adocument.execCommand('indent')%3B&ojh=1&ojj=0&ms=100&oth=0&otj=0&cex=1

Also seen in all Google's editing products (Gmail, Docs, Sites, etc) and Midas Demo.

[Sudarshan C P]

Created attachment 207690 [details]
Patch

[Darin Adler]

Comment on attachment 207690 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207690&action=review

review- because this seems to have a bad cast in it and because there is no test case; we require test cases for WebKit bug fixes.

> Source/WebCore/editing/IndentOutdentCommand.cpp:73
> +    Element* currentElement = static_cast<Element*>(lastNodeInSelectedParagraph);

Should use toElement. But what guarantees this is an element, not a non-element node?

[Sudarshan C P]

Created attachment 208513 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 208513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208513&action=review

> Source/WebCore/ChangeLog:15
> +        Place the cursor before or after the <br> tag,inside the <li> element
> +        and apply the indent command ,applied element always moves to beginning
> +        of the list item.
> +        <br> tag is present inside the <li> tag , text range is split based on
> +        the <br> element present in the <li> element, When we apply  indent to
> +        particular text range of the <li> element ,we need to split the node based
> +        on the previous and  next element , to place the newly created element of 
> +        <ul><li>Text</li></ul>,so added new logic to handle this scenario.

Comma is all messed up in this description.  Each comma should appear immediately after the preceding letters and should be followed by exactly one space.
e.g. hello, world.  Not hello , world or hello,world.
Also, I don't really get what you mean in the second paragraph. Please revise it.

> Source/WebCore/ChangeLog:18
> +        Test: editing/execCommand/indent_on_li_element_containing_br_text.html

Please use - instead of _ to separate words.

> Source/WebCore/editing/IndentOutdentCommand.cpp:-73
> -    // FIXME: previousElementSibling does not ignore non-rendered content like <span></span>.  Should we?

Why are we removing this comment?

> Source/WebCore/editing/IndentOutdentCommand.cpp:74
> +    Node* beforeLastNode = lastNodeInSelectedParagraph->previousSibling();
> +    Node* afterLastNode = lastNodeInSelectedParagraph->nextSibling();

Why are we getting previous and next siblings?  They may not be even visible. e.g. collapsed whitespaces.

> Source/WebCore/editing/IndentOutdentCommand.cpp:78
> +    if (!beforeLastNode && afterLastNode)
> +        insertNodeBefore(newList, selectedListItem);

I don't understand why we need to adjust where we insert a list item based on the content of li.
It looks like you're simply trying to work around a bug in moveParagraphWithClones,

> Source/WebCore/editing/IndentOutdentCommand.cpp:80
> +        splitElement(selectedListItem, lastNodeInSelectedParagraph);

lastNodeInSelectedParagraph is not necessary a child of selectedListItem. e.g. <li><span><br></span></li>
r- because of this wrong assumption.