Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(362)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2288913002: [InputEvent] Support |deleteByDrag| and |insertFromDrop| (Closed)

Created:
4 years, 3 months ago by chongz
Modified:
4 years, 2 months ago
Reviewers:
yosin_UTC9, dtapuska
CC:
blink-reviews, chromium-reviews, dcheng
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[InputEvent] Support |deleteByDrag| and |insertFromDrop| CLOSED for new CL: https://codereview.chromium.org/2374743002/ * Event order: (See [1] [2]) 1. 'drop' 2. 'beforeinput' InputType=|deleteByDrag| 3. 'beforeinput' InputType=|insertFromDrop| 4. (DOM update) 5. 'input' InputType=|deleteByDrag| 6. 'input' InputType=|insertFromDrop| 7. 'dragend' * Canceling 'beforeinput' will only prevent DOM update, and won't affect other operations. e.g. Cancelling 'deleteByDrag' won't affect 'insertFromDrop' * |dataTransfer| field: 1. NULL for |deleteByDrag| (JS could always get from selection) 2. Readonly |dataTransfer| for |insertFromDrop| For more detailed behavior please refer to LayoutTest. [1] GitHub discussion for event order: https://github.com/w3c/input-events/issues/24 [2] |deleteByDrag| and |insertFromDrop| naming SPEC: https://w3c.github.io/input-events/index.html#h-interface-inputevent-attributes Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/InputEvent/blink-dev/RrnitB0OElc/rirueVekCwAJ BUG=639139

Patch Set 1 #

Total comments: 16

Patch Set 2 : yosin's review, handle removed elements and add tests #

Total comments: 4

Patch Set 3 : Yosin's review 2, check |!isConnected()| #

Total comments: 5

Patch Set 4 : yosin's review 3, check |ownerDocument()| #

Unified diffs Side-by-side diffs Delta from patch set Stats (+377 lines, -48 lines) Patch
M third_party/WebKit/LayoutTests/fast/events/inputevents/beforeinput-remove-iframe-crash.html View 1 chunk +46 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html View 1 1 chunk +190 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/events/resources/beforeinput-remove-iframe-crash-iframe.html View 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/editing/EditingUtilities.cpp View 1 2 3 2 chunks +1 line, -3 lines 0 comments Download
M third_party/WebKit/Source/core/editing/Editor.h View 2 chunks +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/editing/Editor.cpp View 1 2 3 2 chunks +16 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/commands/MoveSelectionCommand.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/events/InputEvent.h View 4 chunks +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/events/InputEvent.cpp View 4 chunks +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/page/DragController.cpp View 1 2 3 chunks +103 lines, -29 lines 0 comments Download

Messages

