Bug 19539

Summary: Nested <ul>s are mishandled when converted to <ol> using execCommand('insertorderedlist')
Product: WebKit Reporter: David Bloom <futurama>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: jparent, justin.garcia, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 35281, 36430    
Attachments:
Description Flags
demonstrates the bug
none
fixes InsertListCommand::doApply to merge lists after inserting a list element
none
fixed the style
none
added more tests and fixed a bug in the patch
none
reverted the change to editing/execCommand/create-list-from-range-selection.html justin.garcia: review+

Description David Bloom 2008-06-13 11:18:16 PDT
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"
Comment 1 Ryosuke Niwa 2009-06-29 11:06:36 PDT
Cannot reproduce on nightly build.  I suspect that it has been fixed.
Comment 2 Ryosuke Niwa 2010-03-19 10:38:57 PDT
Created attachment 51166 [details]
demonstrates the bug

Now the bug reproduces. Maybe it's a regression.
Comment 3 Ryosuke Niwa 2010-03-19 10:39:32 PDT
+tony
Comment 4 Ryosuke Niwa 2010-03-19 16:18:00 PDT
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.
Comment 5 WebKit Review Bot 2010-03-19 16:22:29 PDT
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.
Comment 6 Ryosuke Niwa 2010-03-19 17:25:07 PDT
Created attachment 51208 [details]
fixed the style
Comment 7 Ryosuke Niwa 2010-03-21 16:18:41 PDT
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.
Comment 8 Ryosuke Niwa 2010-03-21 16:27:08 PDT
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.
Comment 9 Justin Garcia 2010-04-20 13:59:35 PDT
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
Comment 10 Ryosuke Niwa 2010-04-20 14:22:05 PDT
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.
Comment 11 Ryosuke Niwa 2010-04-21 00:15:11 PDT
Committed in http://trac.webkit.org/changeset/57954.