views: close menus when accelerators are pressed on Mac

This change is a fixed version of
<https://chromium-review.googlesource.com/c/1334147>.

This change:

1) Has MenuController consider command to be an accelerator key, along with
   control and alt, for the purpose of not searching for mnemonics;
2) Has MenuController stop handling a key press, and cancel the menu,
   when the key press looks like it is a window-level accelerator instead.

Testing:
1) Open the app menu
2) With the app menu still open, hit cmd-T
3) The app menu should dismiss
4) A new tab should appear

Bug: 885138
Change-Id: Ib9c9e1517ae9ad3f9ec55eadb9bdfe61762df4c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1742520
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Auto-Submit: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685346}

ui/views/controls/menu/menu_controller.cc

@@ -69,6 +69,38 @@ namespace views {
 
 namespace {
 
+#if defined(OS_MACOSX)
+bool AcceleratorShouldCancelMenu(const ui::Accelerator& accelerator) {
+  // Since AcceleratorShouldCancelMenu() is called quite early in key
+  // event handling, it is actually invoked for modifier keys themselves
+  // changing. In that case, the key code reflects that the modifier key is
+  // being pressed/released. We should never treat those presses as
+  // accelerators, so bail out here.
+  //
+  // Also, we have to check for VKEY_SHIFT here even though we don't check
+  // IsShiftDown() - otherwise this sequence of keypresses will dismiss the
+  // menu:
+  //   Press Cmd
+  //   Press Shift
+  // Which makes it impossible to use menus that have Cmd-Shift accelerators.
+  if (accelerator.key_code() == ui::VKEY_CONTROL ||
+      accelerator.key_code() == ui::VKEY_MENU ||  // aka Alt
+      accelerator.key_code() == ui::VKEY_COMMAND ||
+      accelerator.key_code() == ui::VKEY_SHIFT) {
+    return false;
+  }
+
+  // Using an accelerator on Mac closes any open menu. Note that Mac behavior is
+  // different between context menus (which block use of accelerators) and other
+  // types of menus, which close when an accelerator is sent and do repost the
+  // accelerator. In MacViews, this happens naturally because context menus are
+  // (modal) Cocoa menus and other menus are Views menus, which will go through
+  // this code path.
+  return accelerator.IsCtrlDown() || accelerator.IsAltDown() ||
+         accelerator.IsCmdDown();
+}
+#endif
+
 // When showing context menu on mouse down, the user might accidentally select
 // the menu item on the subsequent mouse up. To prevent this, we add the
 // following delay before the user is able to select an item.
@@ -1169,7 +1201,8 @@ ui::PostDispatchAction MenuController::OnWillDispatchKeyEvent(
     if (!IsEditableCombobox() && !event->stopped_propagation()) {
       // Do not check mnemonics if the Alt or Ctrl modifiers are pressed. For
       // example Ctrl+<T> is an accelerator, but <T> only is a mnemonic.
-      const int kKeyFlagsMask = ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN;
+      const int kKeyFlagsMask =
+          ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN | ui::EF_COMMAND_DOWN;
       const int flags = event->flags();
       if (exit_type() == ExitType::kNone && (flags & kKeyFlagsMask) == 0) {
         base::char16 c = event->GetCharacter();
@@ -1184,6 +1217,14 @@ ui::PostDispatchAction MenuController::OnWillDispatchKeyEvent(
   }
 
   ui::Accelerator accelerator(*event);
+
+#if defined(OS_MACOSX)
+  if (AcceleratorShouldCancelMenu(accelerator)) {
+    Cancel(ExitType::kAll);
+    return ui::POST_DISPATCH_PERFORM_DEFAULT;
+  }
+#endif
+
   ViewsDelegate::ProcessMenuAcceleratorResult result =
       ViewsDelegate::GetInstance()->ProcessAcceleratorWhileMenuShowing(
           accelerator);
@@ -1192,6 +1233,7 @@ ui::PostDispatchAction MenuController::OnWillDispatchKeyEvent(
     event->StopPropagation();
     return ui::POST_DISPATCH_NONE;
   }
+
   if (result == ViewsDelegate::ProcessMenuAcceleratorResult::CLOSE_MENU) {
     Cancel(ExitType::kAll);
     event->StopPropagation();

ui/views/controls/menu/menu_controller_unittest.cc

@@ -2338,5 +2338,33 @@ TEST_F(MenuControllerTest, AccessibilityEmitsSelectChildrenChanged) {
   DispatchKey(ui::VKEY_DOWN);
   EXPECT_EQ(observer.saw_selected_children_changed_, true);
 }
+
+#if defined(OS_MACOSX)
+// This test exercises a Mac-specific behavior, by which hotkeys using modifiers
+// cause menus to close and the hotkeys to be handled by the browser window.
+// This specific test case tries using cmd-ctrl-f, which normally means
+// "Fullscreen".
+TEST_F(MenuControllerTest, BrowserHotkeysCancelMenusAndAreRedispatched) {
+  menu_controller()->Run(owner(), nullptr, menu_item(), gfx::Rect(),
+                         MenuAnchorPosition::kTopLeft, false, false);
+
+  int options = ui::EF_COMMAND_DOWN;
+  ui::KeyEvent press_cmd(ui::ET_KEY_PRESSED, ui::VKEY_COMMAND, options);
+  menu_controller()->OnWillDispatchKeyEvent(&press_cmd);
+  EXPECT_TRUE(IsShowing());  // ensure the command press itself doesn't cancel
+
+  options |= ui::EF_CONTROL_DOWN;
+  ui::KeyEvent press_ctrl(ui::ET_KEY_PRESSED, ui::VKEY_CONTROL, options);
+  menu_controller()->OnWillDispatchKeyEvent(&press_ctrl);
+  EXPECT_TRUE(IsShowing());
+
+  ui::KeyEvent press_f(ui::ET_KEY_PRESSED, ui::VKEY_F, options);
+  menu_controller()->OnWillDispatchKeyEvent(&press_f);  // to pay respects
+  EXPECT_FALSE(IsShowing());
+  EXPECT_FALSE(press_f.handled());
+  EXPECT_FALSE(press_f.stopped_propagation());
+}
+#endif
+
 }  // namespace test
 }  // namespace views

ui/views/views_delegate.h

@@ -72,9 +72,9 @@ class VIEWS_EXPORT ViewsDelegate {
     // is needed and the menu should be kept open.
     LEAVE_MENU_OPEN,
 
-    // The accelerator was not handled. Menu should be closed and the
-    // accelerator will be reposted to be handled after the menu closes.
-    CLOSE_MENU
+    // The accelerator was not handled. The menu should be closed and event
+    // handling should stop for this event.
+    CLOSE_MENU,
   };
 
   virtual ~ViewsDelegate();