Send got/lostpointercapture immediately if possible

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/ExecutionContextTask.h"
 #include "core/dom/shadow/FlatTreeTraversal.h"
 #include "core/events/MouseEvent.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/UseCounter.h"
 #include "core/html/HTMLCanvasElement.h"
 #include "core/input/EventHandler.h"
 #include "core/input/TouchActionUtil.h"
 #include "core/page/ChromeClient.h"
 #include "core/page/Page.h"
 #include "platform/PlatformTouchEvent.h"
@@ skipping 587 matching lines @@
         entry = false;
     m_touchEventManager.clear();
     m_inCanceledStateForPointerTypeTouch = false;
     m_pointerEventFactory.clear();
     m_touchIdsForCanceledPointerdowns.clear();
     m_nodeUnderPointer.clear();
     m_pointerCaptureTarget.clear();
     m_pendingPointerCaptureTarget.clear();
 }
 
+bool PointerEventManager::getPointerCaptureState(int pointerId,
+    EventTarget** pointerCaptureTarget,
+    EventTarget** pendingPointerCaptureTarget)
+{
+    EventTarget* pct = m_pointerCaptureTarget.contains(pointerId)

[Rick Byers, 2016/07/15 17:35:32]

nit: HashMap::find is the efficient way to do contains/get at the same time.

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+        ? m_pointerCaptureTarget.get(pointerId)
+        : nullptr;
+    EventTarget* ppct = m_pendingPointerCaptureTarget.contains(pointerId)
+        ? m_pendingPointerCaptureTarget.get(pointerId)
+        : nullptr;
+
+    if (pointerCaptureTarget)
+        *pointerCaptureTarget = pct;
+    if (pendingPointerCaptureTarget)
+        *pendingPointerCaptureTarget = ppct;
+
+    return pct != ppct;
+}
+
 void PointerEventManager::processCaptureAndPositionOfPointerEvent(
     PointerEvent* pointerEvent,
     EventTarget* hitTestTarget,
     EventTarget* lastNodeUnderMouse,
     const PlatformMouseEvent& mouseEvent,
     bool sendMouseEvent, bool setPointerPosition)
 {
     bool isCaptureChanged = false;
 
     if (setPointerPosition) {
         isCaptureChanged = processPendingPointerCapture(pointerEvent,
             hitTestTarget, mouseEvent, sendMouseEvent);
         // If there was a change in capturing processPendingPointerCapture has
         // already taken care of transition events. So we don't need to send the
         // transition events here.
         setNodeUnderPointer(pointerEvent, hitTestTarget, !isCaptureChanged);
     }
     if (sendMouseEvent && !isCaptureChanged) {
         // lastNodeUnderMouse is needed here because it is still stored in EventHandler.
         sendBoundaryEvents(lastNodeUnderMouse, hitTestTarget,
             pointerEvent, mouseEvent, true);
     }
 }
 
+void PointerEventManager::immediateProcessPendingPointerCapture(int pointerId)

[mustaq, 2016/07/15 19:35:43]

