Pointerevent capture APIs

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 a61ca1d764093e2a2f13ce0c00bfe09cf97a7eaa..cdc17847e7ef559219ab5d84fe605a003acdb226 100644
--- a/third_party/WebKit/Source/core/input/PointerEventManager.cpp
+++ b/third_party/WebKit/Source/core/input/PointerEventManager.cpp
@@ -36,22 +36,6 @@ bool isInDocument(PassRefPtrWillBeRawPtr<EventTarget> n)
     return n && n->toNode() && n->toNode()->inDocument();
 }
 
-WebInputEventResult dispatchPointerEvent(
-    PassRefPtrWillBeRawPtr<EventTarget> prpTarget,
-    PassRefPtrWillBeRawPtr<PointerEvent> prpPointerEvent,
-    bool checkForListener = false)
-{
-    RefPtrWillBeRawPtr<EventTarget> target = prpTarget;
-    RefPtrWillBeRawPtr<PointerEvent> pointerEvent = prpPointerEvent;
-    if (!RuntimeEnabledFeatures::pointerEventEnabled())
-        return WebInputEventResult::NotHandled;
-    if (!checkForListener || target->hasEventListeners(pointerEvent->type())) {
-        DispatchEventResult dispatchResult = target->dispatchEvent(pointerEvent);
-        return EventHandler::toWebInputEventResult(dispatchResult);
-    }
-    return WebInputEventResult::NotHandled;
-}
-
 WebInputEventResult dispatchMouseEvent(
     PassRefPtrWillBeRawPtr<EventTarget> prpTarget,
     const AtomicString& mouseEventType,
@@ -76,12 +60,41 @@ WebInputEventResult dispatchMouseEvent(
 
 } // namespace
 
-PassRefPtrWillBeRawPtr<Node> PointerEventManager::getEffectiveTargetForPointerEvent(
-    PassRefPtrWillBeRawPtr<Node> target,
-    PassRefPtrWillBeRawPtr<PointerEvent> pointerEvent)
+WebInputEventResult PointerEventManager::dispatchPointerEvent(
+    PassRefPtrWillBeRawPtr<EventTarget> prpTarget,
+    PassRefPtrWillBeRawPtr<PointerEvent> prpPointerEvent,
+    bool checkForListener)
+{
+    RefPtrWillBeRawPtr<EventTarget> target = prpTarget;
+    RefPtrWillBeRawPtr<PointerEvent> pointerEvent = prpPointerEvent;
+
+    // Set whether node under pointer has received pointerover or not.
+    int pointerId = pointerEvent->pointerId();
+    if ((pointerEvent->type() == EventTypeNames::pointerout
+        || pointerEvent->type() == EventTypeNames::pointerover)
+        && m_nodeUnderPointer.contains(pointerId)) {
+        RefPtrWillBeRawPtr<EventTarget> targetUnderPointer =
+            m_nodeUnderPointer.get(pointerId).target;
+        if (targetUnderPointer == target) {
+            m_nodeUnderPointer.set(pointerId, NodeUnderPointerAttributes
+                (targetUnderPointer,
+                pointerEvent->type() == EventTypeNames::pointerover));
+        }
+    }
+    if (!RuntimeEnabledFeatures::pointerEventEnabled())
+        return WebInputEventResult::NotHandled;
+    if (!checkForListener || target->hasEventListeners(pointerEvent->type())) {
+        DispatchEventResult dispatchResult = target->dispatchEvent(pointerEvent);
+        return EventHandler::toWebInputEventResult(dispatchResult);
+    }
+    return WebInputEventResult::NotHandled;
+}
+
+PassRefPtrWillBeRawPtr<EventTarget> PointerEventManager::getEffectiveTargetForPointerEvent(
+    PassRefPtrWillBeRawPtr<EventTarget> target, int pointerId)
 {
-    // TODO(nzolghadr): Add APIs to set the capturing nodes and return the correct node here
-    (void) pointerEvent;
+    if (getCapturingNode(pointerId))

[mustaq, 2016/02/29 19:55:40]

Avoid repeated calls to getCapturingNode.

[Navid Zolghadr, 2016/02/29 20:10:42]

Done.

+        return getCapturingNode(pointerId);
     return target;
 }
 
@@ -92,10 +105,14 @@ void PointerEventManager::sendNodeTransitionEvents(
     const PlatformMouseEvent& mouseEvent,
     PassRefPtrWillBeRawPtr<AbstractView> view)
 {
+    RefPtrWillBeRawPtr<PointerEvent> pointerEvent =
+        m_pointerEventFactory.create(EventTypeNames::mouseout, mouseEvent,
+        nullptr, view);
+    processPendingPointerCapture(pointerEvent, enteredNode, mouseEvent, true);
+
     // Pointer event type does not matter as it will be overridden in the sendNodeTransitionEvents
-    sendNodeTransitionEvents(exitedNode, enteredNode,
-        m_pointerEventFactory.create(EventTypeNames::mouseout, mouseEvent, nullptr, view),
-        mouseEvent, true);
+    sendNodeTransitionEvents(exitedNode, enteredNode, pointerEvent, mouseEvent,
+        true);
 }
 
 void PointerEventManager::sendNodeTransitionEvents(
@@ -110,6 +127,16 @@ void PointerEventManager::sendNodeTransitionEvents(
     if (exitedTarget == enteredTarget)
         return;
 
+    if (getCapturingNode(pointerEvent->pointerId())) {

[mustaq, 2016/02/29 19:55:40]

Ditto.

[Navid Zolghadr, 2016/02/29 20:10:42]

Done.

+        EventTarget* capturingNode = getCapturingNode(pointerEvent->pointerId());
+        if (capturingNode == exitedTarget)
+            enteredTarget = nullptr;
+        else if (capturingNode == enteredTarget)
+            exitedTarget = nullptr;
+        else
+            return;
+    }
+
     // Dispatch pointerout/mouseout events
     if (isInDocument(exitedTarget)) {
         dispatchPointerEvent(exitedTarget, m_pointerEventFactory.create(
@@ -230,15 +257,19 @@ void PointerEventManager::setNodeUnderPointer(
     RefPtrWillBeRawPtr<PointerEvent> pointerEvent = prpPointerEvent;
     RefPtrWillBeRawPtr<EventTarget> target = prpTarget;
     if (m_nodeUnderPointer.contains(pointerEvent->pointerId())) {
-        sendNodeTransitionEvents(m_nodeUnderPointer.get(
-            pointerEvent->pointerId()), target, pointerEvent);
-        if (!target)
+        NodeUnderPointerAttributes node = m_nodeUnderPointer.get(
+            pointerEvent->pointerId());
+        if (!target) {
             m_nodeUnderPointer.remove(pointerEvent->pointerId());
-        else
-            m_nodeUnderPointer.set(pointerEvent->pointerId(), target);
+        } else {
+            m_nodeUnderPointer.set(pointerEvent->pointerId(),
+                NodeUnderPointerAttributes(target, false));
+        }
+        sendNodeTransitionEvents(node.target, target, pointerEvent);
     } else if (target) {
+        m_nodeUnderPointer.add(pointerEvent->pointerId(),
+            NodeUnderPointerAttributes(target, false));
         sendNodeTransitionEvents(nullptr, target, pointerEvent);
-        m_nodeUnderPointer.add(pointerEvent->pointerId(), target);
     }
 }
 
@@ -248,11 +279,16 @@ void PointerEventManager::sendTouchCancelPointerEvent(PassRefPtrWillBeRawPtr<Eve
     RefPtrWillBeRawPtr<PointerEvent> pointerEvent =
         m_pointerEventFactory.createPointerCancel(point);
 
+    processPendingPointerCapture(pointerEvent, target);
+
     // TODO(nzolghadr): crbug.com/579553 dealing with implicit touch capturing vs pointer event capturing
-    target->dispatchEvent(pointerEvent.get());
+    dispatchPointerEvent(
+        getEffectiveTargetForPointerEvent(target, pointerEvent->pointerId()),
+        pointerEvent.get());
 
-    m_pointerEventFactory.remove(pointerEvent);
     setNodeUnderPointer(pointerEvent, nullptr);
+
+    removePointerEvent(pointerEvent);
 }
 
 WebInputEventResult PointerEventManager::sendTouchPointerEvent(
@@ -262,22 +298,30 @@ WebInputEventResult PointerEventManager::sendTouchPointerEvent(
     const double clientX, const double clientY)
 {
     RefPtrWillBeRawPtr<EventTarget> target = prpTarget;
+
     RefPtrWillBeRawPtr<PointerEvent> pointerEvent =
         m_pointerEventFactory.create(
         pointerEventNameForTouchPointState(touchPoint.state()),
         touchPoint, modifiers, width, height, clientX, clientY);
 
+    processPendingPointerCapture(pointerEvent, target);
+
     setNodeUnderPointer(pointerEvent, target);
 
     // TODO(nzolghadr): crbug.com/579553 dealing with implicit touch capturing vs pointer event capturing
-    WebInputEventResult result = dispatchPointerEvent(target, pointerEvent.get());
+    WebInputEventResult result = dispatchPointerEvent(
+        getEffectiveTargetForPointerEvent(target, pointerEvent->pointerId()),
+        pointerEvent.get());
 
     if (touchPoint.state() == PlatformTouchPoint::TouchReleased
         || touchPoint.state() == PlatformTouchPoint::TouchCancelled) {
-        m_pointerEventFactory.remove(pointerEvent);
         setNodeUnderPointer(pointerEvent, nullptr);
+        removePointerEvent(pointerEvent);
     }
 
+    if (pointerEvent->buttons() == 0)

[mustaq, 2016/02/29 19:55:40]

Here |buttons| can be zero thru either finger lifted off the screen or
touchcancel. So the above |if| with removePointerEvent sufficient, right?

[Navid Zolghadr, 2016/02/29 20:10:41]

I believe you are right. I can't remember of any reason to have it here.

+        implicitReleasePointerCapture(pointerEvent->pointerId());
+
     return result;
 }
 
@@ -291,8 +335,13 @@ WebInputEventResult PointerEventManager::sendMousePointerEvent(
         m_pointerEventFactory.create(mouseEventType, mouseEvent,
         relatedTarget, view);
 
-    RefPtrWillBeRawPtr<Node> effectiveTarget =
-        getEffectiveTargetForPointerEvent(target, pointerEvent);
+    processPendingPointerCapture(pointerEvent, target, mouseEvent, true);
+
+    // TODO(crbug.com/587955): We should call setNodeUnderPointer here but it causes sending
+    // transition events that should be first removed from EventHandler.
+
+    RefPtrWillBeRawPtr<EventTarget> effectiveTarget =
+        getEffectiveTargetForPointerEvent(target, pointerEvent->pointerId());
 
     WebInputEventResult result =
         dispatchPointerEvent(effectiveTarget, pointerEvent);
@@ -307,6 +356,9 @@ WebInputEventResult PointerEventManager::sendMousePointerEvent(
             nullptr, clickCount));
     }
 
+    if (pointerEvent->buttons() == 0)
+        implicitReleasePointerCapture(pointerEvent->pointerId());
+
     return result;
 }
 
@@ -335,10 +387,113 @@ void PointerEventManager::conditionallyEnableMouseEventForPointerTypeMouse(
         m_preventMouseEventForPointerTypeMouse = false;
 }
 
+void PointerEventManager::processPendingPointerCapture(
+    const PassRefPtrWillBeRawPtr<PointerEvent> pointerEvent,
+    PassRefPtrWillBeRawPtr<EventTarget> hitTestTarget,

[mustaq, 2016/02/29 19:55:40]

const ... hitTestTarget.

Same for the local vars below.

[Navid Zolghadr, 2016/02/29 20:10:42]

Sure.

+    const PlatformMouseEvent& mouseEvent, bool sendMouseEvent)
+{
+    // TODO(nzolghadr): Process sending got/lostpointercapture
+    int pointerId = pointerEvent->pointerId();
+    bool hasPointerCaptureTarget = m_pointerCaptureTarget.contains(pointerId);
+    bool hasPendingPointerCaptureTarget = m_pendingPointerCaptureTarget.contains(pointerId);
+    RefPtrWillBeRawPtr<EventTarget> pointerCaptureTarget =
+        hasPointerCaptureTarget ? m_pointerCaptureTarget.get(pointerId) : nullptr;
+    RefPtrWillBeRawPtr<EventTarget> pendingPointerCaptureTarget =
+        hasPendingPointerCaptureTarget ? m_pendingPointerCaptureTarget.get(pointerId) : nullptr;
+
+    if (hasPendingPointerCaptureTarget

[mustaq, 2016/02/29 19:55:40]

Your ordering is different from the spec algorithm. It is because of
pointerleave vs pointerenter relative ordering? I thought leave vs enter firing
happens on disjoint cases, so the inverted ordering here is not needed.
Moreover, we would want to maintain the got-vs-lost pointercapture event order.

[Navid Zolghadr, 2016/02/29 20:10:42]

It is because of the sendNodeTransitionEvents function. That function cares
about the m_pointerCaptureTarget. As a result I need to fire the leave/out
events before setting anything to that map and fire the enter/over events after
remove the target from the map. At the end as you said the result is the same as
the spec because they don't happen together. Is this fine?

[mustaq, 2016/03/04 15:16:40]

- We need to add a test that covers the independence of the ordering here, i.e.,
the firing of out+leave vs firing of over+enter at hittest node. I guess just
capturing a specific id to node1, then capturing the same id to node2 should be
enough, right?

- Do you think adding firing of got/lostpointercapture here would be easy? If
not, defer the change for a later CL. I understand that with the correct
(spec-like) ordering, this would be trivial when you need to fire both got &
lost here.

[Navid Zolghadr, 2016/03/04 15:46:02]

I'm not quite sure what you want to test here. Do you want me to add got/lost as
well to test along with this change or something else? In the scenario you
explained, where do you want the mouse pointer to be? On the node 1 or 2? Note
that what we have right now in the test for the "explicit release" scenario does
test the over+enter on blue. Right?

Regarding firing got/lostpointercapture, I cannot use a nested if else for those
like spec structure. I don't see a lot of value of the nested checks in the spec
either tough. They only share one condition and even the second condition in the
first line of each step is a subset of the condition in the further line.

+        && (pointerCaptureTarget != pendingPointerCaptureTarget)) {
+        if (!hasPointerCaptureTarget
+            && pendingPointerCaptureTarget != hitTestTarget
+            && m_nodeUnderPointer.contains(pointerId)
+            && m_nodeUnderPointer.get(pointerId).hasRecievedOverEvent) {
+            sendNodeTransitionEvents(hitTestTarget, nullptr, pointerEvent,
+                mouseEvent, sendMouseEvent);
+        }
+    }
+
+    // Set pointerCaptureTarget from pendingPointerCaptureTarget
+    if (!m_pendingPointerCaptureTarget.contains(pointerId))

[mustaq, 2016/02/29 19:55:40]

if (!has...)

[Navid Zolghadr, 2016/02/29 20:10:41]

Done.

+        m_pointerCaptureTarget.remove(pointerId);
+    else
+        m_pointerCaptureTarget.set(pointerId, m_pendingPointerCaptureTarget.get(pointerId));
+
+    if (hasPointerCaptureTarget && (pointerCaptureTarget != pendingPointerCaptureTarget)) {
+        if (!hasPendingPointerCaptureTarget && pointerCaptureTarget != hitTestTarget) {
+            sendNodeTransitionEvents(nullptr, hitTestTarget, pointerEvent,
+                mouseEvent, sendMouseEvent);
+        }
+    }
+
+}
+
+void PointerEventManager::removeTargetFromPointerCapturingMapping(PointerCapturingMap& map, EventTarget* target)

[mustaq, 2016/02/29 19:55:40]

const EventTarget*

[Navid Zolghadr, 2016/02/29 20:10:41]

Done.

+{
+    // We could have kept a reverse mapping to make this deletion possibly
+    // faster but it adds some code complication which might not be worth of
+    // the performance improvement considering there might not be a lot of
+    // active pointer or pointer captures at the same time.
+    PointerCapturingMap tmp = map;

[mustaq, 2016/02/29 19:55:40]

Why do you need tmp here?

[Navid Zolghadr, 2016/02/29 20:10:42]

I was going over the map. So I assumed I cannot modify the container that I'm
traversing. Is that right or does this WTF map takes care of it somehow?

[Rick Byers, 2016/03/01 16:58:35]

I think you can (see the implementation of removeAll in HashTable.h).  

[mustaq, 2016/03/01 17:54:58]

And in this case, you don't need to go further after finding the key.

[Navid Zolghadr, 2016/03/01 20:46:05]

If I correctly understand you Mustaq, I don't think I can stop after finding the
first instance. Note that I'm searching for all the entries with a particular
value and an element may capture multiple pointers at the same time and as a
result have multiple entries in this map with the value equal that node.

[Navid Zolghadr, 2016/03/02 16:54:49]

I still could't figure out how not to create a tmp map here. The removeAll
implementation had 2 sets and it is removing one set from the other one. Can you
give me a few more hints? 

[mustaq, 2016/03/02 21:12:32]

I guess I was thinking about Java iterators which lets you delete and move
ahead. I guess we have to live with the map copy. 

[Rick Byers, 2016/03/03 16:35:45]

Yeah sorry I mis-read the removeAll code.  Anyway this isn't worth debating
about - the set should always be small (in fact a linear list is likely more
efficient than a hash table since the size is almost always one or two and never
more than about 10).  As-is is fine.

[mustaq, 2016/03/03 17:42:48]

I am fine with tmp, sorry for the fuss.

It just happens that crrev.com/1732043002/#ps80001 sends pointercancels in the
order stored in the hashmap, so the layout test outcome becomes dependent on
hashmap's own gc. While that patch is likely to die, I expect that the
fire-all-touch-cancels approach will be there in my final solution.

So, in addition to the performance nit mentioned by Rick, we now have
pointercancel ordering stability plus code maintenance ease in favor of
replacing the two hashmaps with a single list.

+    for (PointerCapturingMap::iterator it = tmp.begin(); it != tmp.end(); ++it) {
+        if (it->value == target)
+            map.remove(it->key);
+    }
+}
+
+EventTarget* PointerEventManager::getCapturingNode(int pointerId)
+{
+    if (m_pointerCaptureTarget.contains(pointerId))
+        return m_pointerCaptureTarget.get(pointerId);
+    return nullptr;
+}
+
+void PointerEventManager::removePointerEvent(

[Rick Byers, 2016/03/01 16:58:35]

seems a little weird to me for this to take a PointerEvent instead of a
pointerId (looks like PointerEventFactory::remove only uses the ID as well).  I
was worried for a moment that we might actually be storing PointerEvent
instances somewhere.

Anyway I don't have super strong opinion here.  If you feel it's best to take a
PointerEvent, how about renaming this method to "removePointer" to make it
clearer that it removes the tracking for the pointer indicated by the supplied
event, not actually removing the event from a list of events or something?

[mustaq, 2016/03/01 17:54:57]

I will take the blame for PointerEventFactory::remove() taking a PE instead of
an |int|. I wanted it before I proposed the Manager/Factory split, to hide the
|int| from EvenHandler. It can be an |int| now.

In fact, my current local patch for touch-action fix (crrev.com/1732043002) has
done the same.

For PointerEventManager, it is still safer to pass the event rather than an
|int| id. I think Rick's renaming suggestion is better.

[Navid Zolghadr, 2016/03/01 20:46:05]

Sure. Then I'll keep the int but with "removePointer" name.

+    const PassRefPtrWillBeRawPtr<PointerEvent> pointerEvent)
+{
+    if (m_pointerEventFactory.remove(pointerEvent)) {
+        int pointerId = pointerEvent->pointerId();
+        m_pendingPointerCaptureTarget.remove(pointerId);
+        m_pointerCaptureTarget.remove(pointerId);
+        m_nodeUnderPointer.remove(pointerId);
+    }
+}
+
+void PointerEventManager::elementRemoved(EventTarget* target)
+{
+    removeTargetFromPointerCapturingMapping(m_pointerCaptureTarget, target);
+    removeTargetFromPointerCapturingMapping(m_pendingPointerCaptureTarget, target);
+    // TODO(nzolghadr): Process sending lostpointercapture to the document
+}
+
+void PointerEventManager::setPointerCapture(int pointerId, EventTarget* target)
+{
+    if (m_pointerEventFactory.isActiveButtonsState(pointerId))
+        m_pendingPointerCaptureTarget.set(pointerId, target);
+}
+
+void PointerEventManager::releasePointerCapture(int pointerId, EventTarget* target)
+{
+    if (m_pointerCaptureTarget.contains(pointerId) &&  m_pointerCaptureTarget.get(pointerId) == target)
+        implicitReleasePointerCapture(pointerId);
+}
+
+void PointerEventManager::implicitReleasePointerCapture(int pointerId)

[Rick Byers, 2016/03/01 16:58:35]

nit: rename this just releasePointerCapture since it's also called in the
explicit case?

[Navid Zolghadr, 2016/03/01 20:46:05]

Is that okay to have the same name for both functions or do you want me to
rename the other one to explicit?

[Rick Byers, 2016/03/03 16:35:45]

Yep and overload seems fine to me (they're conceptually the same thing).

+{
+    if (m_pendingPointerCaptureTarget.contains(pointerId))

[mustaq, 2016/02/29 19:55:40]

s/if/ASSERT/

[Navid Zolghadr, 2016/02/29 20:10:42]

Done.

[Navid Zolghadr, 2016/03/02 16:54:49]

I just remembered that this function is being called even when we don't have any
capturing for a given pointerId like when the buttons=0. So we  don't want to
have that assert here. But I removed the if as I believe remove just does
nothing if the key is not found.

[mustaq, 2016/03/02 21:12:32]

Then both these release functions remove ids conditionally, I guess the
distinction is not clear. "releasePointerCaptureFromTarget" and
"releasePointerCapture"?

+        m_pendingPointerCaptureTarget.remove(pointerId);
+}
+
+bool PointerEventManager::isActive(const int pointerId)
+{
+    return m_pointerEventFactory.isActive(pointerId);
+}
+
 DEFINE_TRACE(PointerEventManager)
 {
 #if ENABLE(OILPAN)
     visitor->trace(m_nodeUnderPointer);
+    visitor->trace(m_pointerCaptureTarget);
+    visitor->trace(m_pendingPointerCaptureTarget);
 #endif
 }