Fixed mouseenter/mouseleave event firing order.

Source/core/page/EventHandler.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2012 Digia Plc. and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
@@ skipping 1428 matching lines @@
 
     // Treat any mouse move events as readonly if the user is currently touching the screen.
     if (m_touchPressed)
         hitType |= HitTestRequest::Active | HitTestRequest::ReadOnly;
     HitTestRequest request(hitType);
     MouseEventWithHitTestResults mev = MouseEventWithHitTestResults(mouseEvent, HitTestResult(LayoutPoint()));
 
     // 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);
 
     if (hoveredNode)
         *hoveredNode = mev.hitTestResult();
 
     Scrollbar* scrollbar = nullptr;
 
     if (m_resizeScrollableArea && m_resizeScrollableArea->inResizeMode())
         m_resizeScrollableArea->resize(mouseEvent, m_offsetFromResizeCorner);
@@ skipping 89 matching lines @@
         m_frame->document()->accessSVGExtensions().updatePan(m_frame->view()->rootFrameToContents(m_lastKnownMousePosition));
         return true;
     }
 
     if (m_frameSetBeingResized)
         return !dispatchMouseEvent(EventTypeNames::mouseup, m_frameSetBeingResized.get(), m_clickCount, mouseEvent, false);
 
     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.
     ASSERT(!mouseEvent.fromTouch());
 
     HitTestRequest::HitTestRequestType hitType = HitTestRequest::Release;
     HitTestRequest request(hitType);
     MouseEventWithHitTestResults mev = prepareMouseEvent(request, mouseEvent);
     LocalFrame* subframe = m_capturingMouseEventsNode.get() ? subframeForTargetNode(m_capturingMouseEventsNode.get()) : subframeForHitTestResult(mev);
     if (m_eventHandlerWillResetCapturingMouseEventsNode)
@@ skipping 269 matching lines @@
     // If we're capturing, we always go right to that node.
     if (m_capturingMouseEventsNode)
         result = m_capturingMouseEventsNode.get();
     else {
         // If the target node is a text node, dispatch on the parent node - rdar://4196646
         if (result && result->isTextNode())
             result = NodeRenderingTraversal::parent(*result);
     }
     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());
         Page* page = m_frame->page();
 
         if (m_lastNodeUnderMouse && (!m_nodeUnderMouse || m_nodeUnderMouse->document() != m_frame->document())) {
             // The mouse has moved between frames.
             if (LocalFrame* frame = m_lastNodeUnderMouse->document().frame()) {
                 if (FrameView* frameView = frame->view())
                     frameView->mouseExitedContentArea();
@@ skipping 14 matching lines @@
             // The mouse has moved between layers.
             if (ScrollableArea* scrollableAreaForNodeUnderMouse = associatedScrollableArea(layerForNodeUnderMouse))
                 scrollableAreaForNodeUnderMouse->mouseEnteredContentArea();
         }
 
         if (m_lastNodeUnderMouse && m_lastNodeUnderMouse->document() != m_frame->document()) {
             m_lastNodeUnderMouse = nullptr;
             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;
+            }
         }
-        m_lastNodeUnderMouse = m_nodeUnderMouse;
+        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)
 {
     updateMouseEventTargetNode(targetNode, mouseEvent, setUnder);
     return !m_nodeUnderMouse || m_nodeUnderMouse->dispatchMouseEvent(mouseEvent, eventType, clickCount);
 }
 
@@ skipping 2045 matching lines @@
 
     // If it's in the direction to hide the top controls, only consume when the frame can also scroll.
     if (m_frame->view()->scrollPosition().y() < m_frame->view()->maximumScrollPosition().y())
         return true;
 
     return false;
 }
 
 
 } // namespace blink