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+

David Bloom
Reported 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"
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
DetailsFormatted DiffDiff
fixed the style (10.34 KB, patch)
2010-03-19 17:25 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
added more tests and fixed a bug in the patch (12.59 KB, patch)
2010-03-21 16:18 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2009-06-29 11:06:36 PDT
Cannot reproduce on nightly build. I suspect that it has been fixed.
Ryosuke Niwa
Comment 2 2010-03-19 10:38:57 PDT
Created attachment 51166 [details] demonstrates the bug Now the bug reproduces. Maybe it's a regression.
Ryosuke Niwa
Comment 3 2010-03-19 10:39:32 PDT
+tony
Ryosuke Niwa
Comment 4 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.
WebKit Review Bot
Comment 5 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.
Ryosuke Niwa
Comment 6 2010-03-19 17:25:07 PDT
Created attachment 51208 [details] fixed the style
Ryosuke Niwa
Comment 7 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.
Ryosuke Niwa
Comment 8 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.
Justin Garcia
Comment 9 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
Ryosuke Niwa
Comment 10 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.
Ryosuke Niwa
Comment 11 2010-04-21 00:15:11 PDT
Committed in http://trac.webkit.org/changeset/57954.
Note You need to log in before you can comment on or make changes to this bug.