Update hover states when dragging the mouse into an element

third_party/WebKit/Source/core/dom/Document.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  *           (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2011, 2012 Apple Inc. All
  * rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) 2008, 2009, 2011, 2012 Google Inc. All rights reserved.
@@ skipping 6453 matching lines @@
 
   while (inner_element_in_document &&
          inner_element_in_document->GetDocument() != this) {
     inner_element_in_document->GetDocument().UpdateHoverActiveState(
         request, inner_element_in_document);
     inner_element_in_document =
         inner_element_in_document->GetDocument().LocalOwner();
   }
 
   UpdateDistribution();
   Element* old_active_element = ActiveHoverElement();

[mustaq, 2017/07/10 15:40:45]

This is not your change but this looks odd: all uses of (Set)ActiveHoverElement
or active_hover_element_ suggest it is really about active element and not about
hover.  Please double-check the uses and (if I am correct) rename them to drop
"hover".

[lanwei, 2017/07/12 21:03:10]

Done.

   if (old_active_element && !request.Active()) {
     // The oldActiveElement layoutObject is null, dropped on :active by setting
     // display: none, for instance. We still need to clear the ActiveChain as
     // the mouse is released.
     for (Element* element = old_active_element; element;
          element = FlatTreeTraversal::ParentElement(*element)) {
       element->SetActive(false);
       user_action_elements_.SetInActiveChain(element, false);
     }
     SetActiveHoverElement(nullptr);
   } else {
     Element* new_active_element = inner_element_in_document;
     if (!old_active_element && new_active_element &&
         !new_active_element->IsDisabledFormControl() && request.Active() &&
         !request.TouchMove()) {
       // We are setting the :active chain and freezing it. If future moves
       // happen, they will need to reference this chain.
       for (Element* element = new_active_element; element;
            element = FlatTreeTraversal::ParentElement(*element)) {
         user_action_elements_.SetInActiveChain(element, true);
       }
       SetActiveHoverElement(new_active_element);
     }
   }
 

[mustaq, 2017/07/10 15:40:45]

The |if-else| blocks above covers two changes in active chain: when the active
element changes from non-null to null, and when it changes from non to non-null.
 I think part of this block should consider button-drag as well.

   // Do not set hover state if event is from touch and on mobile.
   bool allow_hover_changes =
       !(request.TouchEvent() && GetPage() &&
         GetPage()->GetVisualViewport().ShouldDisableDesktopWorkarounds());
 
   // If the mouse has just been pressed, set :active on the chain. Those (and
   // only those) nodes should remain :active until the mouse is released.
   bool allow_active_changes = !old_active_element && ActiveHoverElement();
 
   // If the mouse is down and if this is a mouse move event, we want to restrict
-  // changes in :hover/:active to only apply to elements that are in the :active
+  // changes in :active to only apply to elements that are in the :active
   // chain that we froze at the time the mouse went down.
   bool must_be_in_active_chain = request.Active() && request.Move();
 
   Element* old_hover_element = HoverElement();
 
   // The passed in innerElement may not be a result of a hit test for the
   // current up-to-date flat/layout tree. That means the element may be
   // display:none at this point. Skip up the ancestor chain until we reach an
   // element with a layoutObject or a display:contents element.
   Element* new_hover_element =
       SkipDisplayNoneAncestors(inner_element_in_document);
 
   // Update our current hover element.
   if (allow_hover_changes)
     SetHoverElement(new_hover_element);
 
   Node* ancestor_element = nullptr;
   if (old_hover_element && old_hover_element->isConnected() &&
       new_hover_element) {
     Node* ancestor = FlatTreeTraversal::CommonAncestor(*old_hover_element,
                                                        *new_hover_element);
     if (ancestor && ancestor->IsElementNode())
       ancestor_element = ToElement(ancestor);
   }
 
   HeapVector<Member<Element>, 32> elements_to_remove_from_chain;

[mustaq, 2017/07/10 15:40:45]

This asymmetric list for removal vs addition give me some worries.  Why we
remove w/o worrying about active/hover distinction but care about the
distinction when adding?

[lanwei, 2017/07/12 21:03:10]

Done.

-  HeapVector<Member<Element>, 32> elements_to_add_to_chain;
+  HeapVector<Member<Element>, 32> elements_to_add_to_active_chain;
+  HeapVector<Member<Element>, 32> elements_to_add_to_hover_chain;
 
   if (old_hover_element != new_hover_element) {
     // The old hover path only needs to be cleared up to (and not including) the
     // common ancestor;
     //
     // FIXME(ecobos@igalia.com): oldHoverElement may be disconnected from the
     // tree already. This is due to our handling of m_hoverElement in
     // hoveredElementDetached (which assumes all the parents are hovered) and
     // mustBeInActiveChain (which makes this not hold).

[Navid Zolghadr, 2017/07/07 18:21:00]

nit: I believe this comment also doesn't hold since it talks about
mustBeInActiveChain and you have removed it.

[lanwei, 2017/07/12 21:03:10]

Done.

-    //
-    // In that case, none of the nodes in the chain have the flags, so there's
-    // no problem in skipping this step.
     if (old_hover_element && old_hover_element->isConnected()) {
       for (Element* curr = old_hover_element; curr && curr != ancestor_element;
            curr = FlatTreeTraversal::ParentElement(*curr)) {
-        if (!must_be_in_active_chain || curr->InActiveChain())
-          elements_to_remove_from_chain.push_back(curr);
+        elements_to_remove_from_chain.push_back(curr);
       }
     }
   }
 
   // Now set the hover state for our new object up to the root.
   for (Element* curr = new_hover_element; curr;
        curr = FlatTreeTraversal::ParentElement(*curr)) {
+    if (old_hover_element != new_hover_element)
+      elements_to_add_to_hover_chain.push_back(curr);
     if (!must_be_in_active_chain || curr->InActiveChain())
-      elements_to_add_to_chain.push_back(curr);
+      elements_to_add_to_active_chain.push_back(curr);
   }
 
   if (allow_hover_changes) {
     for (Element* element : elements_to_remove_from_chain)
       element->SetHovered(false);
   }
 
+  // Elements past the common ancestor do not change hover state, but might
+  // change active state.
+  for (Element* element : elements_to_add_to_active_chain) {
+    if (allow_active_changes)
+      element->SetActive(true);
+  }
+
   bool saw_common_ancestor = false;
-  for (Element* element : elements_to_add_to_chain) {
-    // Elements past the common ancestor do not change hover state, but might
-    // change active state.
+  for (Element* element : elements_to_add_to_hover_chain) {
     if (element == ancestor_element)
       saw_common_ancestor = true;
-    if (allow_active_changes)
-      element->SetActive(true);
     if (allow_hover_changes &&
         (!saw_common_ancestor || element == hover_element_))
       element->SetHovered(true);
   }
 }
 
 bool Document::HaveScriptBlockingStylesheetsLoaded() const {
   return style_engine_->HaveScriptBlockingStylesheetsLoaded();
 }
 
@@ skipping 350 matching lines @@
 }
 
 void showLiveDocumentInstances() {
   WeakDocumentSet& set = liveDocumentSet();
   fprintf(stderr, "There are %u documents currently alive:\n", set.size());
   for (blink::Document* document : set)
     fprintf(stderr, "- Document %p URL: %s\n", document,
             document->Url().GetString().Utf8().data());
 }
 #endif