Bug 163948

Summary: Support "insertFromDrop" and "deleteByDrag" inputTypes for the InputEvent spec
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: UI EventsAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 163213    
Bug Blocks: 163112    
Attachments:
Description Flags
WIP (includes 163213)
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews112 for mac-yosemite
none
First pass
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Fix unit tests and address feedback
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Fix bad assertion
none
Patch for landing none

Description Wenson Hsieh 2016-10-25 09:32:22 PDT
Support "insertFromDrop" and "deleteByDrag" for the InputEvent spec
Comment 1 Wenson Hsieh 2016-10-25 09:34:30 PDT
<rdar://problem/28921433>
Comment 2 Wenson Hsieh 2016-10-25 09:41:17 PDT
Created attachment 292764 [details]
WIP (includes 163213)
Comment 3 Build Bot 2016-10-25 10:55:50 PDT
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
Comment 4 Build Bot 2016-10-25 10:55:53 PDT
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
Comment 5 Build Bot 2016-10-25 11:01:01 PDT
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
Comment 6 Build Bot 2016-10-25 11:01:04 PDT
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
Comment 7 Wenson Hsieh 2016-10-25 15:05:36 PDT
Created attachment 292830 [details]
First pass
Comment 8 Ryosuke Niwa 2016-10-25 15:39:07 PDT
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.
Comment 9 Wenson Hsieh 2016-10-25 15:41:29 PDT
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.
Comment 10 Ryosuke Niwa 2016-10-25 15:50:36 PDT
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.
Comment 11 Build Bot 2016-10-25 16:05:14 PDT
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
Comment 12 Build Bot 2016-10-25 16:05:16 PDT
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
Comment 13 Build Bot 2016-10-25 16:33:51 PDT
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
Comment 14 Build Bot 2016-10-25 16:33:56 PDT
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
Comment 15 Wenson Hsieh 2016-10-25 18:17:07 PDT
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.
Comment 16 Wenson Hsieh 2016-10-25 18:33:51 PDT
Created attachment 292861 [details]
Fix unit tests and address feedback
Comment 17 Build Bot 2016-10-25 19:46:01 PDT
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
Comment 18 Build Bot 2016-10-25 19:46:03 PDT
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
Comment 19 Wenson Hsieh 2016-10-25 19:58:15 PDT
Created attachment 292868 [details]
Fix bad assertion
Comment 20 Darin Adler 2016-10-27 14:37:14 PDT
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.
Comment 21 Wenson Hsieh 2016-10-27 14:54:10 PDT
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.
Comment 22 Wenson Hsieh 2016-10-27 15:13:47 PDT
Created attachment 293067 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2016-10-27 15:49:38 PDT
Comment on attachment 293067 [details]
Patch for landing

Clearing flags on attachment: 293067

Committed r208014: <http://trac.webkit.org/changeset/208014>
Comment 24 WebKit Commit Bot 2016-10-27 15:49:44 PDT
All reviewed patches have been landed.  Closing bug.