Bug 19539 - Nested <ul>s are mishandled when converted to <ol> using execCommand('insertorderedlist')
Summary: Nested <ul>s are mishandled when converted to <ol> using execCommand('inserto...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 35281 36430
  Show dependency treegraph
 
Reported: 2008-06-13 11:18 PDT by David Bloom
Modified: 2010-04-21 00:15 PDT (History)
5 users (show)

See Also:


Attachments
demonstrates the bug (905 bytes, text/html)
2010-03-19 10:38 PDT, Ryosuke Niwa
no flags Details
fixes InsertListCommand::doApply to merge lists after inserting a list element (10.34 KB, patch)
2010-03-19 16:18 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed the style (10.34 KB, patch)
2010-03-19 17:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
added more tests and fixed a bug in the patch (12.59 KB, patch)
2010-03-21 16:18 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
reverted the change to editing/execCommand/create-list-from-range-selection.html (11.72 KB, patch)
2010-03-21 16:27 PDT, Ryosuke Niwa
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.