macOS: Fix [cmd + left arrow] not working in Omnibox.

After a recent refactor, [cmd + left arrow] would perform history navigation
instead of moving the cursor to the beginning of the Omnibox. This happens
because the processing for the hotkey [triggering history navigation] has higher
priority than the typical event handling flow.

The hotkeys [cmd + left arrow] and [cmd + right arrow] are special, and should
only trigger if the firstResponder is a WebContents, and the WebContents has
chosen not to process the event.

This CL removes handleExtraKeyboardShortcut:, which was dead code.

Bug: 852332
Change-Id: Id03f5e22337a79d9ca4abfd20d995a8cf8976b2a
Reviewed-on: https://chromium-review.googlesource.com/1099648Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567315}

chrome/browser/global_keyboard_shortcuts_mac.h

@@ -47,6 +47,11 @@ struct KeyboardShortcutData {
 // conflicting symbolic hotkey.
 int CommandForKeyEvent(NSEvent* event);
 
+// For legacy reasons and compatibility with Safari, some commands [e.g. cmd +
+// left arrow] are only allowed to fire if the firstResponder is a WebContents,
+// and the WebContents has chosen not to handle the event.
+int DelayedWebContentsCommandForKeyEvent(NSEvent* event);
+
 // Whether the event goes through the performKeyEquivalent: path and is handled
 // by CommandDispatcher.
 bool EventUsesPerformKeyEquivalent(NSEvent* event);

chrome/browser/global_keyboard_shortcuts_mac.mm

@@ -90,6 +90,17 @@ bool MatchesEventForKeyboardShortcut(const KeyboardShortcutData& shortcut,
          shortcut.opt_key == opt_key && shortcut.vkey_code == vkey_code;
 }
 
+const std::vector<KeyboardShortcutData>&
+GetDelayedShortcutsNotPresentInMainMenu() {
+  static base::NoDestructor<std::vector<KeyboardShortcutData>> keys({
+      // cmd   shift  cntrl  option vkeycode               command
+      //---   -----  -----  ------ --------               -------
+      {true, false, false, false, kVK_LeftArrow, IDC_BACK},
+      {true, false, false, false, kVK_RightArrow, IDC_FORWARD},
+  });
+  return *keys;
+}
+
 }  // namespace
 
 const std::vector<KeyboardShortcutData>& GetShortcutsNotPresentInMainMenu() {
@@ -125,9 +136,6 @@ const std::vector<KeyboardShortcutData>& GetShortcutsNotPresentInMainMenu() {
     {true, false, false, false, kVK_ANSI_Keypad9,       IDC_SELECT_LAST_TAB},
     {true, true,  false, false, kVK_ANSI_M,             IDC_SHOW_AVATAR_MENU},
     {true, false, false, true,  kVK_ANSI_L,             IDC_SHOW_DOWNLOADS},
-
-    {true,  false, false, false, kVK_LeftArrow,         IDC_BACK},
-    {true,  false, false, false, kVK_RightArrow,        IDC_FORWARD},
     {true,  true,  false, false, kVK_ANSI_C,            IDC_DEV_TOOLS_INSPECT},
   });
   // clang-format on
@@ -163,6 +171,31 @@ int CommandForKeyEvent(NSEvent* event) {
   return -1;
 }
 
+int DelayedWebContentsCommandForKeyEvent(NSEvent* event) {
+  DCHECK(event);
+  if ([event type] != NSKeyDown)
+    return -1;
+
+  // Look in secondary keyboard shortcuts.
+  NSUInteger modifiers = [event modifierFlags];
+  const bool cmdKey = (modifiers & NSCommandKeyMask) != 0;
+  const bool shiftKey = (modifiers & NSShiftKeyMask) != 0;
+  const bool cntrlKey = (modifiers & NSControlKeyMask) != 0;
+  const bool optKey = (modifiers & NSAlternateKeyMask) != 0;
+  const int keyCode = [event keyCode];
+
+  // Scan through keycodes and see if it corresponds to one of the non-menu
+  // shortcuts.
+  for (const auto& shortcut : GetDelayedShortcutsNotPresentInMainMenu()) {
+    if (MatchesEventForKeyboardShortcut(shortcut, cmdKey, shiftKey, cntrlKey,
+                                        optKey, keyCode)) {
+      return shortcut.chrome_command;
+    }
+  }
+
+  return -1;
+}
+
 // AppKit sends an event via performKeyEquivalent: if it has at least one of the
 // command or control modifiers, and is an NSKeyDown event. CommandDispatcher
 // supplements this by also sending event with the option modifier to

chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm

