[omnibox] Fix the hover glitch on fast mouse movement in omnibox bug

In the omnibox, if you mouse your mouse fast off of a row, while exiting
via a child View, sometimes the whole row would stay hovered.

This manifested itself by both exiting near the remove button, the
suggestion button row, and the expand/collapse chevron button for
header rows.

This CL creates a new OmniboxMouseEnterExitHandler that generalizes
the robliao/ssigwart solution to all of the row controls.

It's also the same pattern used by:
ArcNotificationContentView::MouseEnterExitHandler

So maybe the Views team wants take over this utility class in the
future.

Bug: 1109473, 893183
Change-Id: Ibf3118c3cbe2231c1356778050764c93c93eb016
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2367396Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800702}

chrome/browser/ui/BUILD.gn

@@ -3621,6 +3621,8 @@ static_library("ui") {
       "views/native_file_system/native_file_system_usage_bubble_view.h",
       "views/omnibox/omnibox_match_cell_view.cc",
       "views/omnibox/omnibox_match_cell_view.h",
+      "views/omnibox/omnibox_mouse_enter_exit_handler.cc",
+      "views/omnibox/omnibox_mouse_enter_exit_handler.h",
       "views/omnibox/omnibox_popup_contents_view.cc",
       "views/omnibox/omnibox_popup_contents_view.h",
       "views/omnibox/omnibox_result_view.cc",

chrome/browser/ui/views/omnibox/omnibox_mouse_enter_exit_handler.cc

+// Copyright 2020 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 "chrome/browser/ui/views/omnibox/omnibox_mouse_enter_exit_handler.h"
+
+#include "ui/events/event.h"
+#include "ui/views/view.h"
+
+OmniboxMouseEnterExitHandler::OmniboxMouseEnterExitHandler(
+    base::Closure enter_exit_callback)
+    : enter_exit_callback_(enter_exit_callback) {}
+
+OmniboxMouseEnterExitHandler::~OmniboxMouseEnterExitHandler() {
+  for (views::View* view : observed_views_)
+    view->RemovePreTargetHandler(this);
+}
+
+void OmniboxMouseEnterExitHandler::ObserveMouseEnterExitOn(views::View* view) {
+  view->AddPreTargetHandler(this);
+  observed_views_.push_back(view);
+}
+
+void OmniboxMouseEnterExitHandler::OnMouseEvent(ui::MouseEvent* event) {
+  if (event->type() == ui::ET_MOUSE_ENTERED ||
+      event->type() == ui::ET_MOUSE_EXITED) {
+    enter_exit_callback_.Run();
+  }
+}

chrome/browser/ui/views/omnibox/omnibox_mouse_enter_exit_handler.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_OMNIBOX_OMNIBOX_MOUSE_ENTER_EXIT_HANDLER_H_
+#define CHROME_BROWSER_UI_VIEWS_OMNIBOX_OMNIBOX_MOUSE_ENTER_EXIT_HANDLER_H_
+
+#include <vector>
+
+#include "base/callback.h"
+#include "ui/events/event_handler.h"
+
+namespace views {
+class View;
+}  // namespace views
+
+// Omnibox suggestion rows (and header rows) rely on mouse-enter and mouse-exit
+// events to update their hover state (whether or not they are highlighted).
+// But child Views sometimes interfere with receiving these events when the
+// mouse is moved quickly, which causes rows to remain erroneously highlighted.
+// This class solves that problem by bridging the child Views mouse event
+// handler to a callback. This class instance must outlive all observed Views.
+class OmniboxMouseEnterExitHandler : public ui::EventHandler {
+ public:
+  // |enter_exit_callback| is called whenever one of the observed Views get a
+  // mouse-enter or mouse-exit event.
+  explicit OmniboxMouseEnterExitHandler(base::Closure enter_exit_callback);
+  OmniboxMouseEnterExitHandler(const OmniboxMouseEnterExitHandler&) = delete;
+  OmniboxMouseEnterExitHandler& operator=(const OmniboxMouseEnterExitHandler&) =
+      delete;
+  ~OmniboxMouseEnterExitHandler() override;
+
+  // This instance must outlive |view|.
+  void ObserveMouseEnterExitOn(views::View* view);
+
+ private:
+  void OnMouseEvent(ui::MouseEvent* event) override;
+
+  // This is called whenever one of the |observed_views_| has a mouse-enter or
+  // mouse-exit event.
+  const base::Closure enter_exit_callback_;
+
+  // These are the Views for which we are observing mouse-enter or mouse-exit
+  // events. This instance must outlive all of these Views, since these are
+  // non-owning pointers, which we use in our destructor.
+  std::vector<views::View*> observed_views_;
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_OMNIBOX_OMNIBOX_MOUSE_ENTER_EXIT_HANDLER_H_

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

@@ -8,6 +8,7 @@
 
 #include <algorithm>
 
+#include "base/bind.h"
 #include "base/macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -74,7 +75,11 @@ OmniboxResultView::OmniboxResultView(
     : AnimationDelegateViews(this),
       popup_contents_view_(popup_contents_view),
       model_index_(model_index),
-      animation_(new gfx::SlideAnimation(this)) {
+      animation_(new gfx::SlideAnimation(this)),
+      // Using base::Unretained is correct here. 'this' outlives the callback.
+      mouse_enter_exit_handler_(
+          base::BindRepeating(&OmniboxResultView::UpdateHoverState,
+                              base::Unretained(this))) {
   CHECK_GE(model_index, 0u);
 
   suggestion_view_ = AddChildView(std::make_unique<OmniboxMatchCellView>(this));
@@ -93,9 +98,7 @@ OmniboxResultView::OmniboxResultView(
   // accessibility node data for removable suggestions.
   remove_suggestion_button_ =
       AddChildView(std::make_unique<OmniboxRemoveSuggestionButton>(this));
-  // The remove suggestion button may receive mouse enter/exit events with very
-  // quick mouse movements. Monitor the button to update our state.
-  update_on_mouse_enter_exit_.emplace(this, remove_suggestion_button_);
+  mouse_enter_exit_handler_.ObserveMouseEnterExitOn(remove_suggestion_button_);
   views::InstallCircleHighlightPathGenerator(remove_suggestion_button_);
   remove_suggestion_button_->SetTooltipText(
       l10n_util::GetStringUTF16(IDS_OMNIBOX_REMOVE_SUGGESTION));
@@ -110,6 +113,10 @@ OmniboxResultView::OmniboxResultView(
   if (OmniboxFieldTrial::IsSuggestionButtonRowEnabled()) {
     button_row_ = AddChildView(std::make_unique<OmniboxSuggestionButtonRowView>(
         popup_contents_view_, model_index));
+    // Quickly mouse-exiting through the suggestion button row sometimes leaves
+    // the whole row highlighted. This fixes that. It doesn't seem necessary to
+    // further observe the child controls of |button_row_|.
+    mouse_enter_exit_handler_.ObserveMouseEnterExitOn(button_row_);
   }
 
   keyword_view_ = AddChildView(std::make_unique<OmniboxMatchCellView>(this));
@@ -546,26 +553,6 @@ void OmniboxResultView::EmitTextChangedAccessiblityEvent() {
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxResultView, private:
 
-OmniboxResultView::UpdateOnMouseEnterExit::UpdateOnMouseEnterExit(
-    OmniboxResultView* omnibox_result_view,
-    View* target)
-    : omnibox_result_view_(omnibox_result_view), target_(target) {
-  target_->AddPreTargetHandler(this);
-}
-
-OmniboxResultView::UpdateOnMouseEnterExit::~UpdateOnMouseEnterExit() {
-  target_->RemovePreTargetHandler(this);
-}
-
-void OmniboxResultView::UpdateOnMouseEnterExit::OnMouseEvent(
-    ui::MouseEvent* event) {
-  auto event_type = event->type();
-  if (event_type != ui::ET_MOUSE_ENTERED && event_type != ui::ET_MOUSE_EXITED)
-    return;
-
-  omnibox_result_view_->UpdateHoverState();
-}
-
 gfx::Image OmniboxResultView::GetIcon() const {
   return popup_contents_view_->GetMatchIcon(
       match_, GetColor(OmniboxPart::RESULTS_ICON));

chrome/browser/ui/views/omnibox/omnibox_result_view.h

@@ -11,12 +11,11 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/optional.h"
+#include "chrome/browser/ui/views/omnibox/omnibox_mouse_enter_exit_handler.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/suggestion_answer.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/window_open_disposition.h"
-#include "ui/events/event_handler.h"
 #include "ui/gfx/animation/slide_animation.h"
 #include "ui/gfx/font_list.h"
 #include "ui/gfx/geometry/rect.h"
@@ -107,22 +106,6 @@ class OmniboxResultView : public views::View,
   void OnThemeChanged() override;
 
  private:
-  // Calls UpdateHoverState() when a target receives a mouse enter/exit.
-  class UpdateOnMouseEnterExit : public ui::EventHandler {
-   public:
-    UpdateOnMouseEnterExit(OmniboxResultView* omnibox_result_view,
-                           View* target);
-    UpdateOnMouseEnterExit(const UpdateOnMouseEnterExit&) = delete;
-    UpdateOnMouseEnterExit& operator=(const UpdateOnMouseEnterExit&) = delete;
-    ~UpdateOnMouseEnterExit() override;
-
-   private:
-    void OnMouseEvent(ui::MouseEvent* event) override;
-
-    OmniboxResultView* const omnibox_result_view_;
-    View* const target_;
-  };
-
   // Returns the height of the text portion of the result view.
   int GetTextHeight() const;
 
@@ -177,7 +160,9 @@ class OmniboxResultView : public views::View,
   // The "X" button at the end of the match cell, used to remove suggestions.
   views::ImageButton* remove_suggestion_button_;
   views::FocusRing* remove_suggestion_focus_ring_ = nullptr;
-  base::Optional<UpdateOnMouseEnterExit> update_on_mouse_enter_exit_;
+
+  // Keeps track of mouse-enter and mouse-exit events of child Views.
+  OmniboxMouseEnterExitHandler mouse_enter_exit_handler_;
 
   base::WeakPtrFactory<OmniboxResultView> weak_factory_{this};
 

chrome/browser/ui/views/omnibox/omnibox_row_view.cc

@@ -4,11 +4,13 @@
 
 #include "chrome/browser/ui/views/omnibox/omnibox_row_view.h"
 
+#include "base/bind.h"
 #include "base/i18n/case_conversion.h"
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/omnibox/omnibox_theme.h"
 #include "chrome/browser/ui/views/chrome_typography.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_match_cell_view.h"
+#include "chrome/browser/ui/views/omnibox/omnibox_mouse_enter_exit_handler.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_result_view.h"
 #include "components/omnibox/browser/omnibox_popup_model.h"
 #include "components/omnibox/browser/omnibox_prefs.h"
@@ -34,7 +36,11 @@
 class OmniboxRowView::HeaderView : public views::View,
                                    public views::ButtonListener {
  public:
-  explicit HeaderView(OmniboxRowView* row_view) : row_view_(row_view) {
+  explicit HeaderView(OmniboxRowView* row_view)
+      : row_view_(row_view),
+        // Using base::Unretained is correct here. 'this' outlives the callback.
+        mouse_enter_exit_handler_(base::BindRepeating(&HeaderView::UpdateUI,
+                                                      base::Unretained(this))) {
     views::BoxLayout* layout =
         SetLayoutManager(std::make_unique<views::BoxLayout>(
             views::BoxLayout::Orientation::kHorizontal));
@@ -51,6 +57,7 @@ class OmniboxRowView::HeaderView : public views::View,
 
     header_toggle_button_ =
         AddChildView(views::CreateVectorToggleImageButton(this));
+    mouse_enter_exit_handler_.ObserveMouseEnterExitOn(header_toggle_button_);
     views::InstallCircleHighlightPathGenerator(header_toggle_button_);
 
     header_toggle_button_focus_ring_ =
@@ -241,6 +248,9 @@ class OmniboxRowView::HeaderView : public views::View,
   // A pref change registrar for toggling the toggle button's state. This is
   // needed because the preference state can change through multiple UIs.
   PrefChangeRegistrar pref_change_registrar_;
+
+  // Keeps track of mouse-enter and mouse-exit events of child Views.
+  OmniboxMouseEnterExitHandler mouse_enter_exit_handler_;
 };
 
 OmniboxRowView::OmniboxRowView(size_t line,