Merged the context menu in apps grid view and shelf in tablet mode

This cl did the following things: (1) AppsGridContextMenu now only shows in clamshell mode. In tablet mode, we appended the sort submenu to ShelfContextMenu. The additional submenu still only shows in AppsGridView. (2) Removed AppsGridCommandId enum in AppsGridContextMenu to use the CommandId in ash/public/cpp/app_menu_constants.h. The change is to remove duplication of the command ids with same usages. Bug: 1304865 Change-Id: I3d2c8393cfd18891d2fa86406e32340bc6fc546a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3523365 Reviewed-by: Xiaoqian Dai <xdai@chromium.org> Reviewed-by: Toni Barzic <tbarzic@chromium.org> Commit-Queue: Wen-Chien Wang <wcwang@chromium.org> Cr-Commit-Position: refs/heads/main@{#984963}
chromium · Mar 24, 2022 · 00c6a1e · 00c6a1e
1 parent 3f446c8
commit 00c6a1e
Show file tree

Hide file tree

Showing 9 changed files with 148 additions and 26 deletions.
diff --git a/ash/app_list/views/app_list_toast_container_view.cc b/ash/app_list/views/app_list_toast_container_view.cc
@@ -58,8 +58,12 @@ AppListToastContainerView::AppListToastContainerView(
           views::FlexSpecification(views::MinimumFlexSizeRule::kPreferred,
                                    views::MaximumFlexSizeRule::kPreferred));
 
-  context_menu_ = std::make_unique<AppsGridContextMenu>();
-  set_context_menu_controller(context_menu_.get());
+  if (!tablet_mode_) {
+    // `context_menu_` is only set in clamshell mode. The sort options in tablet
+    // mode are handled in RootWindowController with ShelfContextMenuModel.
+    context_menu_ = std::make_unique<AppsGridContextMenu>();
+    set_context_menu_controller(context_menu_.get());
+  }
 }
 
 AppListToastContainerView::~AppListToastContainerView() {

diff --git a/ash/app_list/views/apps_grid_context_menu.cc b/ash/app_list/views/apps_grid_context_menu.cc
@@ -8,6 +8,7 @@
 #include "ash/app_list/model/app_list_model.h"
 #include "ash/constants/ash_features.h"
 #include "ash/public/cpp/app_list/app_list_model_delegate.h"
+#include "ash/public/cpp/app_menu_constants.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -32,11 +33,11 @@ void AppsGridContextMenu::Cancel() {
 
 void AppsGridContextMenu::ExecuteCommand(int command_id, int event_flags) {
   switch (command_id) {
-    case AppsGridCommandId::kReorderByNameAlphabetical:
+    case REORDER_BY_NAME_ALPHABETICAL:
       AppListModelProvider::Get()->model()->delegate()->RequestAppListSort(
           AppListSortOrder::kNameAlphabetical);
       break;
-    case AppsGridCommandId::kReorderByColor:
+    case REORDER_BY_COLOR:
       AppListModelProvider::Get()->model()->delegate()->RequestAppListSort(
           AppListSortOrder::kColor);
       break;
@@ -79,13 +80,13 @@ void AppsGridContextMenu::BuildMenuModel() {
   context_menu_model_->AddTitle(l10n_util::GetStringUTF16(
       IDS_ASH_LAUNCHER_APPS_GRID_CONTEXT_MENU_REORDER_TITLE));
   context_menu_model_->AddItemWithIcon(
-      AppsGridCommandId::kReorderByNameAlphabetical,
+      REORDER_BY_NAME_ALPHABETICAL,
       l10n_util::GetStringUTF16(
           IDS_ASH_LAUNCHER_APPS_GRID_CONTEXT_MENU_REORDER_BY_NAME),
       ui::ImageModel::FromVectorIcon(kSortAlphabeticalIcon,
                                      ui::kColorAshSystemUIMenuIcon));
   context_menu_model_->AddItemWithIcon(
-      AppsGridCommandId::kReorderByColor,
+      REORDER_BY_COLOR,
       l10n_util::GetStringUTF16(
           IDS_ASH_LAUNCHER_APPS_GRID_CONTEXT_MENU_REORDER_BY_COLOR),
       ui::ImageModel::FromVectorIcon(kSortColorIcon,

diff --git a/ash/app_list/views/apps_grid_context_menu.h b/ash/app_list/views/apps_grid_context_menu.h
@@ -24,21 +24,6 @@ namespace ash {
 class ASH_EXPORT AppsGridContextMenu : public ui::SimpleMenuModel::Delegate,
                                        public views::ContextMenuController {
  public:
-  // List of command id used in apps grid context menu.
-  enum AppsGridCommandId {
-    // Command Id that contains a submenu with app name reorder options.
-    kReorderByName,
-
-    // Command that will sort the name in alphabetical order.
-    kReorderByNameAlphabetical,
-
-    // Command that will sort the name in reverse alphabetical order.
-    kReorderByNameReverseAlphabetical,
-
-    // Command that will sort by icon color in rainbow order.
-    kReorderByColor
-  };
-
   AppsGridContextMenu();
   AppsGridContextMenu(const AppsGridContextMenu&) = delete;
   AppsGridContextMenu& operator=(const AppsGridContextMenu&) = delete;

diff --git a/ash/app_list/views/apps_grid_view.cc b/ash/app_list/views/apps_grid_view.cc
@@ -369,8 +369,12 @@ AppsGridView::AppsGridView(AppListA11yAnnouncer* a11y_announcer,
         l10n_util::GetStringUTF16(IDS_ALL_APPS_INDICATOR));
   }
 
-  context_menu_ = std::make_unique<AppsGridContextMenu>();
-  set_context_menu_controller(context_menu_.get());
+  if (!IsTabletMode()) {
+    // `context_menu_` is only set in clamshell mode. The sort options in tablet
+    // mode are handled in RootWindowController with ShelfContextMenuModel.
+    context_menu_ = std::make_unique<AppsGridContextMenu>();
+    set_context_menu_controller(context_menu_.get());
+  }
 }
 
 AppsGridView::~AppsGridView() {

diff --git a/ash/app_list/views/apps_grid_view_unittest.cc b/ash/app_list/views/apps_grid_view_unittest.cc
@@ -48,6 +48,7 @@
 #include "ash/public/cpp/shelf_item_delegate.h"
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/public/cpp/test/test_shelf_item_delegate.h"
+#include "ash/root_window_controller.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_view.h"
 #include "ash/shell.h"
@@ -5079,12 +5080,18 @@ TEST_P(AppsGridViewCardifiedStateTest,
   test_api_->LayoutToIdealBounds();
 }
 
-TEST_P(AppsGridViewAppSortTest, ContextMenuInTopLevelAppListSortAllApps) {
+TEST_P(AppsGridViewAppSortTest,
+       ContextMenuInTopLevelAppListSortAllAppsInClamshellMode) {
   // In this test, the sort algorithm is not tested. Instead, the context menu
   // that contains the options to sort is verified to be shown in apps grid
   // view. The menu option selecting is also simulated to ensure the sorting is
   // called. The actual sort algorithm is tested in
   // chrome/browser/ui/app_list/app_list_sort_browsertest.cc.
+
+  // The AppsGridContextMenu is only used in clamshell mode.
+  if (create_as_tablet_mode_)
+    return;
+
   model_->PopulateApps(1);
 
   AppsGridContextMenu* context_menu = apps_grid_view_->context_menu_for_test();
@@ -5128,6 +5135,75 @@ TEST_P(AppsGridViewAppSortTest, ContextMenuInTopLevelAppListSortAllApps) {
   EXPECT_FALSE(context_menu->IsMenuShowing());
 }
 
+TEST_P(AppsGridViewAppSortTest,
+       ContextMenuInTopLevelAppListSortAllAppsInTabletMode) {
+  // This test checks the context menu on root window in tablet mode.
+  if (!create_as_tablet_mode_)
+    return;
+
+  model_->PopulateApps(1);
+  EXPECT_EQ(AppListSortOrder::kCustom, model_->requested_sort_order());
+
+  // Get a point in `apps_grid_view_` that doesn't have an item on it.
+  const gfx::Point empty_space =
+      apps_grid_view_->GetBoundsInScreen().CenterPoint();
+
+  // Open the menu to test the alphabetical sort option.
+  SimulateRightClickOrLongPressAt(empty_space);
+  AppMenuModelAdapter* context_menu =
+      Shell::GetPrimaryRootWindowController()->menu_model_adapter_for_testing();
+  EXPECT_TRUE(context_menu->IsShowingMenu());
+
+  // Cache the current context menu view.
+  views::MenuItemView* reorder_submenu =
+      context_menu->root_for_testing()->GetSubmenu()->GetMenuItemAt(2);
+  ASSERT_TRUE(reorder_submenu->title() == u"Sort by");
+
+  // Open the Sort by submenu.
+  gfx::Point reorder_submenu_point =
+      reorder_submenu->GetBoundsInScreen().CenterPoint();
+  SimulateLeftClickOrTapAt(reorder_submenu_point);
+
+  views::MenuItemView* reorder_option =
+      reorder_submenu->GetSubmenu()->GetMenuItemAt(0);
+  ASSERT_TRUE(reorder_option->title() == u"Name");
+  gfx::Point reorder_option_point =
+      reorder_option->GetBoundsInScreen().CenterPoint();
+  SimulateLeftClickOrTapAt(reorder_option_point);
+
+  // Check that the apps are sorted and the menu is closed.
+  EXPECT_EQ(AppListSortOrder::kNameAlphabetical,
+            model_->requested_sort_order());
+  EXPECT_EQ(
+      Shell::GetPrimaryRootWindowController()->menu_model_adapter_for_testing(),
+      nullptr);
+
+  // Open the menu again to test the color sort option.
+  SimulateRightClickOrLongPressAt(empty_space);
+  context_menu =
+      Shell::GetPrimaryRootWindowController()->menu_model_adapter_for_testing();
+  EXPECT_TRUE(context_menu->IsShowingMenu());
+
+  reorder_submenu =
+      context_menu->root_for_testing()->GetSubmenu()->GetMenuItemAt(2);
+  ASSERT_TRUE(reorder_submenu->title() == u"Sort by");
+
+  // Open the Sort by submenu.
+  reorder_submenu_point = reorder_submenu->GetBoundsInScreen().CenterPoint();
+  SimulateLeftClickOrTapAt(reorder_submenu_point);
+
+  reorder_option = reorder_submenu->GetSubmenu()->GetMenuItemAt(1);
+  ASSERT_TRUE(reorder_option->title() == u"Color");
+  reorder_option_point = reorder_option->GetBoundsInScreen().CenterPoint();
+  SimulateLeftClickOrTapAt(reorder_option_point);
+
+  // Check that the apps are sorted and the menu is closed.
+  EXPECT_EQ(AppListSortOrder::kColor, model_->requested_sort_order());
+  EXPECT_EQ(
+      Shell::GetPrimaryRootWindowController()->menu_model_adapter_for_testing(),
+      nullptr);
+}
+
 TEST_P(AppsGridViewAppSortTest, ContextMenuOnFolderItemSortAllApps) {
   // In this test, the sort algorithm is not tested. Instead, the context menu
   // that contains the options to sort is verified to be shown on folder app

diff --git a/ash/public/cpp/app_menu_constants.h b/ash/public/cpp/app_menu_constants.h
@@ -59,7 +59,8 @@ enum CommandId {
   USE_LAUNCH_TYPE_WINDOW = 203,
   USE_LAUNCH_TYPE_TABBED_WINDOW = 204,
   USE_LAUNCH_TYPE_COMMAND_END,
-  // The reorder submenu options used by AppServiceContextMenu.
+  // The reorder options used by AppsGridContextMenu, ShelfContextMenuModel and
+  // AppServiceContextMenu.
   REORDER_SUBMENU = 300,
   REORDER_BY_NAME_ALPHABETICAL = 301,
   REORDER_BY_NAME_REVERSE_ALPHABETICAL = 302,

diff --git a/ash/root_window_controller.cc b/ash/root_window_controller.cc
@@ -13,6 +13,7 @@
 #include "ash/accessibility/chromevox/touch_exploration_manager.h"
 #include "ash/accessibility/ui/accessibility_panel_layout_manager.h"
 #include "ash/ambient/ambient_controller.h"
+#include "ash/app_list/app_list_controller_impl.h"
 #include "ash/app_menu/app_menu_model_adapter.h"
 #include "ash/constants/ash_constants.h"
 #include "ash/constants/ash_features.h"
@@ -28,9 +29,11 @@
 #include "ash/keyboard/virtual_keyboard_container_layout_manager.h"
 #include "ash/lock_screen_action/lock_screen_action_background_controller.h"
 #include "ash/login_status.h"
+#include "ash/public/cpp/app_menu_constants.h"
 #include "ash/public/cpp/shelf_types.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/public/cpp/window_properties.h"
+#include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/root_window_settings.h"
 #include "ash/scoped_animation_disabler.h"
 #include "ash/screen_util.h"
@@ -41,6 +44,7 @@
 #include "ash/shelf/shelf_widget.h"
 #include "ash/shelf/shelf_window_targeter.h"
 #include "ash/shell.h"
+#include "ash/strings/grit/ash_strings.h"
 #include "ash/system/status_area_widget.h"
 #include "ash/system/tray/tray_background_view.h"
 #include "ash/system/unified/unified_system_tray.h"
@@ -87,7 +91,9 @@
 #include "ui/aura/window_event_dispatcher.h"
 #include "ui/aura/window_observer.h"
 #include "ui/aura/window_tracker.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/base/models/menu_model.h"
+#include "ui/base/models/simple_menu_model.h"
 #include "ui/compositor/layer.h"
 #include "ui/display/types/display_constants.h"
 #include "ui/events/event_utils.h"
@@ -779,13 +785,41 @@ void RootWindowController::ShowContextMenu(const gfx::Point& location_in_screen,
                                  ->GetDisplayNearestWindow(GetRootWindow())
                                  .id();
 
+  const bool tablet_mode =
+      Shell::Get()->tablet_mode_controller()->InTabletMode();
   root_window_menu_model_adapter_ =
       std::make_unique<RootWindowMenuModelAdapter>(
           std::make_unique<ShelfContextMenuModel>(nullptr, display_id),
           wallpaper_widget_controller()->GetWidget(), source_type,
           base::BindOnce(&RootWindowController::OnMenuClosed,
                          base::Unretained(this)),
-          Shell::Get()->tablet_mode_controller()->InTabletMode());
+          tablet_mode);
+
+  // Appends the apps sort options in ShelfContextMenuModel in tablet mode. Note
+  // that the launcher UI is fullscreen in tablet mode, so the whole root window
+  // can be perceived by users to be part of the launcher.
+  if (features::IsLauncherAppSortEnabled() && tablet_mode &&
+      Shell::Get()->app_list_controller()->IsVisible(display_id)) {
+    ui::SimpleMenuModel* menu_model = root_window_menu_model_adapter_->model();
+    sort_apps_submenu_ = std::make_unique<ui::SimpleMenuModel>(
+        static_cast<ShelfContextMenuModel*>(menu_model));
+    sort_apps_submenu_->AddItemWithIcon(
+        REORDER_BY_NAME_ALPHABETICAL,
+        l10n_util::GetStringUTF16(
+            IDS_ASH_LAUNCHER_APPS_GRID_CONTEXT_MENU_REORDER_BY_NAME),
+        ui::ImageModel::FromVectorIcon(kSortAlphabeticalIcon));
+    sort_apps_submenu_->AddItemWithIcon(
+        REORDER_BY_COLOR,
+        l10n_util::GetStringUTF16(
+            IDS_ASH_LAUNCHER_APPS_GRID_CONTEXT_MENU_REORDER_BY_COLOR),
+        ui::ImageModel::FromVectorIcon(kSortColorIcon));
+    menu_model->AddSeparator(ui::NORMAL_SEPARATOR);
+    menu_model->AddSubMenuWithIcon(
+        REORDER_SUBMENU,
+        l10n_util::GetStringUTF16(
+            IDS_ASH_LAUNCHER_APPS_GRID_CONTEXT_MENU_REORDER_TITLE),
+        sort_apps_submenu_.get(), ui::ImageModel::FromVectorIcon(kReorderIcon));
+  }
 
   root_window_menu_model_adapter_->Run(
       gfx::Rect(location_in_screen, gfx::Size()),
@@ -1272,6 +1306,7 @@ RootWindowController::GetAccessibilityPanelLayoutManager() const {
 
 void RootWindowController::OnMenuClosed() {
   root_window_menu_model_adapter_.reset();
+  sort_apps_submenu_.reset();
   shelf_->UpdateVisibilityState();
 }
 

diff --git a/ash/root_window_controller.h b/ash/root_window_controller.h
@@ -26,6 +26,7 @@ class Point;
 
 namespace ui {
 class WindowTreeHost;
+class SimpleMenuModel;
 }
 
 namespace views {
@@ -231,6 +232,9 @@ class ASH_EXPORT RootWindowController {
   void CloseAmbientWidget(bool immediately);
 
   views::Widget* ambient_widget_for_testing() { return ambient_widget_.get(); }
+  AppMenuModelAdapter* menu_model_adapter_for_testing() {
+    return root_window_menu_model_adapter_.get();
+  }
 
   // Returns accessibility panel layout manager for this root window.
   AccessibilityPanelLayoutManager* GetAccessibilityPanelLayoutManagerForTest();
@@ -282,6 +286,7 @@ class ASH_EXPORT RootWindowController {
 
   // Manages the context menu.
   std::unique_ptr<AppMenuModelAdapter> root_window_menu_model_adapter_;
+  std::unique_ptr<ui::SimpleMenuModel> sort_apps_submenu_;
 
   std::unique_ptr<StackingController> stacking_controller_;
 

diff --git a/ash/shelf/shelf_context_menu_model.cc b/ash/shelf/shelf_context_menu_model.cc
@@ -10,6 +10,8 @@
 
 #include "ash/app_list/app_list_controller_impl.h"
 #include "ash/app_list/app_list_metrics.h"
+#include "ash/app_list/app_list_model_provider.h"
+#include "ash/app_list/model/app_list_model.h"
 #include "ash/constants/ash_features.h"
 #include "ash/constants/ash_pref_names.h"
 #include "ash/public/cpp/app_menu_constants.h"
@@ -121,6 +123,15 @@ void ShelfContextMenuModel::ExecuteCommand(int command_id, int event_flags) {
       DCHECK(ash::features::IsPersonalizationHubEnabled());
       NewWindowDelegate::GetPrimary()->OpenPersonalizationHub();
       break;
+    // Using reorder CommandId in ash/public/cpp/app_menu_constants.h
+    case REORDER_BY_NAME_ALPHABETICAL:
+      AppListModelProvider::Get()->model()->delegate()->RequestAppListSort(
+          AppListSortOrder::kNameAlphabetical);
+      break;
+    case REORDER_BY_COLOR:
+      AppListModelProvider::Get()->model()->delegate()->RequestAppListSort(
+          AppListSortOrder::kColor);
+      break;
     default:
       if (delegate_) {
         if (IsCommandIdAnAppLaunch(command_id)) {