Suppressed MEs for gestures from cancelled PEs

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

Index: third_party/WebKit/Source/core/input/PointerEventManager.cpp
diff --git a/third_party/WebKit/Source/core/input/PointerEventManager.cpp b/third_party/WebKit/Source/core/input/PointerEventManager.cpp
index a4a0fa685e3d705a74c016139af1788c30837816..0a53c882f2ff435e5bb74e4fd82c91aa6868f9e6 100644
--- a/third_party/WebKit/Source/core/input/PointerEventManager.cpp
+++ b/third_party/WebKit/Source/core/input/PointerEventManager.cpp
@@ -454,8 +454,6 @@ void PointerEventManager::dispatchTouchPointerEvents(
             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
@@ -474,15 +472,20 @@ void PointerEventManager::dispatchTouchPointerEvents(
                 touchInfo.touchNode ?
                     touchInfo.touchNode->document().domWindow() : nullptr);
 
-            result = sendTouchPointerEvent(touchInfo.touchNode, pointerEvent);
-        }
-        // 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);
+            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.

+            }
         }
+
+        touchInfos.append(touchInfo);
     }
 }
 
@@ -599,6 +602,7 @@ void PointerEventManager::clear()
     m_touchEventManager.clear();
     m_inCanceledStateForPointerTypeTouch = false;
     m_pointerEventFactory.clear();
+    m_primaryPointerdownResults.clear();
     m_nodeUnderPointer.clear();
     m_pointerCaptureTarget.clear();
     m_pendingPointerCaptureTarget.clear();