134897 – Copying and pasting trivial H2 content causes a crash in firstPositionInNode

RESOLVED FIXED 134897

Copying and pasting trivial H2 content causes a crash in firstPositionInNode

https://bugs.webkit.org/show_bug.cgi?id=134897

Summary Copying and pasting trivial H2 content causes a crash in firstPositionInNode

Steven Roussey

Reported 2014-07-14 13:21:54 PDT

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/

Attachments
Patch (4.72 KB, patch) 2014-07-16 18:46 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch (4.71 KB, patch) 2014-07-17 10:28 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch (6.33 KB, patch) 2014-07-22 18:31 PDT, Myles C. Maxfield	rniwa: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Steven Roussey

Comment 1 2014-07-14 13:23:33 PDT

For reference: https://code.google.com/p/chromium/issues/detail?id=245773

Alexey Proskuryakov

Comment 2 2014-07-15 20:54:59 PDT

I can reproduce with current nightly.

Myles C. Maxfield

Comment 3 2014-07-16 18:46:42 PDT

Created attachment 235043 [details] Patch

Ryosuke Niwa

Comment 4 2014-07-16 18:56:27 PDT

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.

Ryosuke Niwa

Comment 5 2014-07-16 19:02:25 PDT

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.

Myles C. Maxfield

Comment 6 2014-07-16 19:06:45 PDT

I wanted to match the assert in InsertNodeBeforeCommand::InsertNodeBeforeCommand(). Should I update that function too, Ryosuke?

Ryosuke Niwa

Comment 7 2014-07-16 19:31:59 PDT

(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.

Myles C. Maxfield

Comment 8 2014-07-17 10:27:15 PDT

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.

Myles C. Maxfield

Comment 9 2014-07-17 10:28:27 PDT

Created attachment 235073 [details] Patch

Ryosuke Niwa

Comment 10 2014-07-17 12:18:03 PDT

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.

Ryosuke Niwa

Comment 11 2014-07-17 12:38:41 PDT

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

Myles C. Maxfield

Comment 12 2014-07-22 18:31:18 PDT

Created attachment 235333 [details] Patch

Ryosuke Niwa

Comment 13 2014-07-22 18:35:00 PDT

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').

Myles C. Maxfield

Comment 14 2014-07-22 19:14:48 PDT

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.

Myles C. Maxfield

Comment 15 2014-07-22 19:20:43 PDT

http://trac.webkit.org/changeset/171383

David Kilzer (:ddkilzer)

Comment 16 2016-06-14 10:19:09 PDT

<rdar://problem/15457831>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Mac (Intel)

OS OS X 10.9

Product WebKit

Component HTML Editing

Assignee

Myles C. Maxfield

Reported

2014-07-14 13:21 PDT

Modified

2016-06-14 10:19 PDT History

CC List

9 users Show

URL

http://jsfiddle.net/qhLAN/

Keywords InRadar

Depends on

Blocks