Open Bug 1414204 Opened 2 years ago Updated 2 years ago

Suppress input events when there is a dnd session

Categories

(Core :: DOM: Events, defect, P3)

defect

Tracking

()

REOPENED
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- affected

People

(Reporter: stone, Assigned: stone)

References

Details

Attachments

(5 files, 2 obsolete files)

No description provided.
Assignee: nobody → sshih
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
Priority: -- → P3
(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.
Early return in PresShell::HandleEvent could break some test cases. Revise the patch to suppress input events in TabChild
Attachment #8925435 - Attachment is obsolete: true
Hmm, we should suppress input events in PresShell so that the mousemove is suppressed when starting a dnd operation with a derived pointermove event.
Attachment #8926766 - Attachment is obsolete: true
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.
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.
Attachment #8926768 - Flags: review?(bugs)
Attachment #8927240 - Flags: review?(bugs)
Attachment #8927243 - Flags: review?(bugs)
Attachment #8927243 - Flags: review?(bugs) → review+
Attachment #8926768 - Flags: review?(bugs) → review+
(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 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?
Attachment #8927240 - Flags: review?(bugs) → review+
(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.
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
Attachment #8935229 - Flags: review?(bugs)
Attachment #8935229 - Flags: review?(bugs) → review?(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.
Attachment #8935229 - Flags: review?(jmathies)
Attachment #8935229 - Flags: review?(bdahl)
Attachment #8935229 - Flags: feedback+
Attachment #8935229 - Flags: review?(jmathies) → review+
Pushed by sshih@mozilla.com:
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.
Depends on: 1427461
I guess we should back this out for now to fix regressions.
Will try to do that now.
I agree, let's back this out please.
Tree is closed atm. I have the backout commits waiting to be pushed...
Perhaps we'll find the issue quickly and can re-land the patches even for 59.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Target Milestone: mozilla59 → ---
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.
Attachment #8940078 - Flags: review?(bugs)
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
Attachment #8940078 - Flags: review?(bugs) → review+
You need to log in before you can comment on or make changes to this bug.