[Launcher Sort] Fix toast/continue section blinking during app list sort

When an app list item has focus before app list reorder, the reorder
toast and continue section blinks during reorder animation. Please
see the crbug for detailed diagnosis.

This CL fixes such an issue by focusing on search box before starting
the reorder animation. In detail, this CL does the following things:

(1) In `AppListPresenterImpl` and `AppListBubbleView`, before triggering
apps grid reorder animation, move the focus to the search box.

(2) In order to avoid the a11y side effects brought by setting focus,
disable the search box from handling a11y events at the start of the
reorder animation. Re-enable when the reorder animation ends.

(3) Add a test helper class called `AppListFocusChangeListener` to
verify that when app list is reordered with focus on an item, the
focus only changes once from the item to the search box.

Bug: 1305378
Change-Id: I9d4d561cd4de64d1da899e5dcc79bf7b026d47e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3525913
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#983420}

ash/BUILD.gn

@@ -3043,6 +3043,8 @@ static_library("test_support") {
     "app_list/app_list_test_api.cc",
     "app_list/test/app_list_test_helper.cc",
     "app_list/test/app_list_test_helper.h",
+    "app_list/test/test_focus_change_listener.cc",
+    "app_list/test/test_focus_change_listener.h",
     "app_list/test_app_list_client.cc",
     "app_list/test_app_list_client.h",
     "app_list/views/assistant/assistant_test_api_impl.cc",

ash/app_list/app_list_presenter_impl.cc

@@ -9,6 +9,7 @@
 #include "ash/app_list/app_list_controller_impl.h"
 #include "ash/app_list/app_list_metrics.h"
 #include "ash/app_list/app_list_presenter_event_filter.h"
+#include "ash/app_list/app_list_util.h"
 #include "ash/app_list/app_list_view_delegate.h"
 #include "ash/app_list/views/app_list_main_view.h"
 #include "ash/app_list/views/apps_container_view.h"
@@ -41,6 +42,7 @@
 #include "ui/gfx/geometry/transform.h"
 #include "ui/gfx/geometry/transform_util.h"
 #include "ui/gfx/presentation_feedback.h"
+#include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/core/transient_window_manager.h"
 #include "ui/wm/public/activation_client.h"
@@ -343,11 +345,29 @@ void AppListPresenterImpl::UpdateForNewSortingOrder(
   if (!view_)
     return;
 
+  base::OnceClosure done_closure;
+  if (animate) {
+    // The search box should ignore a11y events during the reorder animation
+    // so that the announcement of app list reorder is made before that of
+    // focus change.
+    SetViewIgnoredForAccessibility(view_->search_box_view(), true);
+
+    // Focus on the search box before starting the reorder animation to prevent
+    // focus moving through app list items as they're being hidden for order
+    // update animation.
+    view_->search_box_view()->search_box()->RequestFocus();
+
+    done_closure =
+        base::BindOnce(&AppListPresenterImpl::OnAppListReorderAnimationDone,
+                       weak_ptr_factory_.GetWeakPtr());
+  }
+
   view_->app_list_main_view()
       ->contents_view()
       ->apps_container_view()
       ->UpdateForNewSortingOrder(new_order, animate,
-                                 std::move(update_position_closure));
+                                 std::move(update_position_closure),
+                                 std::move(done_closure));
 }
 
 bool AppListPresenterImpl::IsVisibleDeprecated() const {
@@ -688,4 +708,12 @@ void AppListPresenterImpl::SnapAppListBoundsToDisplayEdge() {
   window->SetBounds(bounds);
 }
 
+void AppListPresenterImpl::OnAppListReorderAnimationDone() {
+  if (!view_)
+    return;
+
+  // Re-enable the search box to handle a11y events.
+  SetViewIgnoredForAccessibility(view_->search_box_view(), false);
+}
+
 }  // namespace ash

ash/app_list/app_list_presenter_impl.h

@@ -215,6 +215,9 @@ class ASH_EXPORT AppListPresenterImpl
   // https://crbug.com/884889).
   void SnapAppListBoundsToDisplayEdge();
 
