[Multipaste] Implement the item deletion through the backspace key

This CL implements the feature that the selected item is able to be
deleted by pressing the backspace key. If no item is selected, pressing
the backspace key does nothing.

Bug: 1114773
Change-Id: I179050d4c15b9ef93b3b4da2e9bbc8395bf3a0fd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2390859Reviewed-by: David Black <dmblack@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805528}

ash/clipboard/clipboard_history_controller.cc

@@ -55,27 +55,45 @@ class ClipboardHistoryController::AcceleratorTarget
     : public ui::AcceleratorTarget {
  public:
   explicit AcceleratorTarget(ClipboardHistoryController* controller)
-      : controller_(controller) {}
+      : controller_(controller),
+        show_menu_combo_(
+            ui::Accelerator(/*key_code=*/ui::VKEY_V,
+                            /*modifiers=*/ui::EF_COMMAND_DOWN,
+                            /*key_state=*/ui::Accelerator::KeyState::PRESSED)),
+        delete_selected_(ui::Accelerator(
+            /*key_code=*/ui::VKEY_BACK,
+            /*modifiers=*/ui::EF_NONE,
+            /*key_state=*/ui::Accelerator::KeyState::PRESSED)) {}
   AcceleratorTarget(const AcceleratorTarget&) = delete;
   AcceleratorTarget& operator=(const AcceleratorTarget&) = delete;
   ~AcceleratorTarget() override = default;
 
   void Init() {
-    ui::Accelerator show_menu_combo(ui::VKEY_V, ui::EF_COMMAND_DOWN);
-    show_menu_combo.set_key_state(ui::Accelerator::KeyState::PRESSED);
     // Register, but no need to unregister because this outlives
     // AcceleratorController.
     Shell::Get()->accelerator_controller()->Register(
-        {show_menu_combo}, /*accelerator_target=*/this);
+        {show_menu_combo_}, /*accelerator_target=*/this);
+  }
+
+  void OnMenuShown() {
+    Shell::Get()->accelerator_controller()->Register(
+        {delete_selected_}, /*accelerator_target=*/this);
+  }
+
+  void OnMenuClosed() {
+    Shell::Get()->accelerator_controller()->Unregister(
+        {delete_selected_}, /*accelerator_target=*/this);
   }
 
  private:
   // ui::AcceleratorTarget:
   bool AcceleratorPressed(const ui::Accelerator& accelerator) override {
-    if (controller_->IsMenuShowing())
-      controller_->ExecuteSelectedMenuItem(accelerator.modifiers());
+    DCHECK(accelerator == show_menu_combo_ || accelerator == delete_selected_);
+
+    if (accelerator == show_menu_combo_)
+      HandleShowMenuCombo();
     else
-      controller_->ShowMenu();
+      HandleDeleteSelected();
     return true;
   }
 
@@ -83,8 +101,27 @@ class ClipboardHistoryController::AcceleratorTarget
     return controller_->IsMenuShowing() || controller_->CanShowMenu();
   }
 
+  void HandleShowMenuCombo() {
+    if (controller_->IsMenuShowing())
+      controller_->ExecuteSelectedMenuItem(show_menu_combo_.modifiers());
+    else
+      controller_->ShowMenu();
+  }
+
+  void HandleDeleteSelected() {
+    DCHECK(controller_->IsMenuShowing());
+    controller_->DeleteSelectedMenuItemIfAny();
+  }
+
   // The controller responsible for showing the Clipboard History menu.
   ClipboardHistoryController* const controller_;
+
+  // The accelerator to show the menu or execute the selected menu item.
+  const ui::Accelerator show_menu_combo_;
+
+  // The accelerator to delete the selected menu item. It is only registered
+  // while the menu is showing.
+  const ui::Accelerator delete_selected_;
 };
 
 // ClipboardHistoryController::MenuDelegate ------------------------------------
@@ -100,7 +137,7 @@ class ClipboardHistoryController::MenuDelegate
   // ui::SimpleMenuModel::Delegate:
   void ExecuteCommand(int command_id, int event_flags) override {
     if (command_id == ClipboardHistoryUtil::kDeleteCommandId) {
-      controller_->DeleteSelectedMenuItem();
+      controller_->DeleteSelectedMenuItemIfAny();
       return;
     }
 
@@ -155,8 +192,14 @@ void ClipboardHistoryController::ShowMenu() {
     return;
 
   context_menu_ = ClipboardHistoryMenuModelAdapter::Create(
-      menu_delegate_.get(), clipboard_history_.get());
+      menu_delegate_.get(),
+      base::BindRepeating(&ClipboardHistoryController::OnMenuClosed,
+                          base::Unretained(this)),
+      clipboard_history_.get());
   context_menu_->Run(CalculateAnchorRect());
+
+  DCHECK(IsMenuShowing());
+  accelerator_target_->OnMenuShown();
 }
 
 void ClipboardHistoryController::MenuOptionSelected(int command_id,
@@ -232,11 +275,14 @@ void ClipboardHistoryController::MenuOptionSelected(int command_id,
       base::TimeDelta::FromMilliseconds(200));
 }
 
-void ClipboardHistoryController::DeleteSelectedMenuItem() {
+void ClipboardHistoryController::DeleteSelectedMenuItemIfAny() {
   DCHECK(context_menu_);
   auto selected_command = context_menu_->GetSelectedMenuItemCommand();
 
-  DCHECK(selected_command.has_value());
+  // Return early if no item is selected.
+  if (!selected_command.has_value())
+    return;
+
   DCHECK_GE(*selected_command, ClipboardHistoryUtil::kFirstItemCommandId);
 
   clipboard_history_->RemoveItemForId(
@@ -285,4 +331,8 @@ gfx::Rect ClipboardHistoryController::CalculateAnchorRect() const {
                    gfx::Size());
 }
 
+void ClipboardHistoryController::OnMenuClosed() {
+  accelerator_target_->OnMenuClosed();
+}
+
 }  // namespace ash

ash/clipboard/clipboard_history_controller.h

@@ -54,6 +54,10 @@ class ASH_EXPORT ClipboardHistoryController {
     return nudge_controller_.get();
   }
 
+  const ClipboardHistoryMenuModelAdapter* context_menu_for_test() const {
+    return context_menu_.get();
+  }
+
  private:
   class AcceleratorTarget;
   class MenuDelegate;
@@ -63,11 +67,15 @@ class ASH_EXPORT ClipboardHistoryController {
   void ExecuteSelectedMenuItem(int event_flags);
   void MenuOptionSelected(int command_id, int event_flags);
 
-  // Delete the menu item being selected and its corresponding data.
-  void DeleteSelectedMenuItem();
+  // Delete the menu item being selected and its corresponding data. If no item
+  // is selected, do nothing.
+  void DeleteSelectedMenuItemIfAny();
 
   gfx::Rect CalculateAnchorRect() const;
 
+  // Called when the contextual menu is closed.
+  void OnMenuClosed();
+
   // The menu being shown.
   std::unique_ptr<ClipboardHistoryMenuModelAdapter> context_menu_;
   // Used to keep track of what is being copied to the clipboard.

ash/clipboard/clipboard_history_menu_model_adapter.cc

@@ -5,11 +5,9 @@
 #include "ash/clipboard/clipboard_history_menu_model_adapter.h"
 
 #include "ash/clipboard/clipboard_history.h"
-#include "ash/clipboard/clipboard_history_controller.h"
 #include "ash/clipboard/clipboard_history_util.h"
 #include "ash/clipboard/views/clipboard_history_item_view.h"
 #include "ash/public/cpp/clipboard_image_model_factory.h"
-#include "ash/shell.h"
 #include "ui/base/ui_base_types.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/controls/menu/menu_item_view.h"
@@ -23,9 +21,11 @@ namespace ash {
 std::unique_ptr<ClipboardHistoryMenuModelAdapter>
 ClipboardHistoryMenuModelAdapter::Create(
     ui::SimpleMenuModel::Delegate* delegate,
+    base::RepeatingClosure menu_closed_callback,
     const ClipboardHistory* clipboard_history) {
   return base::WrapUnique(new ClipboardHistoryMenuModelAdapter(
-      std::make_unique<ui::SimpleMenuModel>(delegate), clipboard_history));
+      std::make_unique<ui::SimpleMenuModel>(delegate),
+      std::move(menu_closed_callback), clipboard_history));
 }
 
 ClipboardHistoryMenuModelAdapter::~ClipboardHistoryMenuModelAdapter() = default;
@@ -70,9 +70,12 @@ void ClipboardHistoryMenuModelAdapter::Cancel() {
 base::Optional<int>
 ClipboardHistoryMenuModelAdapter::GetSelectedMenuItemCommand() const {
   DCHECK(root_view_);
+
+  // `root_view_` may be selected if no menu item is under selection.
   auto* menu_item = root_view_->GetMenuController()->GetSelectedMenuItem();
-  return menu_item ? base::make_optional(menu_item->GetCommand())
-                   : base::nullopt;
+  return menu_item && menu_item != root_view_
+             ? base::make_optional(menu_item->GetCommand())
+             : base::nullopt;
 }
 
 const ClipboardHistoryItem&
@@ -101,8 +104,9 @@ gfx::Rect ClipboardHistoryMenuModelAdapter::GetMenuBoundsInScreenForTest()
 
 ClipboardHistoryMenuModelAdapter::ClipboardHistoryMenuModelAdapter(
     std::unique_ptr<ui::SimpleMenuModel> model,
+    base::RepeatingClosure menu_closed_callback,
     const ClipboardHistory* clipboard_history)
-    : views::MenuModelAdapter(model.get()),
+    : views::MenuModelAdapter(model.get(), std::move(menu_closed_callback)),
       model_(std::move(model)),
       clipboard_history_(clipboard_history) {}
 

ash/clipboard/clipboard_history_menu_model_adapter.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "ash/ash_export.h"
 #include "base/optional.h"
 #include "base/unguessable_token.h"
 #include "ui/base/models/simple_menu_model.h"
@@ -28,10 +29,11 @@ class ClipboardHistory;
 
 // Used to show the clipboard history menu, which holds the last few things
 // copied.
-class ClipboardHistoryMenuModelAdapter : views::MenuModelAdapter {
+class ASH_EXPORT ClipboardHistoryMenuModelAdapter : views::MenuModelAdapter {
  public:
   static std::unique_ptr<ClipboardHistoryMenuModelAdapter> Create(
       ui::SimpleMenuModel::Delegate* delegate,
+      base::RepeatingClosure menu_closed_callback,
       const ClipboardHistory* clipboard_history);
 
   ClipboardHistoryMenuModelAdapter(const ClipboardHistoryMenuModelAdapter&) =
@@ -66,8 +68,10 @@ class ClipboardHistoryMenuModelAdapter : views::MenuModelAdapter {
   gfx::Rect GetMenuBoundsInScreenForTest() const;
 
  private:
-  ClipboardHistoryMenuModelAdapter(std::unique_ptr<ui::SimpleMenuModel> model,
-                                   const ClipboardHistory* clipboard_history);
+  ClipboardHistoryMenuModelAdapter(
+      std::unique_ptr<ui::SimpleMenuModel> model,
+      base::RepeatingClosure menu_closed_callback,
+      const ClipboardHistory* clipboard_history);
 
   // views::MenuModelAdapter:
   views::MenuItemView* AppendMenuItem(views::MenuItemView* menu,

chrome/browser/ui/ash/clipboard_history_browsertest.cc

@@ -8,6 +8,7 @@
 #include "ash/clipboard/clipboard_history.h"
 #include "ash/clipboard/clipboard_history_controller.h"
 #include "ash/clipboard/clipboard_history_item.h"
+#include "ash/clipboard/clipboard_history_menu_model_adapter.h"
 #include "ash/shell.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
@@ -76,6 +77,10 @@ ash::ClipboardHistoryController* GetClipboardHistoryController() {
   return ash::Shell::Get()->clipboard_history_controller();
 }
 
+const ash::ClipboardHistoryMenuModelAdapter* GetContextMenu() {
+  return GetClipboardHistoryController()->context_menu_for_test();
+}
+
 const std::list<ash::ClipboardHistoryItem>& GetClipboardItems() {
   return GetClipboardHistoryController()->history()->GetItems();
 }
@@ -84,6 +89,23 @@ gfx::Rect GetClipboardHistoryMenuBoundsInScreen() {
   return GetClipboardHistoryController()->GetMenuBoundsInScreenForTest();
 }
 
+bool VerifyClipboardTextData(const std::initializer_list<std::string>& texts) {
+  const std::list<ash::ClipboardHistoryItem>& items = GetClipboardItems();
+  if (items.size() != texts.size())
+    return false;
+
+  auto items_iter = items.cbegin();
+  const auto* texts_iter = texts.begin();
+  while (items_iter != items.cend() && texts_iter != texts.end()) {
+    if (items_iter->data().text() != *texts_iter)
+      return false;
+    ++items_iter;
+    ++texts_iter;
+  }
+
+  return true;
+}
+
 }  // namespace
 
 // Verify clipboard history's features in the multiprofile environment.
@@ -353,6 +375,52 @@ IN_PROC_BROWSER_TEST_F(ClipboardHistoryWithMultiProfileBrowserTest,
   Release(ui::KeyboardCode::VKEY_COMMAND);
 }
 
+// Verifies that the selected item should be deleted by the backspace key.
+IN_PROC_BROWSER_TEST_F(ClipboardHistoryWithMultiProfileBrowserTest,
+                       DeleteItemViaBackspaceKey) {
+  LoginUser(account_id1_);
+
+  // Write some things to the clipboard.
+  SetClipboardText("A");
+  SetClipboardText("B");
+  SetClipboardText("C");
+
+  // Show the menu.
+  ShowContextMenuViaAccelerator();
+  ASSERT_TRUE(GetClipboardHistoryController()->IsMenuShowing());
+  ASSERT_EQ(3, GetContextMenu()->GetMenuItemsCount());
+
+  // No item is selected. So pressing the backspace key has no effect.
+  PressAndRelease(ui::KeyboardCode::VKEY_BACK, ui::EF_NONE);
+  EXPECT_EQ(3, GetContextMenu()->GetMenuItemsCount());
+  EXPECT_TRUE(VerifyClipboardTextData({"C", "B", "A"}));
+
+  // Select the first menu item via key then delete it. Verify the menu and the
+  // clipboard history.
+  PressAndRelease(ui::KeyboardCode::VKEY_DOWN, ui::EF_NONE);
+  PressAndRelease(ui::KeyboardCode::VKEY_BACK, ui::EF_NONE);
+  EXPECT_EQ(2, GetContextMenu()->GetMenuItemsCount());
+  EXPECT_TRUE(VerifyClipboardTextData({"B", "A"}));
+
+  // Select the second menu item via key then delete it. Verify the menu and the
+  // clipboard history.
+  PressAndRelease(ui::KeyboardCode::VKEY_DOWN, ui::EF_NONE);
+  PressAndRelease(ui::KeyboardCode::VKEY_DOWN, ui::EF_NONE);
+  PressAndRelease(ui::KeyboardCode::VKEY_BACK, ui::EF_NONE);
+  EXPECT_EQ(1, GetContextMenu()->GetMenuItemsCount());
+  EXPECT_TRUE(VerifyClipboardTextData({"B"}));
+
+  // Delete the last item. Verify that the menu is closed.
+  PressAndRelease(ui::KeyboardCode::VKEY_DOWN, ui::EF_COMMAND_DOWN);
+  PressAndRelease(ui::KeyboardCode::VKEY_BACK, ui::EF_NONE);
+  EXPECT_FALSE(GetClipboardHistoryController()->IsMenuShowing());
+
+  // Trigger the accelerator of opening the clipboard history menu. No menu
+  // shows because of the empty history data.
+  ShowContextMenuViaAccelerator();
+  EXPECT_FALSE(GetClipboardHistoryController()->IsMenuShowing());
+}
+
 // Flaky: crbug.com/1123542
 IN_PROC_BROWSER_TEST_F(ClipboardHistoryWithMultiProfileBrowserTest,
                        DISABLED_ShouldPasteHistoryAsPlainText) {