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+

Description Myles C. Maxfield 2014-05-06 17:13:03 PDT
Dragging text from one paragraph to another does not render as expected
Comment 1 Myles C. Maxfield 2014-05-06 17:16:59 PDT
Created attachment 230954 [details]
Patch
Comment 2 Myles C. Maxfield 2014-05-06 17:18:08 PDT
<rdar://problem/14500251>
Comment 3 Darin Adler 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?
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Ryosuke Niwa 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.
Comment 7 Myles C. Maxfield 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.
Comment 8 Myles C. Maxfield 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.
Comment 9 Myles C. Maxfield 2014-05-07 17:45:04 PDT
Created attachment 231031 [details]
Patch
Comment 10 Myles C. Maxfield 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
Comment 11 Myles C. Maxfield 2014-05-07 17:48:46 PDT
Created attachment 231032 [details]
Patch
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Myles C. Maxfield 2014-05-07 18:36:32 PDT
Created attachment 231035 [details]
Patch
Comment 15 Ryosuke Niwa 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 {.
Comment 16 Myles C. Maxfield 2014-05-07 19:52:43 PDT
http://trac.webkit.org/changeset/168460
Comment 17 Rob Płóciennik 2014-06-09 00:19:00 PDT
*** Bug 129098 has been marked as a duplicate of this bug. ***