Bug 132633 – Dragging text from one paragraph to another does not render as expected

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Dragging text from one paragraph to another does not render as expected

[Myles C. Maxfield]

Created attachment 230954 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/14500251>

[Darin Adler]

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 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]

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 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]

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 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]

Created attachment 231031 [details]
Patch

[Myles C. Maxfield]

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]

Created attachment 231032 [details]
Patch

[Build Bot]

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]

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]

Created attachment 231035 [details]
Patch

[Ryosuke Niwa]

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]

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

[Rob Płóciennik]

*** Bug 129098 has been marked as a duplicate of this bug. ***