repro <dl><div id="div" contenteditable="true"A><script> div.focus(); document.execCommand("InsertUnorderedList"); </script> Crash WebCore::InsertListCommand::unlistifyParagraph(WebCore::VisiblePosition const&, WebCore::HTMLElement*, WebCore::Node*) WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::QualifiedName const&, WebCore::Range*) WebCore::InsertListCommand::doApply() WebCore::EditCommand::apply() WebCore::executeInsertUnorderedList(WebCore::Frame*, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) third_party/WebKit/Source/WebCore/editing/EditorCommand.cpp:0 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
Created attachment 107021 [details] Patch
Comment on attachment 107021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review > Source/WebCore/editing/htmlediting.cpp:669 > - if (n->hasTagName(liTag) || isListElement(n->parentNode())) > + if (n->hasTagName(liTag) || (isListElement(n->parentNode()) && n->parentNode()->rendererIsEditable())) This change seems wrong. You should be checking that n != root instead.
Comment on attachment 107021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review > LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:9 > +document.writeln('execCommand("InsertUnorderedList") for contenteditable root element was crashing.<br><br>'); > +document.writeln('The test has passed if it does not crash.<br><br>'); Why do we need so many brs?
Comment on attachment 107021 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review >> LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:9 >> +document.writeln('The test has passed if it does not crash.<br><br>'); > > Why do we need so many brs? Do we do correct thing? i.e. we should get <ul><li><br></li></ul> inside the div after executing InsertUnorderedList. You can use dump-as-markup to verify this.
Created attachment 107030 [details] Patch
(In reply to comment #2) > (From update of attachment 107021 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review > > > Source/WebCore/editing/htmlediting.cpp:669 > > - if (n->hasTagName(liTag) || isListElement(n->parentNode())) > > + if (n->hasTagName(liTag) || (isListElement(n->parentNode()) && n->parentNode()->rendererIsEditable())) > > This change seems wrong. You should be checking that n != root instead. Fixed.
(In reply to comment #4) > (From update of attachment 107021 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107021&action=review > > >> LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:9 > >> +document.writeln('The test has passed if it does not crash.<br><br>'); > > > > Why do we need so many brs? > > Do we do correct thing? i.e. we should get <ul><li><br></li></ul> inside the div after executing InsertUnorderedList. You can use dump-as-markup to verify this. I'm using dump-as-markup for the expected result now. The result seems OK.
Comment on attachment 107030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107030&action=review > LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:1 > +<script src="../../resources/dump-as-markup.js"></script> Can we have <!DOCTYPE html>? or would that break the test? > Source/WebCore/ChangeLog:8 > + execCommand("InsertUnorderedList") was crashing if the parent node of the target is a kind of list element and it is not contenteditable. Nit: This line seems too long.
*** Bug 67966 has been marked as a duplicate of this bug. ***
Created attachment 107130 [details] Patch
(In reply to comment #8) > (From update of attachment 107030 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107030&action=review > > > LayoutTests/editing/execCommand/insert-list-in-noneditable-list-parent.html:1 > > +<script src="../../resources/dump-as-markup.js"></script> > > Can we have <!DOCTYPE html>? or would that break the test? I've added it. It did not break the test. The test fails without my patch, and passes with my patch. > > Source/WebCore/ChangeLog:8 > > + execCommand("InsertUnorderedList") was crashing if the parent node of the target is a kind of list element and it is not contenteditable. > > Nit: This line seems too long. Fixed.
Annie pointed out that we should probably add test cases for <ul><div id="div" contenteditable="true"A><script> div.focus(); document.execCommand("InsertUnorderedList"); </script> and <ol><div id="div" contenteditable="true"A><script> div.focus(); document.execCommand("InsertUnorderedList"); </script> because she's working on a patch to make isListElement return false for dl.
(In reply to comment #12) > Annie pointed out that we should probably add test cases for I meant to say "I pointed out to Annie"
Created attachment 107134 [details] Patch
I added test cases for ul and ol. Could you review it again? (In reply to comment #12) > Annie pointed out that we should probably add test cases for > > <ul><div id="div" contenteditable="true"A><script> > div.focus(); > document.execCommand("InsertUnorderedList"); > </script> > > and > > <ol><div id="div" contenteditable="true"A><script> > div.focus(); > document.execCommand("InsertUnorderedList"); > </script> > > because she's working on a patch to make isListElement return false for dl.
Does the test case still work?
(In reply to comment #16) > Does the test case still work? Yes. I have tested 4 patterns: ul only, ol only, dl only, and all of them. All patterns crash without my patch, and they passes with my patch.
Comment on attachment 107134 [details] Patch (In reply to comment #17) > (In reply to comment #16) > > Does the test case still work? > > Yes. I have tested 4 patterns: ul only, ol only, dl only, and all of them. > All patterns crash without my patch, and they passes with my patch. Great!
Comment on attachment 107134 [details] Patch Clearing flags on attachment: 107134 Committed r95017: <http://trac.webkit.org/changeset/95017>
All reviewed patches have been landed. Closing bug.