Suppressed MEs for gestures from cancelled PEs

third_party/WebKit/Source/core/input/PointerEventManager.cpp

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/input/PointerEventManager.h"
 
 #include "core/dom/ElementTraversal.h"
 #include "core/dom/shadow/FlatTreeTraversal.h"
 #include "core/events/MouseEvent.h"
 #include "core/frame/FrameView.h"
@@ skipping 436 matching lines @@
             }
         } else {
             // Set the target of pointer event to the captured node as this
             // pointer is captured otherwise it would have gone to the if block
             // and perform a hit-test.
             touchInfo.touchNode = m_pendingPointerCaptureTarget
                 .get(pointerId)->toNode();
             touchInfo.targetFrame = touchInfo.touchNode->document().frame();
         }
 
-        WebInputEventResult result = WebInputEventResult::NotHandled;
-
         // Do not send pointer events for stationary touches or null targetFrame
         if (touchInfo.touchNode
             && touchPoint.state() != PlatformTouchPoint::TouchStationary
             && !m_inCanceledStateForPointerTypeTouch) {
             FloatPoint pagePoint = touchInfo.targetFrame->view()
                 ->rootFrameToContents(touchInfo.point.pos());
             float scaleFactor = 1.0f / touchInfo.targetFrame->pageZoomFactor();
             FloatPoint scrollPosition = touchInfo.targetFrame->view()->scrollPosition();
             FloatPoint framePoint = pagePoint.scaledBy(scaleFactor);
             framePoint.moveBy(scrollPosition.scaledBy(-scaleFactor));
             PointerEvent* pointerEvent = m_pointerEventFactory.create(
                 pointerEventNameForTouchPointState(touchPoint.state()),
                 touchPoint, event.getModifiers(),
                 touchPoint.radius().scaledBy(scaleFactor),
                 framePoint,
                 touchInfo.touchNode ?
                     touchInfo.touchNode->document().domWindow() : nullptr);
 
-            result = sendTouchPointerEvent(touchInfo.touchNode, pointerEvent);
+            WebInputEventResult result = sendTouchPointerEvent(touchInfo.touchNode, pointerEvent);
+
+            // Queue the WebInputEventResult to allow suppressing mouse events
+            // from gesture events. Since mouse events are fired only from
+            // GestureTap and GestureLongPress events which are triggered by
+            // single touches only, it is enough to queue the

[Rick Byers, 2016/05/27 20:39:27]

Not quite true - GestureTwoFingerTap behaves similarly (but not identically) to
LongPress on desktop IIRC.

[mustaq, 2016/05/30 15:14:26]

I will update the comment to clarify it: TwoFingerTaps also send MEs but since
they fire no touchstarts so we should be fine here.

We should perhaps fire PEs for TwoFingerTaps? I will file a bug if this may be
the case.

[mustaq, 2016/06/02 19:26:27]

Clarified the comment. My last post seems wrong about touchstart :(

+            // WebInputEventResult for only primary pointerdowns.
+            if (pointerEvent->type() == EventTypeNames::pointerdown
+                && pointerEvent->isPrimary()) {
+                m_primaryPointerdownResults.append(result);

[Rick Byers, 2016/05/27 20:39:27]

Unfortunately I don't think we can currently guarantee a 1:1 correspondence
between touch and gesture events.  Eg:
 - If the page has no touch/pointer listeners at all, blink will never get the
WebTouchEvents (and so I think your take_first will fail because the queue is
empty).  Similarly, can CC touch hit testing still cause touch events to be
hidden completely from blink or did Dave change that?
 - The page may change between when the touch event was received and when the
gesture event was received so that they go to different frames.  Moving to a
page-wide PointerEventManager should fix this except for the OOPIF scenario

One option is to make the code more robust but still a heuristic (eg. identify
failure cases - unexpected empty queue, timeout with something left in the queue
a long time), do something reasonable but log an UMA counter so we know how
often these edge cases are happening (to decide if it's worth fixing).

Another option would be to include some sort of identifier in both the touch and
gesture events to correlate them (and then use a map, but still need some expiry
mechanism).

Or perhaps we just need to flow the information back with the touch ACK and
bring it back with the gesture event.  We discussed this when considering the
"TouchEvent.preventMouseEvents()" API and I think this was our preferred choice
at the time.  Not sure how hard it is though.

[dtapuska, 2016/05/27 20:54:07]

I'd prefer if the information went up the stack and then flowed back down as
opposed to this side channel of data. Side channels can get out of sync. Is the
tap targeted at the same iframe if something moved between the events?

[mustaq, 2016/05/30 15:14:26]

Local sync seems unavoidable. I had uniqueTouchIds plumbed all the way down to
Blink (see Patch#2) but because of brittleness in ID handling in GR (missing ids
on timer-fired GEs, GestureTapUnconfirmed breaking tap ids etc), I had to rely
on TE-vs-GE bracketing behavior to sync the outcome in Blink again.

Flowing the info back to GR was my first attempt but I had to rule out the
option because:
(1) Touch acks are not even sent back for passive listeners, so we can't piggy
back.
(2) Sending an IPC back would make it prone to race conditions: what if the IPC
reaches GR when the GR is processing either previous or next GE.

There are two scenarios that may break the TE/GE braketing behavior I am relying
on:
- Too many GEs: easy case, will ignore empty queues.
- Too many TEs: can it happen except for the OOPIF case?

[mustaq, 2016/06/02 19:26:27]

As we suspected, both of these cases can happen, see the repro
output.jsbin.com/vipuca (touch the pink iframe, then touch at the same coord
again).

My current patch handles all such cases: it uses local sync between TE & GE
handler as before, but brings touch-id plumbing back from Patch#2 to handle
unpaired event streams correctly during the local sync.

+            }
         }
-        // TODO(crbug.com/507408): Right now we add the touch point only if
-        // its pointer event is NotHandled (e.g. preventDefault is called in
-        // the pointer event listener). This behavior needs to change as it
-        // may create some inconsistent touch event sequence.
-        if (result == WebInputEventResult::NotHandled) {
-            touchInfos.append(touchInfo);
-        }
+
+        touchInfos.append(touchInfo);
     }
 }
 
 WebInputEventResult PointerEventManager::sendTouchPointerEvent(
     EventTarget* target, PointerEvent* pointerEvent)
 {
     if (m_inCanceledStateForPointerTypeTouch)
         return WebInputEventResult::NotHandled;
 
     processCaptureAndPositionOfPointerEvent(pointerEvent, target);
@@ skipping 96 matching lines @@
 {
 }
 
 void PointerEventManager::clear()
 {
     for (auto& entry : m_preventMouseEventForPointerType)
         entry = false;
     m_touchEventManager.clear();
     m_inCanceledStateForPointerTypeTouch = false;
     m_pointerEventFactory.clear();
+    m_primaryPointerdownResults.clear();
     m_nodeUnderPointer.clear();
     m_pointerCaptureTarget.clear();
     m_pendingPointerCaptureTarget.clear();
 }
 
 void PointerEventManager::processCaptureAndPositionOfPointerEvent(
     PointerEvent* pointerEvent,
     EventTarget* hitTestTarget,
     EventTarget* lastNodeUnderMouse,
     const PlatformMouseEvent& mouseEvent,
@@ skipping 165 matching lines @@
 {
     visitor->trace(m_frame);
     visitor->trace(m_nodeUnderPointer);
     visitor->trace(m_pointerCaptureTarget);
     visitor->trace(m_pendingPointerCaptureTarget);
     visitor->trace(m_touchEventManager);
 }
 
 
 } // namespace blink