Nit: s/immediate/immediately/

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+{
+    EventTarget* pointerCaptureTarget;
+    EventTarget* pendingPointerCaptureTarget;
+    const bool isCaptureChanged = getPointerCaptureState(pointerId,
+        &pointerCaptureTarget, &pendingPointerCaptureTarget);
+
+    if (!isCaptureChanged)
+        return;
+
+    if (pointerCaptureTarget) {
+        // Re-target lostpointercapture to the document when the element is

[Rick Byers, 2016/07/15 17:35:32]

is this behavior specified somewhere?  It seems reasonable to me, but if it's
not spec'd then please file a GitHub issue and reference it from a TODO here
(but obviously don't block on it).

[Navid Zolghadr, 2016/07/15 20:41:01]

Yes. It is here the second paragraph.
https://w3c.github.io/pointerevents/#implicit-release-of-pointer-capture

+        // no longer participating in the tree.
+        EventTarget* target = pointerCaptureTarget;
+        if (target->toNode()
+            && !target->toNode()->inShadowIncludingDocument()) {
+            target = target->toNode()->ownerDocument();
+        }
+        dispatchPointerEvent(target,
+            m_pointerEventFactory.createPointerCaptureEvent(
+                pointerId, EventTypeNames::lostpointercapture));
+    }
+    dispatchPointerEvent(pendingPointerCaptureTarget,

[Rick Byers, 2016/07/15 17:35:32]

Don't you need to check for null here too - eg. for the
Element.releasePointerCapture cases where only a 'lost' event is fired without
any 'got'?

[mustaq, 2016/07/15 19:35:43]

Yes, pendingPointerCaptureTarget would be null after a release call.

[Navid Zolghadr, 2016/07/15 20:41:01]

Since there was a lot of those checks around every dispatchPointerEvent in this
file I just put them inside dispatchPointerEvent to avoid having it for every
one in one of the early changes. The only reason that I have it in the previous
case is because for the lostpointercapture case it wanted to get the parent not
and I wanted to make sure it is not null.

+        m_pointerEventFactory.createPointerCaptureEvent(
+            pointerId, EventTypeNames::gotpointercapture));
+
+    // Setting |m_pointerCaptureTarget| comes at the end to prevent the infinite
+    // loop if js calls set/releasepointercapture in got/lostpointercapture
+    // handlers.
+    if (pendingPointerCaptureTarget)
+        m_pointerCaptureTarget.set(pointerId, pendingPointerCaptureTarget);
+    else
+        m_pointerCaptureTarget.remove(pointerId);
+}
+
 bool PointerEventManager::processPendingPointerCapture(

[Rick Byers, 2016/07/15 17:35:31]

once we make the other changes here (eg. not firing the boundary events) it
should be easier to merge these two similar "process pending pointer capture"
codepaths, right?  Please add a TODO saying they should be merged.

[Navid Zolghadr, 2016/07/15 20:41:01]

That is correct. I added the TODO.

[mustaq, 2016/07/15 21:06:14]

"Not firing the boundary events": we still need to fire boundary
events here because of the consistency principles regarding entry vs
capturing states, right?
https://github.com/w3c/pointerevents/issues/61#issuecomment-218512170
(The github issue is about boundary events /during capturing/, not
/at the moment of capturing/.)

     PointerEvent* pointerEvent,
     EventTarget* hitTestTarget,
     const PlatformMouseEvent& mouseEvent, bool sendMouseEvent)
 {
     int pointerId = pointerEvent->pointerId();
-    EventTarget* pointerCaptureTarget =
-        m_pointerCaptureTarget.contains(pointerId)
-        ? m_pointerCaptureTarget.get(pointerId) : nullptr;
-    EventTarget* pendingPointerCaptureTarget =
-        m_pendingPointerCaptureTarget.contains(pointerId)
-        ? m_pendingPointerCaptureTarget.get(pointerId) : nullptr;
+    EventTarget* pointerCaptureTarget;
+    EventTarget* pendingPointerCaptureTarget;
+    const bool isCaptureChanged = getPointerCaptureState(pointerId,
+        &pointerCaptureTarget, &pendingPointerCaptureTarget);
     const EventTargetAttributes &nodeUnderPointerAtt =
         m_nodeUnderPointer.contains(pointerId)
         ? m_nodeUnderPointer.get(pointerId) : EventTargetAttributes();
-    const bool isCaptureChanged =
-        pointerCaptureTarget != pendingPointerCaptureTarget;
 
     if (isCaptureChanged) {
         if ((hitTestTarget != nodeUnderPointerAtt.target
             || (pendingPointerCaptureTarget
             && pendingPointerCaptureTarget != nodeUnderPointerAtt.target))
             && nodeUnderPointerAtt.hasRecievedOverEvent) {
             if (sendMouseEvent) {
                 // Send pointer event transitions as the line after this if
                 // block sends the mouse events
                 sendBoundaryEvents(nodeUnderPointerAtt.target, nullptr,
                     pointerEvent);
             }
             sendBoundaryEvents(nodeUnderPointerAtt.target, nullptr,
                 pointerEvent, mouseEvent, sendMouseEvent);
         }
         if (pointerCaptureTarget) {
             // Re-target lostpointercapture to the document when the element is
             // no longer participating in the tree.
             EventTarget* target = pointerCaptureTarget;
             if (target->toNode()
                 && !target->toNode()->isConnected()) {
                 target = target->toNode()->ownerDocument();
             }
             dispatchPointerEvent(target,
                 m_pointerEventFactory.createPointerCaptureEvent(
-                pointerEvent, EventTypeNames::lostpointercapture));
+                    pointerId, EventTypeNames::lostpointercapture));
         }
     }
 
     // Set pointerCaptureTarget from pendingPointerCaptureTarget. This does
     // affect the behavior of sendBoundaryEvents function. So the

[mustaq, 2016/07/15 19:35:43]

Does that mean the boundary event firing is inconsistent between this and the
immediate processing above?

[Navid Zolghadr, 2016/07/15 20:41:01]

I believe yes. But I saw none of the tests failing. I'll see if anything fails
on the bots as well. However, I'm going to get rid of the boundary events and
mouse hit testing early next week in another change and that won't be any
problem I believe.

[mustaq, 2016/07/15 21:06:14]

