WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
163948
Support "insertFromDrop" and "deleteByDrag" inputTypes for the InputEvent spec
https://bugs.webkit.org/show_bug.cgi?id=163948
Summary
Support "insertFromDrop" and "deleteByDrag" inputTypes for the InputEvent spec
Wenson Hsieh
Reported
2016-10-25 09:32:22 PDT
Support "insertFromDrop" and "deleteByDrag" for the InputEvent spec
Attachments
WIP (includes 163213)
(89.63 KB, patch)
2016-10-25 09:41 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews103 for mac-yosemite
(1.48 MB, application/zip)
2016-10-25 10:55 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews112 for mac-yosemite
(2.05 MB, application/zip)
2016-10-25 11:01 PDT
,
Build Bot
no flags
Details
First pass
(38.95 KB, patch)
2016-10-25 15:05 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews117 for mac-yosemite
(1.83 MB, application/zip)
2016-10-25 16:05 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews101 for mac-yosemite
(902.55 KB, application/zip)
2016-10-25 16:33 PDT
,
Build Bot
no flags
Details
Fix unit tests and address feedback
(41.07 KB, patch)
2016-10-25 18:33 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews114 for mac-yosemite
(1.73 MB, application/zip)
2016-10-25 19:46 PDT
,
Build Bot
no flags
Details
Fix bad assertion
(41.02 KB, patch)
2016-10-25 19:58 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch for landing
(40.85 KB, patch)
2016-10-27 15:13 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2016-10-25 09:34:30 PDT
<
rdar://problem/28921433
>
Wenson Hsieh
Comment 2
2016-10-25 09:41:17 PDT
Created
attachment 292764
[details]
WIP (includes 163213)
Build Bot
Comment 3
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
Build Bot
Comment 4
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
Build Bot
Comment 5
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
Build Bot
Comment 6
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
Wenson Hsieh
Comment 7
2016-10-25 15:05:36 PDT
Created
attachment 292830
[details]
First pass
Ryosuke Niwa
Comment 8
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.
Wenson Hsieh
Comment 9
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.
Ryosuke Niwa
Comment 10
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.
Build Bot
Comment 11
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
Build Bot
Comment 12
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
Build Bot
Comment 13
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
Build Bot
Comment 14
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
Wenson Hsieh
Comment 15
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.
Wenson Hsieh
Comment 16
2016-10-25 18:33:51 PDT
Created
attachment 292861
[details]
Fix unit tests and address feedback
Build Bot
Comment 17
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
Build Bot
Comment 18
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
Wenson Hsieh
Comment 19
2016-10-25 19:58:15 PDT
Created
attachment 292868
[details]
Fix bad assertion
Darin Adler
Comment 20
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.
Wenson Hsieh
Comment 21
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.
Wenson Hsieh
Comment 22
2016-10-27 15:13:47 PDT
Created
attachment 293067
[details]
Patch for landing
WebKit Commit Bot
Comment 23
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
>
WebKit Commit Bot
Comment 24
2016-10-27 15:49:44 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug