Send boundary events before gotpointercapture

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/DocumentUserGestureToken.h"
 #include "core/dom/ElementTraversal.h"
 #include "core/dom/shadow/FlatTreeTraversal.h"
 #include "core/events/MouseEvent.h"
@@ skipping 540 matching lines @@
   return pointerCaptureTargetTemp != pendingPointercaptureTargetTemp;
 }
 
 EventTarget* PointerEventManager::processCaptureAndPositionOfPointerEvent(
     PointerEvent* pointerEvent,
     EventTarget* hitTestTarget,
     const PlatformMouseEvent& mouseEvent,
     bool sendMouseEvent) {
   processPendingPointerCapture(pointerEvent);
 
   if (!RuntimeEnabledFeatures::pointerEventV1SpecCapturingEnabled()) {

[mustaq, 2016/11/22 21:46:36]

Wait...one last thing:

- With pointerEventV1SpecCapturingEnabled(), we may see redundant enter/leave
events. I don't care, we will remove the flag soon. But add a comment if this is
the case.

- This block and the setNodeUnderPointer() call below now /effective/ for only
lostpointercapture, right? Please comment here to clarify.

- Any chance we can/should move this to processPendingPointerCapture()?

[Navid Zolghadr, 2016/11/22 21:58:11]

- I noticed this problem before actually. I could have passed an extra parameter
to avoid it. But I was not sure how important that is. Which one do you prefer?

1. Pass the extra parameter to fix the problem when the
pointerEventV1SpecCapturingEnabled is on
2. Add the comment that a problem exists.
3. Just remove that flag by tomorrow if that is not necessary anymore.


- That is correct. Sure. I'll add a comment

- Which block? So you processPendingPointerCapture should somehow return a
hitTestTarget or something?

[mustaq, 2016/11/23 15:38:47]

I vote for #3.
I was thinking about the |if| plus the call to setNodeUnderPointer(). It seems a
bit odd & error-prone that only a part of lostpointercapture is handled outside
processPendingPointerCapture(). Does it look better if
processPendingPointerCapture takes an optional hittestTarget, and returns the
"effective" target?

[Navid Zolghadr, 2016/11/23 15:59:39]

Alright. I work on this before this lands. This change has to wait for web
platform tests to update anyway.
I thought a bit more about it and realized my previous answer was not very
accurate. The setNodeUnderPointer outside of processPendingPointerCapture will
only work when there is no capture including when this very event causes the
lostpointercapture and it will have no effect when this very event sends a
gotpointercapture. So if there is no capturing at all (no got or lost) this
setNodeUnderPointer here is the only place that causes the boundary events.

Regarding the if block not every place that processPendingPointerCapture is used
that is needed. The only place that is needed to have the hit-test target
overrided is processCaptureAndPositionOfPointerEvent that as the name suggests
is a wrapper around processPendingPointerCapture that also deals with the target
and position of the pointer. That is why I put that if block here in the first
place.

     PointerCapturingMap::const_iterator it =
         m_pointerCaptureTarget.find(pointerEvent->pointerId());
     if (EventTarget* pointercaptureTarget =
             (it != m_pointerCaptureTarget.end()) ? it->value : nullptr)
       hitTestTarget = pointercaptureTarget;
   }
 
   setNodeUnderPointer(pointerEvent, hitTestTarget);
   if (sendMouseEvent) {
     m_mouseEventManager->setNodeUnderMouse(
@@ skipping 19 matching lines @@
     // 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));
   }
-  // Note that If pendingPointerCaptureTarget is null dispatchPointerEvent
-  // automatically does nothing.
-  dispatchPointerEvent(pendingPointerCaptureTarget,
-                       m_pointerEventFactory.createPointerCaptureEvent(
-                           pointerEvent, EventTypeNames::gotpointercapture));
 
-  if (pendingPointerCaptureTarget)
+  if (pendingPointerCaptureTarget) {
+    setNodeUnderPointer(pointerEvent, pendingPointerCaptureTarget);
+    dispatchPointerEvent(pendingPointerCaptureTarget,
+                         m_pointerEventFactory.createPointerCaptureEvent(
+                             pointerEvent, EventTypeNames::gotpointercapture));
     m_pointerCaptureTarget.set(pointerId, pendingPointerCaptureTarget);
-  else
+  } else {
     m_pointerCaptureTarget.remove(pointerId);
+  }
 }
 
 void PointerEventManager::removeTargetFromPointerCapturingMapping(
     PointerCapturingMap& map,
     const EventTarget* target) {
   // 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;
@@ skipping 98 matching lines @@
       return true;
   }
   return false;
 }
 
 EventTarget* PointerEventManager::getMouseCapturingNode() {
   return getCapturingNode(PointerEventFactory::s_mouseId);
 }
 
 }  // namespace blink