Fixed mouseenter/mouseleave event firing order.

Source/core/page/EventHandler.cpp

Index: Source/core/page/EventHandler.cpp
diff --git a/Source/core/page/EventHandler.cpp b/Source/core/page/EventHandler.cpp
index 794d82088023a83f39973cf44195c7d0c2d93ede..e086906038aa617038e272e368f6efa65f0b0919 100644
--- a/Source/core/page/EventHandler.cpp
+++ b/Source/core/page/EventHandler.cpp
@@ -1446,7 +1446,7 @@ bool EventHandler::handleMouseMoveOrLeaveEvent(const PlatformMouseEvent& mouseEv
     // We don't want to do a hit-test in forceLeave scenarios because there might actually be some other frame above this one at the specified co-ordinate.
     // So we must force the hit-test to fail, while still clearing hover/active state.
     if (forceLeave)
-        m_frame->document()->updateHoverActiveState(request, 0, &mouseEvent);
+        m_frame->document()->updateHoverActiveState(request, 0);
     else
         mev = prepareMouseEvent(request, mouseEvent);
 
@@ -1556,8 +1556,7 @@ bool EventHandler::handleMouseReleaseEvent(const PlatformMouseEvent& mouseEvent)
     if (m_lastScrollbarUnderMouse) {
         invalidateClick();
         m_lastScrollbarUnderMouse->mouseUp(mouseEvent);
-        bool setUnder = false;
-        return !dispatchMouseEvent(EventTypeNames::mouseup, m_lastNodeUnderMouse.get(), m_clickCount, mouseEvent, setUnder);
+        return !dispatchMouseEvent(EventTypeNames::mouseup, m_nodeUnderMouse.get(), m_clickCount, mouseEvent, false);

[Mike West, 2015/03/31 11:45:33]

Nit: We should change `dispatchMouseEvent` to accept an enum rather than a
boolean. `false` is fairly meaningless here, which is why we had `setUnder` in
the first place. If you'd like to get rid of the local variable, I'd prefer for
you to add a `TODO` here, and add a descriptive enum in a subsequent CL.

[mustaq, 2015/03/31 14:08:31]

My main complain was the name setUnder. Added a FIXME in dispatchMouseEvent().

     }
 
     // Mouse events simulated from touch should not hit-test again.
@@ -1847,7 +1846,6 @@ void EventHandler::updateMouseEventTargetNode(Node* targetNode, const PlatformMo
     }
     m_nodeUnderMouse = result;
 
-    // Fire mouseout/mouseover if the mouse has shifted to a different node.
     if (fireMouseOverOut) {
         DeprecatedPaintLayer* layerForLastNode = layerForNode(m_lastNodeUnderMouse.get());
         DeprecatedPaintLayer* layerForNodeUnderMouse = layerForNode(m_nodeUnderMouse.get());
@@ -1882,18 +1880,83 @@ void EventHandler::updateMouseEventTargetNode(Node* targetNode, const PlatformMo
             m_lastScrollbarUnderMouse = nullptr;
         }
 
-        if (m_lastNodeUnderMouse != m_nodeUnderMouse) {
-            // send mouseout event to the old node
-            if (m_lastNodeUnderMouse)
-                m_lastNodeUnderMouse->dispatchMouseEvent(mouseEvent, EventTypeNames::mouseout, 0, m_nodeUnderMouse.get());
-            // send mouseover event to the new node
-            if (m_nodeUnderMouse)
-                m_nodeUnderMouse->dispatchMouseEvent(mouseEvent, EventTypeNames::mouseover, 0, m_lastNodeUnderMouse.get());
-        }
+        if (m_lastNodeUnderMouse != m_nodeUnderMouse)
+            sendMouseEventsForNodeTransition(m_lastNodeUnderMouse.get(), m_nodeUnderMouse.get(), mouseEvent);
+
         m_lastNodeUnderMouse = m_nodeUnderMouse;
     }
 }
 
