Keep search box active on close button click in bubble launcher

The existing peeking/fullscreen launcher deactivates the search box
after a click on its close button. For bubble launcher we want the
search box to stay active.

Refactor SearchBoxViewBase so that close button handling happens via
SearchBoxViewDelegate. This makes it similar to the back button and
assistant button. Provide separate implementations for the
AppListMainView (fullscreen launcher) and AppListBubbleView (bubble
launcher).

Move some tests out of SearchBoxViewTest since the behavior now
depends on which view is hosting the search box.

Bug: 1216082
Test: added to ash_unittests

Change-Id: If441778abf1e65867bfedaa71fe1b75242dc633f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2999304
Reviewed-by: Yulun Wu <yulunwu@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#897793}

ash/app_list/bubble/app_list_bubble_view.cc

@@ -193,6 +193,12 @@ void AppListBubbleView::QueryChanged(SearchBoxViewBase* sender) {
   view_delegate_->StartSearch(query);
 }
 
+void AppListBubbleView::CloseButtonPressed() {
+  // Activate and focus the search box.
+  search_box_view_->SetSearchBoxActive(true, /*event_type=*/ui::ET_UNKNOWN);
+  search_box_view_->ClearSearch();
+}
+
 void AppListBubbleView::OnSearchBoxKeyEvent(ui::KeyEvent* event) {
   // Nothing to do. Search box starts focused, and FocusManager handles arrow
   // key traversal from there.

ash/app_list/bubble/app_list_bubble_view.h

@@ -45,6 +45,7 @@ class ASH_EXPORT AppListBubbleView : public views::BubbleDialogDelegateView,
   void QueryChanged(SearchBoxViewBase* sender) override;
   void AssistantButtonPressed() override {}
   void BackButtonPressed() override {}
+  void CloseButtonPressed() override;
   void ActiveChanged(SearchBoxViewBase* sender) override {}
   void SearchBoxFocusChanged(SearchBoxViewBase* sender) override {}
   void OnSearchBoxKeyEvent(ui::KeyEvent* event) override;

ash/app_list/bubble/app_list_bubble_view_unittest.cc

@@ -114,6 +114,11 @@ class AppListBubbleViewTest : public AshTestBase {
     GetEventGenerator()->ReleaseKey(key, ui::EF_NONE);
   }
 
+  void ClickButton(views::Button* button) {
+    GetEventGenerator()->MoveMouseTo(button->GetBoundsInScreen().CenterPoint());
+    GetEventGenerator()->ClickLeftButton();
+  }
+
   base::test::ScopedFeatureList scoped_features_;
 };
 
@@ -191,6 +196,24 @@ TEST_F(AppListBubbleViewTest, SearchBoxShowsAssistantButton) {
   EXPECT_TRUE(view->close_button()->GetVisible());
 }
 
+TEST_F(AppListBubbleViewTest, SearchBoxCloseButton) {
+  ShowAppList();
+  PressAndReleaseKey(ui::VKEY_A);
+
+  // Close button is visible after typing text.
+  SearchBoxView* search_box_view = GetSearchBoxView();
+  EXPECT_TRUE(search_box_view->close_button()->GetVisible());
+  EXPECT_FALSE(search_box_view->search_box()->GetText().empty());
+
+  // Clicking the close button clears the search, but the search box is still
+  // focused/active.
+  ClickButton(search_box_view->close_button());
+  EXPECT_FALSE(search_box_view->close_button()->GetVisible());
+  EXPECT_TRUE(search_box_view->search_box()->GetText().empty());
+  EXPECT_TRUE(search_box_view->search_box()->HasFocus());
+  EXPECT_TRUE(search_box_view->is_search_box_active());
+}
+
 TEST_F(AppListBubbleViewTest, AppsPageShownByDefault) {
   ShowAppList();
 

ash/app_list/views/app_list_main_view.cc

@@ -268,4 +268,10 @@ void AppListMainView::BackButtonPressed() {
     app_list_view_->Dismiss();
 }
 
+void AppListMainView::CloseButtonPressed() {
+  // Deactivate the search box.
+  search_box_view_->SetSearchBoxActive(false, ui::ET_UNKNOWN);
+  search_box_view_->ClearSearch();
+}
+
 }  // namespace ash

ash/app_list/views/app_list_main_view.h

@@ -84,6 +84,7 @@ class ASH_EXPORT AppListMainView : public views::View,
   void QueryChanged(SearchBoxViewBase* sender) override;
   void AssistantButtonPressed() override;
   void BackButtonPressed() override;
+  void CloseButtonPressed() override;
   void ActiveChanged(SearchBoxViewBase* sender) override;
   void SearchBoxFocusChanged(SearchBoxViewBase* sender) override;
   void OnSearchBoxKeyEvent(ui::KeyEvent* event) override;

ash/app_list/views/app_list_main_view_unittest.cc

@@ -24,7 +24,10 @@
 #include "base/test/scoped_feature_list.h"
 #include "ui/compositor/layer.h"
 #include "ui/events/base_event_utils.h"