Here is the catch for not sending boundary events consistently with
got/lost events: the actual sequence of boundary events will be
determined by whether pending-pointer-capture was processed through
posted-tasks or through events fired during a jank. We need to fix it.

     // ordering of the surrounding blocks of code for sending transition events
     // are important.
     if (pendingPointerCaptureTarget)
         m_pointerCaptureTarget.set(pointerId, pendingPointerCaptureTarget);
     else
         m_pointerCaptureTarget.remove(pointerId);
 
     if (isCaptureChanged) {
         dispatchPointerEvent(pendingPointerCaptureTarget,
             m_pointerEventFactory.createPointerCaptureEvent(
-            pointerEvent, EventTypeNames::gotpointercapture));
+                pointerId, EventTypeNames::gotpointercapture));
         if ((pendingPointerCaptureTarget == hitTestTarget
             || !pendingPointerCaptureTarget)
             && (nodeUnderPointerAtt.target != hitTestTarget
             || !nodeUnderPointerAtt.hasRecievedOverEvent)) {
             if (sendMouseEvent) {
                 // Send pointer event transitions as the line after this if
                 // block sends the mouse events
                 sendBoundaryEvents(nullptr, hitTestTarget, pointerEvent);
             }
             sendBoundaryEvents(nullptr, hitTestTarget, pointerEvent,
@@ skipping 33 matching lines @@
         m_pointerCaptureTarget.remove(pointerId);
         m_nodeUnderPointer.remove(pointerId);
     }
 }
 
 void PointerEventManager::elementRemoved(EventTarget* target)
 {
     removeTargetFromPointerCapturingMapping(m_pendingPointerCaptureTarget, target);
 }
 
 void PointerEventManager::setPointerCapture(int pointerId, EventTarget* target)

[mustaq, 2016/07/15 19:35:43]

It's not clear to me what happens in case of a janky JS handler where
setPointerCapture call is after the jank: what guarantees here that the posted
task goes ahead of the other PE handler calls triggered by user activities
during the jank?

[Navid Zolghadr, 2016/07/15 20:41:01]

Nothing I guess. Actually before we fire every pointerevent we make sure to run
that processPendingPointerCapture anyway through
processCaptureAndPositionOfPointerEvent. So we never miss sending
got/lostpointercapture before the next pointerevent.

 {
     UseCounter::count(m_frame->document(), UseCounter::PointerEventSetCapture);
-    if (m_pointerEventFactory.isActiveButtonsState(pointerId))
+    if (m_pointerEventFactory.isActiveButtonsState(pointerId)) {
+        bool wasCaptureStationary = !getPointerCaptureState(pointerId, nullptr, nullptr);
         m_pendingPointerCaptureTarget.set(pointerId, target);
+        // Only queue the processing pending pointer capture if the pending

[Rick Byers, 2016/07/15 17:35:32]

nit: Add a comment referencing the github issue where we've debated the
semantics (since it may still change).

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+        // capture target and capture target were the same to prevent infinite
+        // got/lostpointercapture event sequence.
+        if (wasCaptureStationary) {
+            m_frame->document()->getExecutionContext()->postTask(

[Rick Byers, 2016/07/15 17:35:31]

Looking at this more, I think you probably should be using the
document()->postTask - if for nothing more than to be consistent with the wide
variety of existing uses (they can all be migrated at once).

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+                BLINK_FROM_HERE,
+                createSameThreadTask(
+                    &PointerEventManager::immediateProcessPendingPointerCapture,
+                    WTF::unretained(this), pointerId));
+        }
+    }
 }
 
 void PointerEventManager::releasePointerCapture(int pointerId, EventTarget* target)
 {
     // Only the element that is going to get the next pointer event can release
     // the capture. Note that this might be different from
     // |m_pointercaptureTarget|. |m_pointercaptureTarget| holds the element
     // that had the capture until now and has been receiving the pointerevents
     // but |m_pendingPointerCaptureTarget| indicated the element that gets the
     // very next pointer event. They will be the same if there was no change in
     // capturing of a particular |pointerId|. See crbug.com/614481.
     if (m_pendingPointerCaptureTarget.get(pointerId) == target)
         releasePointerCapture(pointerId);
 }
 
 void PointerEventManager::releasePointerCapture(int pointerId)
 {
+    bool wasCaptureStationary = !getPointerCaptureState(pointerId, nullptr, nullptr);
     m_pendingPointerCaptureTarget.remove(pointerId);
+    // Only queue the processing pending pointer capture if the pending

[Rick Byers, 2016/07/15 17:35:32]

please avoid copy/paste - share this code

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+    // capture target and capture target were the same to prevent infinite
+    // got/lostpointercapture event sequence.
+    if (wasCaptureStationary) {
+        m_frame->document()->getExecutionContext()->postTask(
+            BLINK_FROM_HERE,
+            createSameThreadTask(
+                &PointerEventManager::immediateProcessPendingPointerCapture,
+                WTF::unretained(this), pointerId));
+    }
 }
 
 bool PointerEventManager::isActive(const int pointerId) const
 {
     return m_pointerEventFactory.isActive(pointerId);
 }
 
 bool PointerEventManager::isAnyTouchActive() const
 {
     return m_touchEventManager.isAnyTouchActive();
@@ skipping 19 matching lines @@
 {
     visitor->trace(m_frame);
     visitor->trace(m_nodeUnderPointer);
     visitor->trace(m_pointerCaptureTarget);
     visitor->trace(m_pendingPointerCaptureTarget);
     visitor->trace(m_touchEventManager);
 }
 
 
 } // namespace blink