+void EventHandler::sendMouseEventsForNodeTransition(Node* exitedNode, Node* enteredNode, const PlatformMouseEvent& mouseEvent)
+{
+    ASSERT(exitedNode != enteredNode);
+
+    // First, send the mouseout and mouseover events (which bubble to ancestors)
+    if (exitedNode) {
+        exitedNode->dispatchMouseEvent(mouseEvent, EventTypeNames::mouseout, 0, enteredNode);
+    }
+    if (enteredNode) {
+        enteredNode->dispatchMouseEvent(mouseEvent, EventTypeNames::mouseover, 0, exitedNode);
+    }

[Mike West, 2015/03/31 11:45:33]

Nit: Drop {} in these two one-line `if`s.

[mustaq, 2015/03/31 14:08:31]

Done.

+
+    // Then send mouseenter and mouseleave events. Since these are non-bubbling events, @@@

[Mike West, 2015/03/31 11:45:33]

Nit: "@@@"?

[mustaq, 2015/03/31 14:08:31]

Ooops, used this temp marker to update the comment later on. Done.

+    // they are dispatched iff there is a capturing
+    // event handler on an ancestor or a normal event handler on the element itself. This special
+    // handling is necessary to avoid O(n^2) capturing event handler checks. We'll check the previously
+    // hovered node's ancestor tree for 'mouseleave' handlers here, then check the newly hovered node's
+    // ancestor tree for 'mouseenter' handlers after dispatching the 'mouseleave' events (as the handler
+    // for 'mouseleave' might set a capturing 'mouseenter' handler, odd as that might be).

[Mike West, 2015/03/31 11:45:34]

What about other mutations? I think we had some issues a while ago with
use-after-frees generated by unexpected mutations from mouse events that removed
elements we'd targeted for events. I think oilpan will take care of the danger
here, but doesn't seem to bring sanity to the event chain. That is, if a
mouseleave event reparents a node, we'll end up firing mouseenter events on the
wrong chain, won't we?

[mustaq, 2015/03/31 18:06:27]

You are right but this CL is not introducing any new issues here with respect to
mutations through event handlers.

- This comment block (along with the core logic in the method) here has been
moved from 2nd half of Document::updateHoverActiveState(). I am sort of
preserving the old behavior here, specially in the next patch (Patch Set #3)
where I brought back the support for
adding-capturing-mouseenter-listener-in-mouseleave (and the test). The only
change is decoupling event-sending from active/hover states.

- I think use-after-free problem won't happen since the two lists are of
ref-counted pointers, and the caller passes the Node* parameters to this method
updateMouseEventTargetNode from a ref-counted type.

- Re DOM changes in event handlers: I think the spec is silent (or at least
sloppy) about it:
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener...
so it makes sense to optimize for things that are not forbidden. The existing
code does exactly that, so does this CL.
  (Supporting all possible dynamic scenarios would make the logic complicated
and slow, and I don't think we need to go for this complication given the
little/unlikely benefit. Here is an argument to support my claim: no matter what
optimization we do, we can always come up with an event handler that would act
as an adversary. E.g., in the existing implementation, it is possible to change
the DOM so that the two lists and/or the common ancestor becomes useless, so we
will be forced to send events to /all/ ancestors and force the event mechanism
pay extra cost.)

+
+    // Create lists of all exited/entered ancestors, and mark the presence of capturing listeners for
+    // mouseenter and mouseleave.
+    WillBeHeapVector<RefPtrWillBeMember<Node>, 32> exitedAncestors;
+    WillBeHeapVector<RefPtrWillBeMember<Node>, 32> enteredAncestors;
+    bool exitedHasCapturingAncestor = false;
+    bool enteredHasCapturingAncestor = false;
+
+    for (Node* node = exitedNode; node; node = NodeRenderingTraversal::parent(*node)) {
+        exitedAncestors.append(node);
+        if (node->hasCapturingEventListeners(EventTypeNames::mouseleave))
+            exitedHasCapturingAncestor = true;
+    }
+
+    for (Node* node = enteredNode; node; node = NodeRenderingTraversal::parent(*node)) {
+        enteredAncestors.append(node);
+        if (node->hasCapturingEventListeners(EventTypeNames::mouseenter))
+            enteredHasCapturingAncestor = true;
+    }
+
+    size_t numExitedAncestors = exitedAncestors.size();
+    size_t numEnteredAncestors = enteredAncestors.size();
+
+    // Locate the common ancestor in the two lists. Start with the assumption that it's off both the lists.
+    size_t exitedAncestorIndex = numExitedAncestors;
+    size_t enteredAncestorIndex = numEnteredAncestors;
+    for (size_t j = 0; j < numExitedAncestors; j++) {
+        for (size_t i = 0; i < numEnteredAncestors; i++) {
+            if (exitedAncestors[j] == enteredAncestors[i]) {
+                exitedAncestorIndex = j;
+                enteredAncestorIndex = i;
+                break;
+            }
+        }
+        if (exitedAncestorIndex < numExitedAncestors)
+            break;
+    }
+
+    // Send mouseleave events to appropriate exited ancestors
+    for (size_t j = 0; j < exitedAncestorIndex; j++) {
+        if (exitedHasCapturingAncestor || exitedAncestors[j]->hasEventListeners(EventTypeNames::mouseleave))
+            exitedAncestors[j]->dispatchMouseEvent(mouseEvent, EventTypeNames::mouseleave, 0, enteredNode);
+    }
+
+    // Send mouseenter events to appropriate entered ancestors
+    for (size_t i = enteredAncestorIndex; i > 0; i--) {

[Mike West, 2015/03/31 11:45:33]

Why count backwards here?

[mustaq, 2015/03/31 14:08:31]

We want to fire mouseenter in parent-to-child order, update the block comment to
clarify.
http://www.w3.org/TR/DOM-Level-3-Events/#events-mouseevent-event-order

+        if (enteredHasCapturingAncestor || enteredAncestors[i-1]->hasEventListeners(EventTypeNames::mouseenter))
+            enteredAncestors[i-1]->dispatchMouseEvent(mouseEvent, EventTypeNames::mouseenter, 0, exitedNode);
+    }
+}
+
 // The return value means 'continue default handling.'
 bool EventHandler::dispatchMouseEvent(const AtomicString& eventType, Node* targetNode, int clickCount, const PlatformMouseEvent& mouseEvent, bool setUnder)
 {