+#include "ui/events/keycodes/keyboard_codes_posix.h"
+#include "ui/events/types/event_type.h"
 #include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/test/button_test_api.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/view_model.h"
 #include "ui/views/widget/widget.h"
@@ -48,8 +51,11 @@ class AppListMainViewTest : public views::ViewsTestBase,
   void SetUp() override {
     AppListView::SetShortAnimationForTesting(true);
     views::ViewsTestBase::SetUp();
-    feature_list.InitWithFeatureState(app_list_features::kNewDragSpecInLauncher,
-                                      GetParam());
+    // Allow TEST_F for tests that don't need to be parameterized.
+    if (testing::UnitTest::GetInstance()->current_test_info()->value_param()) {
+      feature_list_.InitWithFeatureState(
+          app_list_features::kNewDragSpecInLauncher, GetParam());
+    }
 
     // Create, and show the app list is fullscreen apps grid state.
     delegate_ = std::make_unique<AppListTestViewDelegate>();
@@ -220,19 +226,51 @@ class AppListMainViewTest : public views::ViewsTestBase,
 
   bool IsPaginationPreviewActive() { return GetParam(); }
 
+  void PressKeyInSearchBox(ui::KeyboardCode key_code) {
+    ui::KeyEvent press(ui::ET_KEY_PRESSED, key_code, ui::EF_NONE);
+    search_box_view()->search_box()->OnKeyEvent(&press);
+  }
+
+  void ClickButton(views::Button* button) {
+    views::test::ButtonTestApi(button).NotifyClick(ui::MouseEvent(
+        ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), base::TimeTicks(),
+        ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
+  }
+
  protected:
   TestAppListColorProvider color_provider_;  // Needed by AppListView.
   AppListView* app_list_view_ = nullptr;  // Owned by native widget.
   std::unique_ptr<AppListTestViewDelegate> delegate_;
 
  private:
-  base::test::ScopedFeatureList feature_list;
+  base::test::ScopedFeatureList feature_list_;
 };
 
 INSTANTIATE_TEST_SUITE_P(All, AppListMainViewTest, testing::Bool());
 
 }  // namespace
 
