Listen to 'drag' to detect actual start of drag operation.

Listen to 'drag' to detect actual start of drag operation.

When Files.app receives 'dragstart', it cancels file drag operation in some situations.
In these cases, 'dragend' or other drag-related events won't be dispatched.
Missing 'dragend' causes this issue.
'drag' is dispatched only when 'dragstart' is not cancelled, so we can take it as start of user's drag operation.

BUG=326981
TEST=manually confirmed

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273794

[fukino, 2014-05-29 09:16:53]

hirono san, could you take a look?

https://codereview.chromium.org/307863003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/drag_selector.js (right):

https://codereview.chromium.org/307863003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/drag_selector.js:139:
cr.dispatchSimpleEvent(this.target_, 'dragselectionstart');
'drag' won't be dispatched by drag selection operation. (Because native drag has
been already cancelled in 'dragstart')
Drag selector need to notify the start of drag selection operation, so I added
an event which is symmetric to 'dragselectionend'.

[hirono, 2014-05-29 10:00:30]

https://codereview.chromium.org/307863003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/drag_selector.js (right):

https://codereview.chromium.org/307863003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/drag_selector.js:139:
cr.dispatchSimpleEvent(this.target_, 'dragselectionstart');
On 2014/05/29 09:16:53, fukino wrote:
> 'drag' won't be dispatched by drag selection operation. (Because native drag
has
> been already cancelled in 'dragstart')
> Drag selector need to notify the start of drag selection operation, so I added
> an event which is symmetric to 'dragselectionend'.

SGTM.

https://codereview.chromium.org/307863003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/307863003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:1008:
this.table_.list.addEventListener('drag', dragStartBound);
nit: It seems drag events are published repeatedly during a drag operation.
https://developer.mozilla.org/en/docs/DragDrop/Drag_and_Drop#events

It sounds unpredictable that the onDragStart_ handler is called multiple times
before onDragEnd_ is called, though it is not a problem currently.

How about calling stopImmediatePropagation for the dragstart events as well if
we prevents the default action? (To do so, maybe we need to catch dragstart
events on the capturing phase...)

[fukino, 2014-05-29 12:35:17]

Thank you for the comment! I modified the handler name.

https://codereview.chromium.org/307863003/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/file_manager.js (right):

https://codereview.chromium.org/307863003/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/file_manager.js:1008:
this.table_.list.addEventListener('drag', dragStartBound);
On 2014/05/29 10:00:30, hirono wrote:
> nit: It seems drag events are published repeatedly during a drag operation.
> https://developer.mozilla.org/en/docs/DragDrop/Drag_and_Drop#events
> 
> It sounds unpredictable that the onDragStart_ handler is called multiple times
> before onDragEnd_ is called, though it is not a problem currently.
> 
> How about calling stopImmediatePropagation for the dragstart events as well if
> we prevents the default action? (To do so, maybe we need to catch dragstart
> events on the capturing phase...)
> 

In target phase, listeners seem to be invoked in registered order regardless of
specifying useCapture.
So I modify the handler name from onDratStart_ to onDragging_ and add Note to
avoid confusion.

[hirono, 2014-05-30 03:03:26]

lgtm. Thank you!

[fukino, 2014-05-30 03:04:05]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2014-05-30 03:06:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fukino@chromium.org/307863003/10001

[commit-bot: I haz the power, 2014-05-30 07:12:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-30 10:42:45]

Change committed as 273794