| Summary: | Copying and pasting trivial H2 content causes a crash in firstPositionInNode | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Steven Roussey <sroussey> | ||||||||
| Component: | HTML Editing | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ap, ddkilzer, dino, enrica, jonlee, mmaxfield, rniwa, simon.fraser, thorton | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Mac (Intel) | ||||||||||
| OS: | OS X 10.9 | ||||||||||
| URL: | http://jsfiddle.net/qhLAN/ | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=158724 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Steven Roussey
2014-07-14 13:21:54 PDT
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. |