+// Tests that the close button becomes invisible after close button is clicked.
+TEST_F(AppListMainViewTest, CloseButtonInvisibleAfterCloseButtonClicked) {
+  PressKeyInSearchBox(ui::VKEY_A);
+  ClickButton(search_box_view()->close_button());
+  EXPECT_FALSE(search_box_view()->close_button()->GetVisible());
+}
+
+// Tests that the search box becomes empty after close button is clicked.
+TEST_F(AppListMainViewTest, SearchBoxEmptyAfterCloseButtonClicked) {
+  PressKeyInSearchBox(ui::VKEY_A);
+  ClickButton(search_box_view()->close_button());
+  EXPECT_TRUE(search_box_view()->search_box()->GetText().empty());
+}
+
+// Tests that the search box is no longer active after close button is clicked.
+TEST_F(AppListMainViewTest, SearchBoxActiveAfterCloseButtonClicked) {
+  PressKeyInSearchBox(ui::VKEY_A);
+  ClickButton(search_box_view()->close_button());
+  EXPECT_FALSE(search_box_view()->is_search_box_active());
+}
+
 // Tests changing the AppListModel when switching profiles.
 TEST_P(AppListMainViewTest, ModelChanged) {
   delegate_->GetTestModel()->PopulateApps(kInitialItems);

ash/app_list/views/search_box_view.cc

@@ -28,7 +28,6 @@
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/search_box/search_box_constants.h"
 #include "ash/search_box/search_box_view_delegate.h"
-#include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/notreached.h"
@@ -239,12 +238,6 @@ int SearchBoxView::GetFocusRingSpacing() {
 
 void SearchBoxView::SetupCloseButton() {
   views::ImageButton* close = close_button();
-  close->SetCallback(base::BindRepeating(
-      [](SearchBoxView* view) {
-        view->SetSearchBoxActive(false, ui::ET_UNKNOWN);
-        view->ClearSearch();
-      },
-      this));
   close->SetImage(
       views::ImageButton::STATE_NORMAL,
       gfx::CreateVectorIcon(views::kIcCloseIcon, kSearchBoxIconSize,

ash/app_list/views/search_box_view_unittest.cc

@@ -42,7 +42,6 @@
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/textfield/textfield.h"
-#include "ui/views/test/button_test_api.h"
 #include "ui/views/test/widget_test.h"
 
 namespace ash {
@@ -200,6 +199,7 @@ class SearchBoxViewTest : public views::test::WidgetTest,
 
   void AssistantButtonPressed() override {}
   void BackButtonPressed() override {}
+  void CloseButtonPressed() override {}
   void ActiveChanged(SearchBoxViewBase* sender) override {}
   void SearchBoxFocusChanged(SearchBoxViewBase* sender) override {}
   void OnSearchBoxKeyEvent(ui::KeyEvent* event) override {}
@@ -236,36 +236,6 @@ TEST_F(SearchBoxViewTest, CloseButtonIVisibleInZeroStateSearchBox) {
   EXPECT_TRUE(view()->close_button()->GetVisible());
 }
 
-// Tests that the close button becomes invisible after close button is clicked.
-TEST_F(SearchBoxViewTest, CloseButtonInvisibleAfterCloseButtonClicked) {
-  KeyPress(ui::VKEY_A);
-  views::test::ButtonTestApi(view()->close_button())
-      .NotifyClick(ui::MouseEvent(
-          ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), base::TimeTicks(),
-          ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
-  EXPECT_FALSE(view()->close_button()->GetVisible());
-}
-
-// Tests that the search box becomes empty after close button is clicked.
-TEST_F(SearchBoxViewTest, SearchBoxEmptyAfterCloseButtonClicked) {
-  KeyPress(ui::VKEY_A);
-  views::test::ButtonTestApi(view()->close_button())
-      .NotifyClick(ui::MouseEvent(
-          ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), base::TimeTicks(),
-          ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
-  EXPECT_TRUE(view()->search_box()->GetText().empty());
-}
-
-// Tests that the search box is no longer active after close button is clicked.
-TEST_F(SearchBoxViewTest, SearchBoxActiveAfterCloseButtonClicked) {
-  KeyPress(ui::VKEY_A);
-  views::test::ButtonTestApi(view()->close_button())
-      .NotifyClick(ui::MouseEvent(
-          ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), base::TimeTicks(),
-          ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
-  EXPECT_FALSE(view()->is_search_box_active());
-}
-
 // Tests that the search box is inactive by default.
 TEST_F(SearchBoxViewTest, SearchBoxInactiveByDefault) {
   ASSERT_FALSE(view()->is_search_box_active());

ash/search_box/search_box_view_base.cc

@@ -289,7 +289,7 @@ SearchBoxViewBase::SearchBoxViewBase(SearchBoxViewDelegate* delegate)
   content_container_->AddChildView(assistant_button_);
 
   close_button_ = new SearchBoxImageButton(base::BindRepeating(
-      &SearchBoxViewBase::ClearSearch, base::Unretained(this)));
+      &SearchBoxViewDelegate::CloseButtonPressed, base::Unretained(delegate_)));
   content_container_->AddChildView(close_button_);
 }
 

ash/search_box/search_box_view_delegate.h

@@ -24,6 +24,10 @@ class SearchBoxViewDelegate {
   // Invoked when the back button has been pressed.
   virtual void BackButtonPressed() = 0;
 
+  // Invoked when the close button has been pressed. Implementations should
+  // clear the search box, but may or may not want to take focus.
+  virtual void CloseButtonPressed() = 0;
+
   // Invoked when search box active status has changed.
   virtual void ActiveChanged(SearchBoxViewBase* sender) = 0;
 

ash/shortcut_viewer/views/keyboard_shortcut_view.cc

@@ -340,6 +340,12 @@ void KeyboardShortcutView::BackButtonPressed() {
   search_box_view_->SetSearchBoxActive(false, ui::ET_UNKNOWN);
 }
 
+void KeyboardShortcutView::CloseButtonPressed() {
+  // After clicking search box close button focus the search box text field.
+  search_box_view_->search_box()->RequestFocus();
+  search_box_view_->ClearSearch();
+}
+
 void KeyboardShortcutView::ActiveChanged(ash::SearchBoxViewBase* sender) {
   const bool is_search_box_active = sender->is_search_box_active();
   is_search_box_empty_ = sender->IsSearchBoxTrimmedQueryEmpty();

ash/shortcut_viewer/views/keyboard_shortcut_view.h

@@ -56,6 +56,7 @@ class KeyboardShortcutView : public views::WidgetDelegateView,
   void QueryChanged(ash::SearchBoxViewBase* sender) override;
   void AssistantButtonPressed() override {}
   void BackButtonPressed() override;
+  void CloseButtonPressed() override;
   void ActiveChanged(ash::SearchBoxViewBase* sender) override;
   void SearchBoxFocusChanged(ash::SearchBoxViewBase* sender) override {}
   void OnSearchBoxKeyEvent(ui::KeyEvent* event) override {}

ash/shortcut_viewer/views/ksv_search_box_view.cc

@@ -8,7 +8,6 @@
 #include "ash/search_box/search_box_view_delegate.h"
 #include "ash/shortcut_viewer/strings/grit/shortcut_viewer_strings.h"
 #include "ash/shortcut_viewer/vector_icons/vector_icons.h"
-#include "base/bind.h"
 #include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -105,13 +104,6 @@ void KSVSearchBoxView::UpdateSearchBoxBorder() {
 
 void KSVSearchBoxView::SetupCloseButton() {
   views::ImageButton* close = close_button();
-  close->SetCallback(base::BindRepeating(
-      [](ash::SearchBoxViewBase* view) {
-        // Focus on the search box text field after clicking close button.
-        view->search_box()->RequestFocus();
-        view->ClearSearch();
-      },
-      this));
   close->SetHasInkDropActionOnClick(true);
   close->SetImage(
       views::ImageButton::STATE_NORMAL,