Total messages: 43 (29 generated)
chongz
yosin@ PTAL at this InputEvent CL for Drag&Drop, thanks! https://codereview.chromium.org/2288913002/diff/20001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2288913002/diff/20001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode113 third_party/WebKit/Source/core/editing/Editor.cpp:113: ...
4 years, 3 months ago (2016-08-30 23:59:21 UTC) #14
yosin_UTC9
https://codereview.chromium.org/2288913002/diff/20001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2288913002/diff/20001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode118 third_party/WebKit/Source/core/editing/Editor.cpp:118: dispatchInputEvent(endRoot, InputEvent::InputType::InsertFromDrop, data, isComposing); Q: Do we dispatch "insertFromDrop" ...
4 years, 3 months ago (2016-08-31 01:20:34 UTC) #15
chongz
yosin@ sorry for the delay. I've added check for removed elements and updated layout tests, ...
4 years, 3 months ago (2016-09-02 18:02:10 UTC) #20
yosin_UTC9
https://codereview.chromium.org/2288913002/diff/20001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2288913002/diff/20001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode118 third_party/WebKit/Source/core/editing/Editor.cpp:118: dispatchInputEvent(endRoot, InputEvent::InputType::InsertFromDrop, data, isComposing); On 2016/09/02 at 18:02:10, chongz ...
4 years, 3 months ago (2016-09-05 01:37:50 UTC) #21
chongz
yosin@ I've added check for |!isConnected()|, PTAL again, thanks! https://codereview.chromium.org/2288913002/diff/20001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2288913002/diff/20001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode118 third_party/WebKit/Source/core/editing/Editor.cpp:118: ...
4 years, 3 months ago (2016-09-07 17:12:56 UTC) #26
yosin_UTC9
lgtm Thanks!
4 years, 3 months ago (2016-09-08 04:23:53 UTC) #27
yosin_UTC9
Oops, one more thing. https://codereview.chromium.org/2288913002/diff/60001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2288913002/diff/60001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode105 third_party/WebKit/Source/core/editing/Editor.cpp:105: if (!target || !target->isConnected()) One ...
4 years, 3 months ago (2016-09-08 04:34:40 UTC) #28
chongz
yosin@ I've updated CL but don't really understand why, PTAL again, thanks! https://codereview.chromium.org/2288913002/diff/60001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp ...
4 years, 3 months ago (2016-09-08 22:37:50 UTC) #31
chongz
On 2016/09/08 22:37:50, chongz wrote: > yosin@ I've updated CL but don't really understand why, ...
4 years, 3 months ago (2016-09-12 21:34:53 UTC) #34
chongz
dtapuska@ can you take a look at the "|target->ownerDocument() == originateDocument|" issue please? Thanks! (Seems ...
4 years, 3 months ago (2016-09-13 17:17:12 UTC) #37
dtapuska
https://codereview.chromium.org/2288913002/diff/60001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2288913002/diff/60001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode105 third_party/WebKit/Source/core/editing/Editor.cpp:105: if (!target || !target->isConnected()) On 2016/09/13 17:17:12, chongz wrote: ...
4 years, 3 months ago (2016-09-13 17:27:50 UTC) #38
yosin_UTC9
https://codereview.chromium.org/2288913002/diff/60001/third_party/WebKit/Source/core/editing/Editor.cpp File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2288913002/diff/60001/third_party/WebKit/Source/core/editing/Editor.cpp#newcode105 third_party/WebKit/Source/core/editing/Editor.cpp:105: if (!target || !target->isConnected()) On 2016/09/13 at 17:27:50, dtapuska ...
4 years, 3 months ago (2016-09-14 01:57:37 UTC) #39
yosin_UTC9
I could not find the resolution of event order of beforeinput/input for drag-and-drop in [1] ...
4 years, 3 months ago (2016-09-14 02:06:23 UTC) #40
chongz
4 years, 3 months ago (2016-09-15 22:42:25 UTC) #41 
On 2016/09/14 02:06:23, Yosi_UTC9 wrote:
> I could not find the resolution of event order of beforeinput/input for
> drag-and-drop in [1] and [2]. Could you point out?
> 
> It seems proposed event order makes our life harder:
>   1. 'drop'
>   2. 'beforeinput' InputType=|deleteByDrag|
>   3. 'beforeinput' InputType=|insertFromDrop|
>   4. (DOM update) 
> UA may choose one of following to update DOM:
> - Delete source then replace target by source
> - Replace target by source then delete source.
>   5. 'input' InputType=|deleteByDrag|
>   6. 'input' InputType=|insertFromDrop|
>   7. 'dragend'
> 
> IMO, beforeinput and input are pair and should not be nested.
> 
> I prefer following order:
>   1. 'drop'
>   2. 'beforeinput' InputType=|deleteByDrag|
>   3. Remove source
>   4. 'input' InputType=|deleteByDrag|
>   5. 'beforeinput' InputType=|insertFromDrop|
>   6. Replace target by source
>   7. 'input' InputType=|insertFromDrop|
>   8. 'dragend'

Sorry for the confusion. We only have resolution for the order between
InputEvent and Drag&Drop events, but no resolution for the order between
'deleteByDrag' and 'insertFromDrop'.
(Just filed https://github.com/w3c/input-events/issues/24)

I agree that un-nested events are better, but I'm not sure what's the best way
to implement it.

I can come up with 2 possible solutions:

1. Removed |MoveSelectionCommand| and split it into |DeleteSelectionCommand| and
|ReplaceSelectionCommand|
Pros:
  * Cleaner code, 'input' event handling goes into "Editor.cpp"
Cons:
  * Requires 2 undo to undo drag-and-drop
  * Or should we do something similar to |TypingCommand| to merge these 2 undo?

2. Still use |MoveSelectionCommand|, but inject 'beforeinput'/'input' inside
|MoveSelectionCommand::doApply()|
Pros:
  * Requires 1 undo (existing behavior)
Cons:
  * Confusing code, we don't usually dispatch events in |doApply()|

Powered by Google App Engine
This is Rietveld 408576698