Editing commands executed during ReplaceSelectionCommand::moveNodeOutOfAncestor() (utilized by makeInsertedContentRoundTrippableWithHTMLTreeBuilder and responsible for moving prohibited children out of their ancestors) are not applied properly when attempting to modify a non-editable element. As a result, the code that follows causes a crash.
Created attachment 224747 [details] Proposed patch
Attachment 224747 [details] did not pass style-queue: ERROR: Source/WebCore/editing/CompositeEditCommand.cpp:1517: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > ERROR: Source/WebCore/editing/CompositeEditCommand.cpp:1517: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] This in fact looks like a false positive.
Created attachment 224754 [details] Simple test case (.html) Attached a simple test case. Inserting a horizontal rule anywhere in the paragraph should cause a crash.
The aforementioned crash is also related to the commit e32f78a11ed2f7bd9b46bb82fdcb426c3c65aace.
(In reply to comment #5) > The aforementioned crash is also related to the commit e32f78a11ed2f7bd9b46bb82fdcb426c3c65aace. http://trac.webkit.org/changeset/145253
Comment on attachment 224747 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=224747&action=review > Source/WebCore/ChangeLog:12 > + Editing commands executed during > + ReplaceSelectionCommand::moveNodeOutOfAncestor() (utilized by > + makeInsertedContentRoundTrippableWithHTMLTreeBuilder and responsible for > + moving prohibited children out of their ancestors) are not applied > + properly when attempting to modify a non-editable element. We should never be modifying non-editable element. That's what we need to fix. We shouldn't be working-around our bugs like this by ignoring contenteditable value. r-.
I believe that final result of the code is correct according to w3c spec. According to spec final effect of that Robert’s solution is correct. Even if at the end we move child to non-editable parent (after split). Specification details: https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-inserthorizontalrule-command -> step 2. If "p" is not an allowed child of the editing host of node, abort these steps. --> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#fix-disallowed-ancestors <-- p is not allowed child of p -> step 4: While node is not an allowed child of its parent, split the parent of the one-node list consisting of node. --> split the parent: ---> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#split-the-parent Do you have any other idea to implement it since embedding <hr> in <p> is also bad...
> Do you have any other idea to implement it since embedding <hr> in <p> is also bad... This, I believe, relates to the solution found in Blink, which leaves the <hr> child inside <p> whenever it would end up messing with a non-editable ancestor. Besides the obvious advantage it seems to me that such approach effectively undermines the whole idea of moving out prohibited child elements.
w3c specification says specifically that we should split the parent and we should move child to grandpa ;) (parent of current parent), specification does not say if new parent must be editable. That is why I think your patch is correct. On the other hand leaving hr nested in p is violation of older html standard so I do not like blink solution.
Our solution made a bad impression by admitting that we end up implicitly inserting a node under a non-editable element, yet we found this approach to be the lesser evil. Also, the fact that approach similar to ours has been adopted in Gecko made us believe this was the right way. Additionally, I would like to point out that a manner in which this goal is achieved is no different to approaches already present in the code.
We DO NOT follow the editing specifications. The existing editing API specification has NOT been thoroughly reviewed by implementors including us, WebKit. In fact, our current implementation is so different from the specification that following the spec. in some edge cases is almost never the right thing to do.
(In reply to comment #11) > Our solution made a bad impression by admitting that we end up implicitly inserting a node under a non-editable element, yet we found this approach to be the lesser evil. It didn't make a bad impression. I noticed this without reading the change log. The only reason I commented on the change log was because you explicitly mentioned it. I would have r-ed it regardless. We shouldn't be modifying a node outside of a root content editable node. In fact, as it stands, we support -webkit-user-modify CSS property to control the editability so we must constantly query whether a given node is editable or not since a mere act of inserting a new node could affect the editability in the presence of that CSS property.
Comment on attachment 224747 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=224747&action=review > LayoutTests/editing/execCommand/insertHorizontalRule-into-paragraph-crash-expected.txt:23 > +| <p> > +| contenteditable="true" > +| id="testParagraph" > +| "Foo" > +| <hr> > +| <#selection-caret> > +| <p> > +| contenteditable="true" > +| "Bar" This output is incorrect. The hr element must appear inside a single p element between "Foo" and "Bar". We need to make insertHorizontalRule fail or simply strip the hr element in makeInsertedContentRoundTrippableWithHTMLTreeBuilder when we can't modify the root editable element in cases like this.
Created attachment 226682 [details] Proposed patch This approach attempts to make insertHorizontalRule fail when faced with either inserting a forbidden child or placing it under a non-editable element.
Created attachment 226683 [details] Simple test case
Comment on attachment 226682 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=226682&action=review > LayoutTests/editing/execCommand/insertHorizontalRule-into-paragraph-crash.html:28 > +Markup.waitUntilDone(); > + > +function runTests() > +{ > + Markup.dump("content", "Before execCommand('insertHorizontalRule')"); > + > + var node = document.getElementById("testParagraph").firstChild; > + > + setSelectionCommand(node, 3, node, 3); > + document.execCommand("insertHorizontalRule", false); > + > + Markup.dump("content", "After execCommand('insertHorizontalRule')"); > + > + Markup.notifyDone(); > +} Just move all the script elements to after #content and run the code there. Then we wouldn't need waitUntilDone/notifyDone calls. > Source/WebCore/editing/CompositeEditCommand.cpp:232 > + > + // If the command has failed, unapply it entirely. > + if (hasFailed()) > + composition()->unapply(); I'm not sure if this is desirable. I think the user would like to see things happen even if failed to complete. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1158 > + if (!makeInsertedContentRoundTrippableWithHTMLTreeBuilder(insertedNodes)) { > + this->reportFailure(); > + return; > + } I think it's better to simply exit here.
Created attachment 227173 [details] Proposed patch
Comment on attachment 227173 [details] Proposed patch The following patch, instead of undoing the whole command, attempts to avoid inserting unmovable forbidden child nodes while executing the ReplaceSelectionCommand, using a method derived from existing code. This approach has been chosen (instead of removing the offending nodes /after/ inserting them) to prevent the registration of undo steps for commands that have no effect. This behavior has been highlighted in a new test.
Any comments appreciated.
Comment on attachment 227173 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=227173&action=review > Source/WebCore/ChangeLog:18 > + The following patch attempts to prevent insertion of unmovable forbidden > + child nodes during execution of ReplaceSelectionCommand. Additionally, > + it prevents the registration of undo steps for CompositeEditCommands that > + contain no commands (and have no effect). Why are we doing this in the same patch? We should split this into a separate patch if anything. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1165 > + // A failure to unapply the last command means the text node split wasn't > + // the last command and warrants a crash. Why? It's extremely bad to crash in the release. A broken editing behavior is much better than losing all the data.
Comment on attachment 227173 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=227173&action=review > LayoutTests/editing/execCommand/insertHorizontalRule-into-paragraph-undo.html:17 > + + "Performing undo twice should remove both 'Foo' and 'Baz' since the " > + + "failing InsertHorizontalRule command should not have its undo steps " > + + "registered."); Why is InsertHorizontalRule so special? There are other editing commands that fail to do anything useful in these edge cases.
(In reply to comment #22) > Why is InsertHorizontalRule so special? There are other editing commands that fail to do anything useful in these edge cases. If there are really that many commands that could end up in registering an empty undo step then I see no reason for complicating things for the sake of simple ReplaceSelectionCommand. Let's try a simpler approach then...
Created attachment 228048 [details] [test] Proposed patch
Comment on attachment 228048 [details] [test] Proposed patch Attachment 228048 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5957258252910592 New failing tests: editing/pasteboard/paste-text-011.html editing/pasteboard/paste-text-016.html editing/pasteboard/block-wrappers-necessary.html
Created attachment 228055 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 228048 [details] [test] Proposed patch Attachment 228048 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5429642458562560 New failing tests: editing/pasteboard/paste-text-011.html editing/pasteboard/paste-text-016.html editing/pasteboard/block-wrappers-necessary.html
Created attachment 228060 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 229131 [details] Proposed patch Let's try that again...
Comment on attachment 229131 [details] Proposed patch The following approach prevents the insertion of unmovable forbidden child nodes that otherwise would have to be removed.
*** This bug has been marked as a duplicate of bug 132633 ***
Comment on attachment 229131 [details] Proposed patch Cleared review? from attachment 229131 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).