Dragging text from one paragraph to another does not render as expected
Created attachment 230954 [details] Patch
<rdar://problem/14500251>
Comment on attachment 230954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230954&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:633 > + if (paragraphElement && paragraphElement->parentNode()->hasEditableStyle()) Do we have an ironclad guarantee that paragraphElement->parentNode() is non-null? No way it could have been removed from its parent during the editing process?
Comment on attachment 230954 [details] Patch Attachment 230954 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4853441708949504 New failing tests: svg/text/non-bmp-positioning-lists.svg editing/pasteboard/drag-drop-paragraph-crasher.html
Created attachment 230964 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 230954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230954&action=review > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:10 > +function debug(msg) { > + var console = document.getElementById('console'); > + var line = document.createElement('div'); > + line.textContent = msg; > + console.appendChild(line); > +} > + We don't need to have this helper function if we had used editing.js or dump-as-markup as I described below. > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:16 > + testRunner.waitUntilDone(); We don't need to call waitUntilDone/notifyDone since we're not doing anything inside a timer. > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:18 > + // Drag text in the source Instead of adding a comment like this, add a helper function of the said name. e.g. selectNodeContents(document.getElementById("source")). > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:19 > + var sel = window.getSelection(); Please don't use abbreviations like sel. > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:22 > + var range = document.createRange(); > + range.setStartBefore(document.getElementById("source")); > + range.setEndBefore(document.getElementById("destination")); Why can't we just call selectNodeContents on source? > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:29 > + x = source.offsetLeft + 10; > + y = source.offsetTop + source.offsetHeight / 2; > + eventSender.mouseMoveTo(x, y); > + eventSender.mouseDown(); These x and y variables are used exactly once. I'd rather code it directly in moveMouseTo instead. > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:39 > + var result = destination.value; > + debug("Pass"); > + > + testRunner.notifyDone(); We should dump markup using dump-as-markup.js to also verify the output in the case it regresses in the future. > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:49 > + <p> > + <span id=source>Select > + <p>me</p> > + <p id=destination contenteditable>and drag it here</p> > + </span> > + </p> There are tab characters here. > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:52 > +<script>editingTest();</script> editingTest() idiom is deprecated. We should just move the script above here instead.
I don't believe we have such a guarantee, though current code uses parent()-> without a null check. I might as well practice defensive programming.
Comment on attachment 230954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230954&action=review >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:633 >> + if (paragraphElement && paragraphElement->parentNode()->hasEditableStyle()) > > Do we have an ironclad guarantee that paragraphElement->parentNode() is non-null? No way it could have been removed from its parent during the editing process? I don't believe we have such a guarantee, though current code uses parent()-> without a null check. I might as well practice defensive programming. Done. >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:10 >> + > > We don't need to have this helper function if we had used editing.js or dump-as-markup as I described below. Done. >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:16 >> + testRunner.waitUntilDone(); > > We don't need to call waitUntilDone/notifyDone since we're not doing anything inside a timer. Done. >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:18 >> + // Drag text in the source > > Instead of adding a comment like this, add a helper function of the said name. > e.g. selectNodeContents(document.getElementById("source")). Done. >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:19 >> + var sel = window.getSelection(); > > Please don't use abbreviations like sel. Done. >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:22 >> + range.setEndBefore(document.getElementById("destination")); > > Why can't we just call selectNodeContents on source? We need the selection to include the inner <p> >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:29 >> + eventSender.mouseDown(); > > These x and y variables are used exactly once. I'd rather code it directly in moveMouseTo instead. Done. >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:39 >> + testRunner.notifyDone(); > > We should dump markup using dump-as-markup.js to also verify the output in the case it regresses in the future. The dumpAsText() should handle this. >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:49 >> + </p> > > There are tab characters here. Done. >> LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:52 >> +<script>editingTest();</script> > > editingTest() idiom is deprecated. We should just move the script above here instead. Done.
Created attachment 231031 [details] Patch
Comment on attachment 231031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231031&action=review > LayoutTests/editing/pasteboard/drag-drop-paragraph-crasher.html:30 > + // and drop it off to the destination field. This should not be here
Created attachment 231032 [details] Patch
Comment on attachment 231032 [details] Patch Attachment 231032 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6585688858296320 New failing tests: svg/text/non-bmp-positioning-lists.svg editing/pasteboard/drag-drop-paragraph-crasher.html
Created attachment 231033 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 231035 [details] Patch
Comment on attachment 231035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231035&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:632 > + auto* paragraphElement = enclosingElementWithTag(positionInParentBeforeNode(node.get()), pTag); Put auto* ~ inside if () to limit the scope. Basically, the first line doesn't need to change other than adding {.
http://trac.webkit.org/changeset/168460
*** Bug 129098 has been marked as a duplicate of this bug. ***