Bug 163948 – Support "insertFromDrop" and "deleteByDrag" inputTypes for the InputEvent spec

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Wenson Hsieh]

Support "insertFromDrop" and "deleteByDrag" for the InputEvent spec

[Wenson Hsieh]

<rdar://problem/28921433>

[Wenson Hsieh]

Created attachment 292764 [details]
WIP (includes 163213)

[Build Bot]

Comment on attachment 292764 [details]
WIP (includes 163213)

Attachment 292764 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2368758

New failing tests:
editing/pasteboard/subframe-dragndrop-1.html
editing/pasteboard/drag-drop-modifies-page.html
editing/pasteboard/smart-drag-drop.html

[Build Bot]

Created attachment 292775 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292764 [details]
WIP (includes 163213)

Attachment 292764 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2368710

New failing tests:
editing/pasteboard/subframe-dragndrop-1.html
editing/pasteboard/drag-drop-modifies-page.html
editing/pasteboard/smart-drag-drop.html

[Build Bot]

Created attachment 292777 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Wenson Hsieh]

Created attachment 292830 [details]
First pass

[Ryosuke Niwa]

Comment on attachment 292830 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=292830&action=review

> Source/WebCore/editing/CompositeEditCommand.h:109
> -    EditCommandComposition* composition() { return m_composition.get(); }
> +    EditCommandComposition* topLevelComposition() const;
> +    EditCommandComposition* composition() const { return m_composition.get(); }

This doesn't make any sense. EditCommandComposition should always be created by the top-level CompositeEditCommand.

[Wenson Hsieh]

Comment on attachment 292830 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=292830&action=review

>> Source/WebCore/editing/CompositeEditCommand.h:109
>> +    EditCommandComposition* composition() const { return m_composition.get(); }
> 
> This doesn't make any sense. EditCommandComposition should always be created by the top-level CompositeEditCommand.

topLevelComposition() fetches the composition of the top-level command. m_composition will still be 0 for a CompositeEditCommand that is not at the top level.

[Ryosuke Niwa]

Comment on attachment 292830 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=292830&action=review

> Source/WebCore/editing/Editor.cpp:1087
> +    if (command.defersInputEventsToChildCommands())
> +        return true;

I think it's more intuitive to have shouldFireInputEvent and make it return true iff it's the top-level command, and then override it in MoveSelectionCommand.

> LayoutTests/fast/events/before-input-events-prevent-drag-and-drop.html:48
> +        moveToCenter(source);
> +        eventSender.leapForward(500);
> +        eventSender.mouseDown();
> +        eventSender.leapForward(1000);
> +        moveToCenter(destination);
> +        eventSender.leapForward(500);
> +        eventSender.mouseUp();

You should wrap this code inside if (window.eventSender).

> LayoutTests/fast/events/before-input-events-prevent-drag-and-drop.html:53
> +                copy.innerHTML = event.dataTransfer.getData("text/html");

Surely, you meant textContent. Otherwise, any markup you've included in the html won't show in the output.

> LayoutTests/fast/events/input-events-drag-and-drop.html:20
> +    <script src="../../resources/js-test-pre.js"></script>

It really doesn't much sense this test to be including js-test-pre.js given it doesn't use should* function at all.
Alternatively, you can log a list of events that got fired, and assert that those events fired in the order
but that's a bit tricky to make it work both using eventSender and during manual testing.

> LayoutTests/fast/events/input-events-drag-and-drop.html:29
> +        description("To manually test this, drag the text in the first input into the second, then undo (cmd-z) and observe the output.");

You should describe what output is expected.

> LayoutTests/fast/events/input-events-drag-and-drop.html:35
> +            debug("* * * Performing drag and drop * * *");

Instead of using * to separate, why don't we just have an empty debug('') to insert a new line?

> LayoutTests/fast/events/input-events-drag-and-drop.html:51
> +        function handleInput(event)

This is more like logInputEvent.

> LayoutTests/fast/events/input-events-insert-by-drop.html:4
> +    <script src="../../resources/js-test-pre.js"></script>

It really doesn't much sense this test to be including js-test-pre.js given it doesn't use should* function at all.

> LayoutTests/fast/events/input-events-insert-by-drop.html:6
> +<style>

Putting a style element between head & body is strange.  Just put it inside body.

[Build Bot]

Comment on attachment 292830 [details]
First pass

Attachment 292830 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2370554

New failing tests:
fast/forms/drag-into-textarea.html
fast/forms/drag-out-of-textarea.html

[Build Bot]

Created attachment 292840 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292830 [details]
First pass

Attachment 292830 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2370755

New failing tests:
fast/forms/drag-into-textarea.html
fast/forms/drag-out-of-textarea.html

[Build Bot]

Created attachment 292844 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Wenson Hsieh]

Comment on attachment 292830 [details]
First pass

View in context: https://bugs.webkit.org/attachment.cgi?id=292830&action=review

>> Source/WebCore/editing/Editor.cpp:1087
>> +        return true;
> 
> I think it's more intuitive to have shouldFireInputEvent and make it return true iff it's the top-level command, and then override it in MoveSelectionCommand.

