Bug 19539 – Nested <ul>s are mishandled when converted to <ol> using execCommand('insertorderedlist')

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[David Bloom]

Steps to reproduce:
1. go to http://www.mozilla.org/editor/midasdemo/
2. type: "test\ntest\ntest" (with the newlines unescaped ;-))
3. select the text
4. click the "Bulleted list" icon in the toolbar
5. click the "indent right" icon in the toolbar
6. click the "Numbered list" icon in the toolbar

expected: "1. test
           2. test
           3. test"

actual: "1. test
         1. test
         1. test"

[Ryosuke Niwa]

Cannot reproduce on nightly build.  I suspect that it has been fixed.

[Ryosuke Niwa]

Created attachment 51166 [details]
demonstrates the bug

Now the bug reproduces. Maybe it's a regression.

[Ryosuke Niwa]

+tony

[Ryosuke Niwa]

Created attachment 51201 [details]
fixes InsertListCommand::doApply to merge lists after inserting a list element

The bug was caused by the second part of InsertListCommand::doApply where it does not merge lists after inserting a list element.  While I'm quite convinced that we need to cleanup InsertListCommand.cpp, this bug seems to be simple enough that we can fix it now.

[WebKit Review Bot]

Attachment 51201 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/editing/htmlediting.h:198:  Missing spaces around =  [whitespace/operators] [4]
WebCore/editing/htmlediting.h:198:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 51208 [details]
fixed the style

[Ryosuke Niwa]

Created attachment 51260 [details]
added more tests and fixed a bug in the patch

There was a bug in my patch. Merging lists must occur after moveParagraph moved the paragraph because nextList isn't visually next to the inserted list element otherwise. Fixed the problem and added test cases to demonstrate this point.

[Ryosuke Niwa]

Created attachment 51262 [details]
reverted the change to editing/execCommand/create-list-from-range-selection.html

For some reason, editing/execCommand/create-list-from-range-selection.html passes without the change.  Fixed the patch.

[Justin Garcia]

Comment on attachment 51262 [details]
reverted the change to editing/execCommand/create-list-from-range-selection.html

> -        if (nextList && previousList)
> +        if (m_listElement) {
> +            if (canMergeLists(previousList, m_listElement.get()))
> +                mergeIdenticalElements(previousList, m_listElement.get());
> +            if (canMergeLists(m_listElement.get(), nextList))
> +                mergeIdenticalElements(m_listElement.get(), nextList);

Is m_listElement necessarily still valid if it and previousList got merged?


> -HTMLElement* outermostEnclosingList(Node* node)
> +HTMLElement* outermostEnclosingList(Node* node, Node* rootNode)

I would call this rootList.

r=me

[Ryosuke Niwa]

Thanks for the review!

(In reply to comment #9)
> (From update of attachment 51262 [details])
> > -        if (nextList && previousList)
> > +        if (m_listElement) {
> > +            if (canMergeLists(previousList, m_listElement.get()))
> > +                mergeIdenticalElements(previousList, m_listElement.get());
> > +            if (canMergeLists(m_listElement.get(), nextList))
> > +                mergeIdenticalElements(m_listElement.get(), nextList);
> Is m_listElement necessarily still valid if it and previousList got merged?

Yes. mergeIdenticalElements puts the contents of the first element into the second element and deletes the first element, so the second element survives.

> > -HTMLElement* outermostEnclosingList(Node* node)
> > +HTMLElement* outermostEnclosingList(Node* node, Node* rootNode)
> I would call this rootList.

Done.

[Ryosuke Niwa]

Committed in http://trac.webkit.org/changeset/57954.