1414204 - Suppress input events when there is a dnd session

Status: REOPENED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Comment 1

[Ming-Chou Shih [:stone]]

Spec [1] says "From the moment that the user agent is to initiate the drag-and-drop operation, until the end of the drag-and-drop operation, device input events (e.g., mouse and keyboard events) must be suppressed."

[1] http://w3c.github.io/html/editing.html#drag-and-drop-processing-model

Comment 2

[Ming-Chou Shih [:stone]]

Attachment: Suppress input events when there is a dnd session

Comment 3

[Ming-Chou Shih [:stone]]

(In reply to Ming-Chou Shih [:stone] from comment #2)
> Created attachment 8925435 [details] [diff] [review]
> Suppress input events when there is a dnd session

Early return in PresShell::HandleEvent and after dispatching pointerevent. That's because we may start a dnd operation with a pointermove event and we also want to suppress the mousemove event.

Comment 4

[Ming-Chou Shih [:stone]]

Attachment: Part1: Suppress input events when there is a dnd session

Early return in PresShell::HandleEvent could break some test cases. Revise the patch to suppress input events in TabChild

Comment 5

[Ming-Chou Shih [:stone]]

Attachment: Part2: Tweak the API to synthesize dnd operations

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Part1: Suppress input events when there is a dnd session

Hmm, we should suppress input events in PresShell so that the mousemove is suppressed when starting a dnd operation with a derived pointermove event.

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Part3: Revise test_bug470212.html

This test case is broken by the part1 patch. That's because the anchor element is drag but not canvas element. Shift the position of mousedown event a little bit so that the canvas element is drag.

Comment 8

[Ming-Chou Shih [:stone]]

There are three patches in this bug
Part1:
There may be some pending input events in the queue of thread when content starts a dnd operation. Spec says that input events should be suppressed when there is a dnd operation. Add a flag in ESM and turn on/off when start/finish a dnd operation. Checking the flag in PresShell::HandleEvent because we may start a dnd operation with pointermove and we want to suppress the mousemove as well.

Part2:
synthesizeDrop synthesizes a dnd operation by starting a drag session then firing mouse events. Tweak the API to follow the order of real use cases.

Part3:
test_bug470212.html is broken because the anchor element is drag but not canvas element. Shift the position of mousedown event a little bit to fix it.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Ming-Chou Shih [:stone] from comment #1)
> Spec [1] says "From the moment that the user agent is to initiate the
> drag-and-drop operation, until the end of the drag-and-drop operation,
> device input events (e.g., mouse and keyboard events) must be suppressed."
> 
> [1] http://w3c.github.io/html/editing.html#drag-and-drop-processing-model

FWIW, that is a link to a spec we don't try to implement, but
https://html.spec.whatwg.org/#initiate-the-drag-and-drop-operation has the same text.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8927240 [details] [diff] [review]
Part1: Suppress input events when there is a dnd session

>+  if (EventStateManager::IsInputEventsSuppressed() &&
>+      (aEvent->mClass == eMouseEventClass ||
>+       aEvent->mClass == eWheelEventClass ||
>+       aEvent->mClass == ePointerEventClass ||
>+       aEvent->mClass == eTouchEventClass ||
>+       aEvent->mClass == eKeyboardEventClass)) {
Should also WidgetCompositionEvents be suppressed? I'd guess so.

How are ctrl keys during dnd handled with this?
Currently one can control the operation (move or copy) by using ctrl key (at least on linux)
(and looks like that is a bit buggy on Chrome, but FF works rather well)
Do you know why keyevents are suppressed?
It is odd if ctrl key gets keyup after dnd, but there hasn't been keydown.
I asked annevk and Domenic on #whatwg if they happen to know the reasoning.
https://freenode.logbot.info/whatwg/20171112#c1368409
But looks like we also block the keydown for ctrl already somehow.
And I guess esc key has somewhat similar special behavior.

>+nsDragServiceProxy::StartDragSession()
>+{
>+  // Normally, OS stops firing input events when a drag operation starts. But
>+  // there may be some pending input events queued in the content process. We
>+  // have to suppress them since spec says that input events must be suppressed
>+  // when there is a dnd session.
>+  EventStateManager::SuppressInputEvents();
>+  return nsBaseDragService::StartDragSession();
>+}
Ahaa, so this relies on the fact that the pending input events are dispatched before dnd ends. That is guaranteed, right?

Comment 11

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8927240 [details] [diff] [review]
> Part1: Suppress input events when there is a dnd session
> 
> >+  if (EventStateManager::IsInputEventsSuppressed() &&
> >+      (aEvent->mClass == eMouseEventClass ||
> >+       aEvent->mClass == eWheelEventClass ||
> >+       aEvent->mClass == ePointerEventClass ||
> >+       aEvent->mClass == eTouchEventClass ||
> >+       aEvent->mClass == eKeyboardEventClass)) {
> Should also WidgetCompositionEvents be suppressed? I'd guess so.
> 
> How are ctrl keys during dnd handled with this?
> Currently one can control the operation (move or copy) by using ctrl key (at
> least on linux)
> (and looks like that is a bit buggy on Chrome, but FF works rather well)
> Do you know why keyevents are suppressed?
Sorry for replying late. I guess that's because OS (at least Windows and Linux) stops firing keyboard events to applications when there is a dnd operation.

> It is odd if ctrl key gets keyup after dnd, but there hasn't been keydown.
OS continues firing keyboard events when finishing the dnd operation. So we will receive keydown if the dnd operation is finished and the user still holds on a key.

> I asked annevk and Domenic on #whatwg if they happen to know the reasoning.
> https://freenode.logbot.info/whatwg/20171112#c1368409
> But looks like we also block the keydown for ctrl already somehow.
> And I guess esc key has somewhat similar special behavior.
OS handles esc key and end the dnd operation. OS only firs keyup to the application.

> >+nsDragServiceProxy::StartDragSession()
> >+{
> >+  // Normally, OS stops firing input events when a drag operation starts. But
> >+  // there may be some pending input events queued in the content process. We
> >+  // have to suppress them since spec says that input events must be suppressed
> >+  // when there is a dnd session.
> >+  EventStateManager::SuppressInputEvents();
> >+  return nsBaseDragService::StartDragSession();
> >+}
> Ahaa, so this relies on the fact that the pending input events are
> dispatched before dnd ends. That is guaranteed, right?
If I understand the question correctly, yes, we handle the pending input events and the IPC message to stop the dnd operation by the order of time.

Comment 12

[Ming-Chou Shih [:stone]]

Got a failure when running mochitest in headless mode. It's happened after fixing the problem of test_bug470212.html. It's broken by a synthetic mousemove event. nsPluginInstanceOwner invoked GDK_ROOT_WINDOW and then got a 'Segmentation fault' in [1].

[1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/plugins/base/nsPluginInstanceOwner.cpp#2303

Comment 13

[Ming-Chou Shih [:stone]]

Attachment: Part4: Check headless mode before calling GDK_ROOT_WINDOW

Comment 14

[Brendan Dahl [:bdahl]]

Comment on attachment 8935229 [details] [diff] [review]
Part4: Check headless mode before calling GDK_ROOT_WINDOW

Review of attachment 8935229 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable from the headless side of things. I'm not a plugin peer, so I'll forward the review.

Comment 15

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96f2f419f59ee7105d7df46029ae25e535debd61

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61e28d94b9bd
Part1: Suppress input events when there is a dnd session. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e98360fb30
Part2: Tweak the API to synthesize dnd operations. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a84069a023b3
Part3: Revise test_bug470212.html to drag the correct element. r=smaug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d7a9a3500ca
Part4: Check headless mode before calling GDK_ROOT_WINDOW. f=bdahl. r=jimm.

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/61e28d94b9bd
https://hg.mozilla.org/mozilla-central/rev/e0e98360fb30
https://hg.mozilla.org/mozilla-central/rev/a84069a023b3
https://hg.mozilla.org/mozilla-central/rev/8d7a9a3500ca

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I guess we should back this out for now to fix regressions.
Will try to do that now.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

I agree, let's back this out please.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Tree is closed atm. I have the backout commits waiting to be pushed...

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f68860d4dff6
backout because of regressions, r=backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab31d78bc21
backout because of regressions, r=backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee98f9838ef
backout because of regressions, r=backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/a53f97560b8f
backout because of regressions, r=backout

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Perhaps we'll find the issue quickly and can re-land the patches even for 59.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/f68860d4dff6
https://hg.mozilla.org/mozilla-central/rev/3ab31d78bc21
https://hg.mozilla.org/mozilla-central/rev/4ee98f9838ef
https://hg.mozilla.org/mozilla-central/rev/a53f97560b8f

This was merged to m-c in time for the second round of nightlies today, so people should be getting the builds already.

Comment 24

[Ming-Chou Shih [:stone]]

Attachment: Part5: Handle the corner cases that TabParent fails to start a dnd session

Comment 25

[Ming-Chou Shih [:stone]]

Comment on attachment 8940078 [details] [diff] [review]
Part5: Handle the corner cases that TabParent fails to start a dnd session

There are cases that we should stop the drag session on the remote.
1. Receives the request to invoke a drag session while mouseup / touchend is handled.
2. Fails to start a native drag session.
3. Gets mouseup or touchend after starting a drag session.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8940078 [details] [diff] [review]
Part5: Handle the corner cases that TabParent fails to start a dnd session

>+  // If we get a mouseup or touchend event while tracking the drag gesture for a
>+  // remote target, we have to call EndDragSession to stop the drag session in
>+  // the content process. This happens when content starts a drag session and we
>+  // are waiting for the next move event to start the OS native drag operation.
... but we get mouseup or touchend instead.

Something like that could clarify the comment a bit
>+  // End the drag session when TabChild starts a drag session while TabParent
>+  // already handled a mouseup or touchend event.
>+  if ((!shell && Manager()->IsContentParent()) ||
>+      (esm && !esm->CanRemoteBeginTrackingDragGesture())) {
Manager()->IsContentParent() check is odd, since in both cases we still access AsContentParent().
so, perhaps have
if (Manager()->IsContentParent() && (!shell || !esm || !esm->CanRemoteBeginTrackingDragGesture())) ...

> NS_IMETHODIMP
> nsBaseDragService::EndDragSession(bool aDoneDrag, uint32_t aKeyModifiers)
> {
>+  // Always propagate to child processes since they may already start a drag
since they may have already started

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.