Bug 132633

Summary: Dragging text from one paragraph to another does not render as expected
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dino, enrica, jonlee, rniwa, r.plociennik, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Patch rniwa: review+

Myles C. Maxfield
Reported 2014-05-06 17:13:03 PDT
Dragging text from one paragraph to another does not render as expected
Attachments
Patch (5.44 KB, patch)
2014-05-06 17:16 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (464.30 KB, application/zip)
2014-05-06 19:21 PDT, Build Bot
no flags
Patch (5.33 KB, patch)
2014-05-07 17:45 PDT, Myles C. Maxfield
no flags
Patch (5.28 KB, patch)
2014-05-07 17:48 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (512.86 KB, application/zip)
2014-05-07 18:15 PDT, Build Bot
no flags
Patch (5.95 KB, patch)
2014-05-07 18:36 PDT, Myles C. Maxfield
rniwa: review+
Myles C. Maxfield
Comment 1 2014-05-06 17:16:59 PDT
Myles C. Maxfield
Comment 2 2014-05-06 17:18:08 PDT
Darin Adler
Comment 3 2014-05-06 17:57:12 PDT
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?
Build Bot
Comment 4 2014-05-06 19:21:53 PDT
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
Build Bot
Comment 5 2014-05-06 19:21:55 PDT
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
Ryosuke Niwa
Comment 6 2014-05-06 21:45:15 PDT
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.
Myles C. Maxfield
Comment 7 2014-05-07 16:09:27 PDT
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.
Myles C. Maxfield
Comment 8 2014-05-07 17:39:11 PDT
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.
Myles C. Maxfield
Comment 9 2014-05-07 17:45:04 PDT
Myles C. Maxfield
Comment 10 2014-05-07 17:47:56 PDT
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
Myles C. Maxfield
Comment 11 2014-05-07 17:48:46 PDT
Build Bot
Comment 12 2014-05-07 18:15:37 PDT
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
Build Bot
Comment 13 2014-05-07 18:15:41 PDT
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
Myles C. Maxfield
Comment 14 2014-05-07 18:36:32 PDT
Ryosuke Niwa
Comment 15 2014-05-07 19:45:12 PDT
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 {.
Myles C. Maxfield
Comment 16 2014-05-07 19:52:43 PDT
Rob Płóciennik
Comment 17 2014-06-09 00:19:00 PDT
*** Bug 129098 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.