[af] Do not clear autofill selection if no suggestion is selected

This fixes crbug.com/835019, in which the current suggestion may not be cleared when the mouse exits the popup area, by reusing the OnMouseExited handler implemented by AutofillPopupBaseView and overriden by AutofillPopupViewNativeViews as empty. The current implementation posts a task that will be a no-op if the mouse leaves the popup and there is no suggestion selected, so this is a safe change. Bug: 835019 Change-Id: Iee65e18e8d9c020e99ed2f0ed8d6643573ae6950 Reviewed-on: https://chromium-review.googlesource.com/1070788 Commit-Queue: Fabio Tirelo <ftirelo@chromium.org> Reviewed-by: Rachel Blum <groby@chromium.org> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Cr-Commit-Position: refs/heads/master@{#564070}
chromium · Jun 4, 2018 · 4a696ac · 4a696ac
1 parent 42930fc
commit 4a696ac
Show file tree

Hide file tree

Showing 13 changed files with 44 additions and 1 deletion.
diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
@@ -281,6 +281,10 @@ void AutofillPopupControllerImpl::SelectionCleared() {
   SetSelectedLine(base::nullopt);
 }
 
+bool AutofillPopupControllerImpl::HasSelection() const {
+  return selected_line_.has_value();
+}
+
 void AutofillPopupControllerImpl::AcceptSuggestion(int index) {
   const autofill::Suggestion& suggestion = suggestions_[index];
   delegate_->DidAcceptSuggestion(suggestion.value, suggestion.frontend_id,

diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.h b/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
@@ -82,6 +82,7 @@ class AutofillPopupControllerImpl : public AutofillPopupController {
   void SetSelectionAtPoint(const gfx::Point& point) override;
   bool AcceptSelectedLine() override;
   void SelectionCleared() override;
+  bool HasSelection() const override;
   gfx::Rect popup_bounds() const override;
   gfx::NativeView container_view() override;
   const gfx::RectF& element_bounds() const override;

diff --git a/chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc b/chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc
@@ -39,6 +39,7 @@ class TestAutofillPopupViewDelegate : public AutofillPopupViewDelegate {
   void SetSelectionAtPoint(const gfx::Point& point) override {}
   bool AcceptSelectedLine() override { return true; }
   void SelectionCleared() override {}
+  bool HasSelection() const override { return false; }
   gfx::Rect popup_bounds() const override { return gfx::Rect(0, 0, 100, 100); }
   gfx::NativeView container_view() override { return container_view_; }
   const gfx::RectF& element_bounds() const override { return element_bounds_; }

diff --git a/chrome/browser/ui/autofill/autofill_popup_view_delegate.h b/chrome/browser/ui/autofill/autofill_popup_view_delegate.h
@@ -44,6 +44,9 @@ class AutofillPopupViewDelegate {
   // out of the popup bounds.
   virtual void SelectionCleared() = 0;
 
+  // Returns true if any of the suggestions is selected.
+  virtual bool HasSelection() const = 0;
+
   // The actual bounds of the popup.
   virtual gfx::Rect popup_bounds() const = 0;
 

diff --git a/chrome/browser/ui/cocoa/autofill/credit_card_autofill_touch_bar_controller_unittest.mm b/chrome/browser/ui/cocoa/autofill/credit_card_autofill_touch_bar_controller_unittest.mm
@@ -47,6 +47,7 @@
   MOCK_METHOD1(SetSelectionAtPoint, void(const gfx::Point& point));
   MOCK_METHOD0(AcceptSelectedLine, bool());
   MOCK_METHOD0(SelectionCleared, void());
+  MOCK_CONST_METHOD0(HasSelection, bool());
   MOCK_CONST_METHOD0(popup_bounds, gfx::Rect());
   MOCK_METHOD0(container_view, gfx::NativeView());
   MOCK_CONST_METHOD0(element_bounds, const gfx::RectF&());

diff --git a/chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm b/chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm
@@ -57,6 +57,7 @@ void Hide() override {}
   void SetSelectionAtPoint(const gfx::Point&) override {}
   bool AcceptSelectedLine() override { return true; }
   void SelectionCleared() override {}
+  bool HasSelection() const override { return password_selected(); }
   gfx::Rect popup_bounds() const override { return popup_bounds_; }
   MOCK_METHOD0(container_view, gfx::NativeView());
   MOCK_CONST_METHOD0(element_bounds, gfx::RectF&());

diff --git a/chrome/browser/ui/passwords/password_generation_popup_controller_impl.cc b/chrome/browser/ui/passwords/password_generation_popup_controller_impl.cc
@@ -252,6 +252,10 @@ void PasswordGenerationPopupControllerImpl::SelectionCleared() {
   PasswordSelected(false);
 }
 
+bool PasswordGenerationPopupControllerImpl::HasSelection() const {
+  return password_selected();
+}
+
 gfx::NativeView PasswordGenerationPopupControllerImpl::container_view() {
   return controller_common_.container_view;
 }

diff --git a/chrome/browser/ui/passwords/password_generation_popup_controller_impl.h b/chrome/browser/ui/passwords/password_generation_popup_controller_impl.h
@@ -92,6 +92,7 @@ class PasswordGenerationPopupControllerImpl
   void SetSelectionAtPoint(const gfx::Point& point) override;
   bool AcceptSelectedLine() override;
   void SelectionCleared() override;
+  bool HasSelection() const override;
   void PasswordAccepted() override;
   void OnSavedPasswordsLinkClicked() override;
   int GetMinimumWidth() override;

diff --git a/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc b/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
@@ -195,6 +195,10 @@ bool AutofillPopupBaseView::OnMouseDragged(const ui::MouseEvent& event) {
 }
 
 void AutofillPopupBaseView::OnMouseExited(const ui::MouseEvent& event) {
+  // There is no need to post a ClearSelection task if no row is selected.
+  if (!delegate_->HasSelection())
+    return;
+
   // Pressing return causes the cursor to hide, which will generate an
   // OnMouseExited event. Pressing return should activate the current selection
   // via AcceleratorPressed, so we need to let that run first.

diff --git a/chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc b/chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/views/autofill/autofill_popup_base_view.h"
 
+#include "base/macros.h"
+#include "base/run_loop.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view_delegate.h"
 #include "chrome/browser/ui/browser.h"
@@ -32,6 +34,8 @@ class MockAutofillPopupViewDelegate : public AutofillPopupViewDelegate {
   MOCK_METHOD1(SetSelectionAtPoint, void(const gfx::Point&));
   MOCK_METHOD0(AcceptSelectedLine, bool());
   MOCK_METHOD0(SelectionCleared, void());
+  MOCK_CONST_METHOD0(HasSelection, bool());
+
   // TODO(jdduke): Mock this method upon resolution of crbug.com/352463.
   MOCK_CONST_METHOD0(popup_bounds, gfx::Rect());
   MOCK_METHOD0(container_view, gfx::NativeView());
@@ -84,6 +88,8 @@ class AutofillPopupBaseViewTest : public InProcessBrowserTest {
   test::ScopedMacViewsBrowserMode views_mode_{true};
   testing::NiceMock<MockAutofillPopupViewDelegate> mock_delegate_;
   AutofillPopupBaseView* view_;
+
+  DISALLOW_COPY_AND_ASSIGN(AutofillPopupBaseViewTest);
 };
 
 // Flaky on Win and Linux.  http://crbug.com/376299
@@ -154,4 +160,20 @@ IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, CorrectBoundsTest) {
   EXPECT_EQ(expected_point, display_point);
 }
 
+IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, MouseExitedTest) {
+  for (bool has_selection : {true, false}) {
+    EXPECT_CALL(mock_delegate_, HasSelection()).WillOnce(Return(has_selection));
+    EXPECT_CALL(mock_delegate_, SelectionCleared())
+        .Times(has_selection ? 1 : 0);
+
+    ShowView();
+
+    ui::MouseEvent exit_event(ui::ET_MOUSE_EXITED, gfx::Point(), gfx::Point(),
+                              ui::EventTimeForNow(), 0, 0);
+    static_cast<views::View*>(view_)->OnMouseExited(exit_event);
+
+    base::RunLoop().RunUntilIdle();
+  }
+}
+
 }  // namespace autofill
diff --git a/chrome/browser/ui/views/autofill/autofill_popup_view_native_views.h b/chrome/browser/ui/views/autofill/autofill_popup_view_native_views.h
@@ -83,7 +83,6 @@ class AutofillPopupViewNativeViews : public AutofillPopupBaseView,
   // TODO(crbug.com/831603): Remove these overrides and the corresponding
   // methods in AutofillPopupBaseView once deprecation of
   // AutofillPopupViewViews is complete.
-  void OnMouseExited(const ui::MouseEvent& event) override {}
   void OnMouseMoved(const ui::MouseEvent& event) override {}
 
  private:

diff --git a/chrome/browser/ui/views/autofill/autofill_popup_view_native_views_unittest.cc b/chrome/browser/ui/views/autofill/autofill_popup_view_native_views_unittest.cc
@@ -60,6 +60,7 @@ class MockAutofillPopupController : public autofill::AutofillPopupController {
   MOCK_METHOD1(SetSelectionAtPoint, void(const gfx::Point& point));
   MOCK_METHOD0(AcceptSelectedLine, bool());
   MOCK_METHOD0(SelectionCleared, void());
+  MOCK_CONST_METHOD0(HasSelection, bool());
   MOCK_CONST_METHOD0(popup_bounds, gfx::Rect());
   MOCK_METHOD0(container_view, gfx::NativeView());
   MOCK_CONST_METHOD0(element_bounds, const gfx::RectF&());

diff --git a/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc b/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc
@@ -45,6 +45,7 @@ class MockAutofillPopupController : public AutofillPopupController {
   MOCK_METHOD1(SetSelectionAtPoint, void(const gfx::Point& point));
   MOCK_METHOD0(AcceptSelectedLine, bool());
   MOCK_METHOD0(SelectionCleared, void());
+  MOCK_CONST_METHOD0(HasSelection, bool());
   MOCK_CONST_METHOD0(popup_bounds, gfx::Rect());
   MOCK_METHOD0(container_view, gfx::NativeView());
   MOCK_CONST_METHOD0(element_bounds, const gfx::RectF&());