+  // Called when the reorder animation completes.
+  void OnAppListReorderAnimationDone();
+
   // Owns |this|.
   AppListControllerImpl* const controller_;
 
@@ -245,6 +248,8 @@ class ASH_EXPORT AppListPresenterImpl
   // Data we need to store for metrics.
   absl::optional<base::Time> last_open_time_;
   absl::optional<AppListShowSource> last_open_source_;
+
+  base::WeakPtrFactory<AppListPresenterImpl> weak_ptr_factory_{this};
 };
 
 }  // namespace ash

ash/app_list/app_list_presenter_unittest.cc

@@ -410,7 +410,8 @@ class AppListBubbleAndTabletTestBase : public AshTestBase {
               ->UpdateForNewSortingOrder(
                   order,
                   /*animate=*/true,
-                  /*update_position_closure=*/base::DoNothing())
+                  /*update_position_closure=*/base::DoNothing(),
+                  /*animation_done_closure=*/base::DoNothing())
         : GetAppListTestHelper()->GetBubbleView()->UpdateForNewSortingOrder(
               order,
               /*animate=*/true, /*update_position_closure=*/base::DoNothing());

ash/app_list/test/app_list_test_helper.cc

@@ -34,6 +34,20 @@
 
 namespace ash {
 
+namespace {
+
+// An app list should be either a bubble app list or a fullscreen app list.
+// Returns true if a bubble app list should be used under the current mode.
+bool ShouldUseBubbleAppList() {
+  // A bubble app list should be used only when:
+  // (1) It is in clamshell mode; and
+  // (2) The productivity launcher flag is enabled.
+  return !Shell::Get()->IsInTabletMode() &&
+         features::IsProductivityLauncherEnabled();
+}
+
+}  // namespace
+
 AppListTestHelper::AppListTestHelper() {
   // The app list controller is ready after Shell is created.
   app_list_controller_ = Shell::Get()->app_list_controller();
@@ -166,10 +180,9 @@ void AppListTestHelper::AddRecentApps(int num_apps) {
 }
 
 bool AppListTestHelper::IsInFolderView() {
-  if (!Shell::Get()->IsInTabletMode() &&
-      features::IsProductivityLauncherEnabled()) {
+  if (ShouldUseBubbleAppList())
     return GetBubbleView()->showing_folder_for_test();
-  }
+
   return GetAppListView()
       ->app_list_main_view()
       ->contents_view()
@@ -186,6 +199,9 @@ AppListView* AppListTestHelper::GetAppListView() {
 }
 
 SearchBoxView* AppListTestHelper::GetSearchBoxView() {
+  if (ShouldUseBubbleAppList())
+    return GetBubbleView()->search_box_view_for_test();
+
   return GetAppListView()->search_box_view();
 }
 

ash/app_list/test/test_focus_change_listener.cc

@@ -0,0 +1,39 @@
+// Copyright 2022 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 "ash/app_list/test/test_focus_change_listener.h"
+
+#include "ash/app_list/test/app_list_test_helper.h"
+#include "ash/app_list/views/apps_grid_view.h"
+
+namespace ash {
+
+namespace {
+static TestFocusChangeListener* g_instance = nullptr;
+}
+
+TestFocusChangeListener::TestFocusChangeListener(
+    views::FocusManager* focus_manager)
+    : focus_manager_(focus_manager) {
+  DCHECK(focus_manager_);
+  focus_manager_->AddFocusChangeListener(this);
+
+  DCHECK(!g_instance);
+  g_instance = this;
+}
+
+TestFocusChangeListener::~TestFocusChangeListener() {
+  focus_manager_->RemoveFocusChangeListener(this);
+
+  DCHECK_EQ(g_instance, this);
+  g_instance = nullptr;
+}
+
+// views::FocusChangeListener:
+void TestFocusChangeListener::OnDidChangeFocus(views::View* focused_before,
+                                               views::View* focused_now) {
+  ++focus_change_count_;
+}
+
+}  // namespace ash

ash/app_list/test/test_focus_change_listener.h

@@ -0,0 +1,37 @@
+// Copyright 2022 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 ASH_APP_LIST_TEST_TEST_FOCUS_CHANGE_LISTENER_H_
+#define ASH_APP_LIST_TEST_TEST_FOCUS_CHANGE_LISTENER_H_
+
+#include "ui/views/focus/focus_manager.h"
+
+namespace ash {
+
+// A helper class to observe focus changes on the specified focus manager.
+class TestFocusChangeListener : public views::FocusChangeListener {
+ public:
+  explicit TestFocusChangeListener(views::FocusManager* focus_manager);
+  TestFocusChangeListener(const TestFocusChangeListener&) = delete;
+  TestFocusChangeListener& operator=(const TestFocusChangeListener&) = delete;
+  ~TestFocusChangeListener() override;
+
+  // views::FocusChangeListener:
+  void OnWillChangeFocus(views::View* focused_before,
+                         views::View* focused_now) override {}
+  void OnDidChangeFocus(views::View* focused_before,
+                        views::View* focused_now) override;
+
+  int focus_change_count() { return focus_change_count_; }
+
+ private:
+  views::FocusManager* const focus_manager_;
+
+  // Records the count of focus changes.
+  int focus_change_count_ = 0;
+};
+
+}  // namespace ash
+
+#endif  // ASH_APP_LIST_TEST_TEST_FOCUS_CHANGE_LISTENER_H_

ash/app_list/views/app_list_bubble_apps_page.cc

@@ -399,9 +399,11 @@ void AppListBubbleAppsPage::DisableFocusForShowingActiveFolder(bool disabled) {
 void AppListBubbleAppsPage::UpdateForNewSortingOrder(
     const absl::optional<AppListSortOrder>& new_order,
     bool animate,
-    base::OnceClosure update_position_closure) {
+    base::OnceClosure update_position_closure,
+    base::OnceClosure animation_done_closure) {
   DCHECK(features::IsLauncherAppSortEnabled());
   DCHECK_EQ(animate, !update_position_closure.is_null());
+  DCHECK(!animation_done_closure || animate);
 
   // A11y announcements must happen before animations, otherwise "Search your
   // apps..." is spoken first because focus moves immediately to the search box.
@@ -416,10 +418,12 @@ void AppListBubbleAppsPage::UpdateForNewSortingOrder(
   }
 
   // Abort the old reorder animation if any before closure update to avoid data
-  // races on the the closure.
+  // races on closures.
   scrollable_apps_grid_view_->MaybeAbortReorderAnimation();
   DCHECK(!update_position_closure_);
   update_position_closure_ = std::move(update_position_closure);
+  DCHECK(!reorder_animation_done_closure_);
+  reorder_animation_done_closure_ = std::move(animation_done_closure);
 
   views::AnimationBuilder animation_builder =
       scrollable_apps_grid_view_->FadeOutVisibleItemsForReorder(
@@ -685,6 +689,9 @@ void AppListBubbleAppsPage::OnAppsGridViewFadeInAnimationEnded(bool aborted) {
 
 void AppListBubbleAppsPage::OnReorderAnimationEnded() {
   update_position_closure_.Reset();
+
+  if (reorder_animation_done_closure_)
+    std::move(reorder_animation_done_closure_).Run();
 }
 
 void AppListBubbleAppsPage::SlideViewIntoPosition(views::View* view,

ash/app_list/views/app_list_bubble_apps_page.h

@@ -97,7 +97,8 @@ class ASH_EXPORT AppListBubbleAppsPage : public views::View,
   void UpdateForNewSortingOrder(
       const absl::optional<AppListSortOrder>& new_order,
       bool animate,
-      base::OnceClosure update_position_closure);
+      base::OnceClosure update_position_closure,
+      base::OnceClosure animation_done_closure);
 
   // Scrolls to fully show the toast if the toast is partially shown or hidden
   // from the scroll view's perspective. Returns true if scrolling is performed.
@@ -195,6 +196,9 @@ class ASH_EXPORT AppListBubbleAppsPage : public views::View,
   // out animation when items are reordered.
   base::OnceClosure update_position_closure_;
 
+  // A closure that runs at the end of the reorder animation.
+  base::OnceClosure reorder_animation_done_closure_;
+
   base::WeakPtrFactory<AppListBubbleAppsPage> weak_factory_{this};
 };