Copy from one content editable and paste in another results in a crash. Seems like a repeat of a bug fixed in Chrome last year. Sample site and instructions can be found here: http://jsfiddle.net/qhLAN/
For reference: https://code.google.com/p/chromium/issues/detail?id=245773
I can reproduce with current nightly.
Created attachment 235043 [details] Patch
Comment on attachment 235043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235043&action=review > LayoutTests/editing/pasteboard/heading-crash.html:1 > +<html> Missing DOCTYPE. > LayoutTests/editing/pasteboard/heading-crash.html:3 > +<script src="../../resources/js-test-pre.js"></script> Why do we need js-test-pre in this test? > LayoutTests/editing/pasteboard/heading-crash.html:8 > + <h2 id="source" contenteditable="true">Copy This Text</h2> > + <h2 id="destination" contenteditable="true">Paste Here</h2> We should dump the result using dump-as-markup.js so that we can see the pasted markup.
Comment on attachment 235043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235043&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:642 > + if (headerElement && headerElement->parentNode() && headerElement->parentNode()->hasEditableStyle()) Also, we should probably call isContentRichlyEditable instead.
I wanted to match the assert in InsertNodeBeforeCommand::InsertNodeBeforeCommand(). Should I update that function too, Ryosuke?
(In reply to comment #6) > I wanted to match the assert in InsertNodeBeforeCommand::InsertNodeBeforeCommand(). Should I update that function too, Ryosuke? isContentRichlyEditable updates whereas hasEditableStyle doesn't so we should continue to use hasEditableStyle in the assertion. i.e. no code change.
Comment on attachment 235043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235043&action=review >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:642 >> + if (headerElement && headerElement->parentNode() && headerElement->parentNode()->hasEditableStyle()) > > Also, we should probably call isContentRichlyEditable instead. Done. >> LayoutTests/editing/pasteboard/heading-crash.html:1 >> +<html> > > Missing DOCTYPE. Done. >> LayoutTests/editing/pasteboard/heading-crash.html:3 >> +<script src="../../resources/js-test-pre.js"></script> > > Why do we need js-test-pre in this test? for description() and finishJSTest() (in js-test-post.js) >> LayoutTests/editing/pasteboard/heading-crash.html:8 >> + <h2 id="destination" contenteditable="true">Paste Here</h2> > > We should dump the result using dump-as-markup.js so that we can see the pasted markup. Done.
Created attachment 235073 [details] Patch
Comment on attachment 235073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235073&action=review > LayoutTests/editing/pasteboard/heading-crash-expected.txt:6 > +| <h2> > +| id="source" > +| "Copy This Text<#selection-caret>" This output is incorrect. We should be stripping the inner h2. Otherwise, we'd create two h2s next to each other. So the fix is incorrect.
Comment on attachment 235073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235073&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:643 > + if (headerElement && headerElement->parentNode() && headerElement->parentNode()->isContentRichlyEditable()) > moveNodeOutOfAncestor(node, headerElement); In the case where we're already inside a header element, we should probably convert the element into span. See replaceElementWithSpanPreservingChildrenAndAttributes
Created attachment 235333 [details] Patch
Comment on attachment 235333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235333&action=review > LayoutTests/editing/pasteboard/heading-crash.html:6 > +<head> > +<script src="../editing.js"></script> > +<script src="../../resources/dump-as-markup.js"></script> > +</head> We don't need to put this in head. > LayoutTests/editing/pasteboard/heading-crash.html:12 > + <h2 id="source" contenteditable="true">Copy This Text</h2> > + <h2 id="destination" contenteditable="true">Paste Here</h2> > + <script> > + if (window.testRunner) > + testRunner.dumpAsText(); We don't normally indent test content like this. > LayoutTests/editing/pasteboard/heading-crash.html:21 > + var range = document.createRange(); > + source.focus(); > + range.selectNodeContents(source); > + selection.removeAllRanges(); > + selection.addRange(range); Why don't you just do document.execCommand('selectAll') instead? > LayoutTests/editing/pasteboard/heading-crash.html:27 > + selection.removeAllRanges(); > + range.selectNodeContents(destination); > + selection.addRange(range); Ditto. destination.focus(); document.execCommand('selectAll').
Comment on attachment 235333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235333&action=review >> LayoutTests/editing/pasteboard/heading-crash.html:6 >> +</head> > > We don't need to put this in head. Done. >> LayoutTests/editing/pasteboard/heading-crash.html:12 >> + testRunner.dumpAsText(); > > We don't normally indent test content like this. Done. >> LayoutTests/editing/pasteboard/heading-crash.html:21 >> + selection.addRange(range); > > Why don't you just do document.execCommand('selectAll') instead? Good idea. Done. >> LayoutTests/editing/pasteboard/heading-crash.html:27 >> + selection.addRange(range); > > Ditto. destination.focus(); document.execCommand('selectAll'). Done.
http://trac.webkit.org/changeset/171383
<rdar://problem/15457831>