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"
Cannot reproduce on nightly build. I suspect that it has been fixed.
Created attachment 51166 [details] demonstrates the bug Now the bug reproduces. Maybe it's a regression.
+tony
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.
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.
Created attachment 51208 [details] fixed the style
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.
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 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
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.
Committed in http://trac.webkit.org/changeset/57954.