@@ -86,6 +86,36 @@ IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, SwitchTabsMac) {
   EXPECT_TRUE(tab_strip->IsTabSelected(0));
 }
 
+// Test that cmd + left arrow can be used for history navigation.
+IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, HistoryNavigation) {
+  TabStripModel* tab_strip = browser()->tab_strip_model();
+  NSWindow* ns_window = browser()->window()->GetNativeWindow();
+
+  GURL test_url = ui_test_utils::GetTestUrl(
+      base::FilePath(), base::FilePath(FILE_PATH_LITERAL("title1.html")));
+  ASSERT_NE(tab_strip->GetActiveWebContents()->GetLastCommittedURL(), test_url);
+
+  // Navigate the active tab to a dummy URL.
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+      browser(), test_url,
+      /*number_of_navigations=*/1);
+  ASSERT_EQ(tab_strip->GetActiveWebContents()->GetLastCommittedURL(), test_url);
+
+  // Focus the WebContents.
+  tab_strip->GetActiveWebContents()->Focus();
+
+  // Cmd + left arrow performs history navigation, but only after the
+  // WebContents chooses not to handle the event.
+  SendEvent(SynthesizeKeyEvent(ns_window, /*keydown=*/true, ui::VKEY_LEFT,
+                               NSCommandKeyMask));
+  while (true) {
+    if (tab_strip->GetActiveWebContents()->GetLastCommittedURL() != test_url)
+      break;
+    base::RunLoop().RunUntilIdle();
+  }
+  ASSERT_NE(tab_strip->GetActiveWebContents()->GetLastCommittedURL(), test_url);
+}
+
 // Test that common hotkeys for editing the omnibox work.
 IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, CopyPasteOmnibox) {
   BrowserWindow* window = browser()->window();
@@ -117,14 +147,22 @@ IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, CopyPasteOmnibox) {
   SendEvent(SynthesizeKeyEvent(ns_window, /*keydown=*/true, ui::VKEY_C,
                                NSCommandKeyMask));
 
-  // Right arrow moves to the end.
-  SendEvent(SynthesizeKeyEvent(ns_window, /*keydown=*/true, ui::VKEY_RIGHT,
+  // The first typed letter overrides the existing contents.
+  SendEvent(SynthesizeKeyEvent(ns_window, /*keydown=*/true, ui::VKEY_C,
                                /*flags=*/0));
+  SendEvent(SynthesizeKeyEvent(ns_window, /*keydown=*/true, ui::VKEY_D,
+                               /*flags=*/0));
+  ASSERT_EQ(omnibox_view->GetText(), base::ASCIIToUTF16("cd"));
+
+  // Cmd + left arrow moves to the beginning. It should not perform history
+  // navigation because the firstResponder is not a WebContents..
+  SendEvent(SynthesizeKeyEvent(ns_window, /*keydown=*/true, ui::VKEY_LEFT,
+                               NSCommandKeyMask));
 
   // Cmd+V pastes the contents.
   SendEvent(SynthesizeKeyEvent(ns_window, /*keydown=*/true, ui::VKEY_V,
                                NSCommandKeyMask));
-  EXPECT_EQ(omnibox_view->GetText(), base::ASCIIToUTF16("abab"));
+  EXPECT_EQ(omnibox_view->GetText(), base::ASCIIToUTF16("abcd"));
 }
 
 // Tests that the shortcut to reopen a previous tab works.

chrome/browser/ui/cocoa/browser_window_cocoa.mm

@@ -584,6 +584,8 @@ BrowserWindowCocoa::PreHandleKeyboardEvent(
     return Result::NOT_HANDLED;
 
   int command = CommandForKeyEvent(event.os_event);
+  if (command == -1)
+    command = DelayedWebContentsCommandForKeyEvent(event.os_event);
   return command == -1 ? Result::NOT_HANDLED : Result::NOT_HANDLED_IS_SHORTCUT;
 }
 

chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h

@@ -13,11 +13,6 @@
 // shortcuts and executing them with chrome::ExecuteCommand.
 @interface ChromeCommandDispatcherDelegate : NSObject<CommandDispatcherDelegate>
 
-// Checks if |event| is a keyboard shortcut listed in
-// global_keyboard_shortcuts_mac.h. If so, execute the associated command.
-// Returns YES if the event was handled.
-- (BOOL)handleExtraKeyboardShortcut:(NSEvent*)event window:(NSWindow*)window;
-
 @end
 
 #endif  // CHROME_BROWSER_UI_COCOA_CHROME_COMMAND_DISPATCHER_DELEGATE_H_

chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm

@@ -17,15 +17,6 @@
 
 @implementation ChromeCommandDispatcherDelegate
 
-- (BOOL)handleExtraKeyboardShortcut:(NSEvent*)event window:(NSWindow*)window {
-  int cmd = CommandForKeyEvent(event);
-  if (cmd == -1)
-    return false;
-
-  chrome::ExecuteCommand(chrome::FindBrowserWithWindow(window), cmd);
-  return true;
-}
-
 - (BOOL)eventHandledByExtensionCommand:(NSEvent*)event
                               priority:(ui::AcceleratorManager::HandlerPriority)
                                            priority {
@@ -100,7 +91,9 @@
   return NO;
 }
 
-- (BOOL)postPerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window {
+- (BOOL)postPerformKeyEquivalent:(NSEvent*)event
+                          window:(NSWindow*)window
+                    isRedispatch:(BOOL)isRedispatch {
   if ([self eventHandledByExtensionCommand:event
                                   priority:ui::AcceleratorManager::
                                                kNormalPriority]) {
@@ -108,6 +101,10 @@
   }
 
   int cmd = CommandForKeyEvent(event);
+
+  if (cmd == -1 && isRedispatch)
+    cmd = DelayedWebContentsCommandForKeyEvent(event);
+
   if (cmd != -1) {
     Browser* browser = chrome::FindBrowserWithWindow(window);
     if (browser) {

chrome/browser/ui/cocoa/chrome_event_processing_window.h

@@ -19,11 +19,6 @@
 @interface ChromeEventProcessingWindow
     : UnderlayOpenGLHostingWindow<CommandDispatchingWindow>
 
-// Checks if |event| is a window, delayed window, or browser keyboard shortcut.
-// (See global_keyboard_shortcuts_mac.h for details). If so, execute the
-// associated command. Returns YES if the event was handled.
-- (BOOL)handleExtraKeyboardShortcut:(NSEvent*)event;
-
 @end
 
 #endif  // CHROME_BROWSER_UI_COCOA_CHROME_EVENT_PROCESSING_WINDOW_H_

chrome/browser/ui/cocoa/chrome_event_processing_window.mm

@@ -34,11 +34,6 @@
   return self;
 }
 
-- (BOOL)handleExtraKeyboardShortcut:(NSEvent*)event {
-  return [commandDispatcherDelegate_ handleExtraKeyboardShortcut:event
-                                                          window:self];
-}
-
 // CommandDispatchingWindow implementation.
 
 - (void)setCommandHandler:(id<UserInterfaceItemCommandHandler>)commandHandler {

chrome/browser/ui/views/frame/browser_frame_mac.mm

@@ -148,6 +148,8 @@ content::KeyboardEventProcessingResult BrowserFrameMac::PreHandleKeyboardEvent(
   // NOT_HANDLED or NOT_HANDLED_IS_SHORTCUT.
   if (EventUsesPerformKeyEquivalent(event.os_event)) {
     int command_id = CommandForKeyEvent(event.os_event);
+    if (command_id == -1)
+      command_id = DelayedWebContentsCommandForKeyEvent(event.os_event);
     if (command_id != -1)
       return content::KeyboardEventProcessingResult::NOT_HANDLED_IS_SHORTCUT;
   }

ui/base/cocoa/command_dispatcher.h

@@ -88,7 +88,9 @@ UI_BASE_EXPORT
 // for more details on keyEquivalent consumer ordering.
 // |window| is the CommandDispatchingWindow that owns CommandDispatcher, not the
 // window of the event.
-- (BOOL)postPerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window;
+- (BOOL)postPerformKeyEquivalent:(NSEvent*)event
+                          window:(NSWindow*)window
+                    isRedispatch:(BOOL)isRedispatch;
 
 @end
 

ui/base/cocoa/command_dispatcher.mm

@@ -92,8 +92,11 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
   // We skip all steps before postPerformKeyEquivalent, since those were already
   // triggered on the first pass of the event.
   if (redispatchingEvent_) {
-    if ([delegate_ postPerformKeyEquivalent:event window:owner_])
+    if ([delegate_ postPerformKeyEquivalent:event
+                                     window:owner_
+                               isRedispatch:YES]) {
       return YES;
+    }
     return [[self bubbleParent] performKeyEquivalent:event];
   }
 
@@ -112,7 +115,7 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
 
   // If the firstResponder [e.g. omnibox] chose not to handle the keyEquivalent,
   // then give the delegate another chance to consume it.
-  if ([delegate_ postPerformKeyEquivalent:event window:owner_])
+  if ([delegate_ postPerformKeyEquivalent:event window:owner_ isRedispatch:NO])
     return YES;
 
   // Allow commands to "bubble up" to CommandDispatchers in parent windows, if