I like this idea! Done.

>> LayoutTests/fast/events/before-input-events-prevent-drag-and-drop.html:48
>> +        eventSender.mouseUp();
> 
> You should wrap this code inside if (window.eventSender).

Good catch -- done.

>> LayoutTests/fast/events/before-input-events-prevent-drag-and-drop.html:53
>> +                copy.innerHTML = event.dataTransfer.getData("text/html");
> 
> Surely, you meant textContent. Otherwise, any markup you've included in the html won't show in the output.

The original intent was actually to make `copy` mirror the contents of the first content editable, but on second thought, setting textContent to event.dataTransfer.getData("text/html") is a better idea. Changed.

>> LayoutTests/fast/events/input-events-drag-and-drop.html:20
>> +    <script src="../../resources/js-test-pre.js"></script>
> 
> It really doesn't much sense this test to be including js-test-pre.js given it doesn't use should* function at all.
> Alternatively, you can log a list of events that got fired, and assert that those events fired in the order
> but that's a bit tricky to make it work both using eventSender and during manual testing.

This is a good point. Really the only thing I use them for here is debug(). I'll remove the js-test-* script includes.

>> LayoutTests/fast/events/input-events-drag-and-drop.html:29
>> +        description("To manually test this, drag the text in the first input into the second, then undo (cmd-z) and observe the output.");
> 
> You should describe what output is expected.

Added more detail to the descriptions in all 3 tests.

>> LayoutTests/fast/events/input-events-drag-and-drop.html:35
>> +            debug("* * * Performing drag and drop * * *");
> 
> Instead of using * to separate, why don't we just have an empty debug('') to insert a new line?

Done.

>> LayoutTests/fast/events/input-events-drag-and-drop.html:51
>> +        function handleInput(event)
> 
> This is more like logInputEvent.

Renamed.

>> LayoutTests/fast/events/input-events-insert-by-drop.html:4
>> +    <script src="../../resources/js-test-pre.js"></script>
> 
> It really doesn't much sense this test to be including js-test-pre.js given it doesn't use should* function at all.

I agree -- removed the js-test-* includes.

>> LayoutTests/fast/events/input-events-insert-by-drop.html:6
>> +<style>
> 
> Putting a style element between head & body is strange.  Just put it inside body.

Oh, good catch! Fixed.

[Wenson Hsieh]

Created attachment 292861 [details]
Fix unit tests and address feedback

[Build Bot]

Comment on attachment 292861 [details]
Fix unit tests and address feedback

Attachment 292861 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2371635

New failing tests:
fast/events/focus-change-crash2.html
fast/forms/focus-change-on-keypress.html

[Build Bot]

Created attachment 292865 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Wenson Hsieh]

Created attachment 292868 [details]
Fix bad assertion

[Darin Adler]

Comment on attachment 292868 [details]
Fix bad assertion

View in context: https://bugs.webkit.org/attachment.cgi?id=292868&action=review

> Source/WebCore/editing/CompositeEditCommand.cpp:416
> +    auto* command = this;
> +    while (command->parent()) {
> +        ASSERT(!command->m_composition);
> +        command = command->parent();
> +    }
> +    return command->m_composition.get();

This loop could be written in a more straightforward manner. I would suggest this version:

    for (auto* command = this; command; command = command->parent())
        if (auto* composition = command->m_composition) {
            ASSERT(!command->parent());
            return composition;
        }
    }
    return nullptr;

But I can also imagine other versions that are closer to what you wrote, but without evaluating command->parent() twice.

> Source/WebCore/editing/EditCommand.cpp:183
> +        if (EditCommandComposition* composition = compositionIfPossible(cmd))

I suggest auto* here.

> Source/WebCore/editing/EditCommand.cpp:194
> +        if (EditCommandComposition* composition = compositionIfPossible(cmd))

I suggest auto* here.

[Wenson Hsieh]

Comment on attachment 292868 [details]
Fix bad assertion

View in context: https://bugs.webkit.org/attachment.cgi?id=292868&action=review

>> Source/WebCore/editing/CompositeEditCommand.cpp:416
>> +    return command->m_composition.get();
> 
> This loop could be written in a more straightforward manner. I would suggest this version:
> 
>     for (auto* command = this; command; command = command->parent())
>         if (auto* composition = command->m_composition) {
>             ASSERT(!command->parent());
>             return composition;
>         }
>     }
>     return nullptr;
> 
> But I can also imagine other versions that are closer to what you wrote, but without evaluating command->parent() twice.

Sounds good! Done.

>> Source/WebCore/editing/EditCommand.cpp:183
>> +        if (EditCommandComposition* composition = compositionIfPossible(cmd))
> 
> I suggest auto* here.

Done.

>> Source/WebCore/editing/EditCommand.cpp:194
>> +        if (EditCommandComposition* composition = compositionIfPossible(cmd))
> 
> I suggest auto* here.

Done.

[Wenson Hsieh]

Created attachment 293067 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 293067 [details]
Patch for landing

Clearing flags on attachment: 293067

Committed r208014: <http://trac.webkit.org/changeset/208014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.