MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/global_keyboard_shortcuts_mac.mm

 // Copyright (c) 2011 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 "chrome/browser/global_keyboard_shortcuts_mac.h"
 
 #import <AppKit/AppKit.h>
 
 #include "base/logging.h"
 #include "base/macros.h"
@@ skipping 15 matching lines @@
       result = item;
     }
 
     if (result)
       break;
   }
 
   return result;
 }
 
-}  // namespace
-
-// Basically, there are two kinds of keyboard shortcuts: Ones that should work
-// only if the tab contents is focused (BrowserKeyboardShortcut), and ones that
-// should work in all other cases (WindowKeyboardShortcut). In the latter case,
-// we differentiate between shortcuts that are checked before any other view
-// gets the chance to handle them (WindowKeyboardShortcut) or after all views
-// had a chance but did not handle the keypress event
-// (DelayedWindowKeyboardShortcut).
-
-const KeyboardShortcutData* GetWindowKeyboardShortcutTable(
-    size_t* num_entries) {
-  static const KeyboardShortcutData keyboard_shortcuts[] = {
-      // cmd   shift  cntrl  option
-      // ---   -----  -----  ------
-      // '{' / '}' characters should be matched earlier than virtual key code
-      // (therefore we can match alt-8 as '{' on german keyboards).
-      {true, false, false, false, 0, '}', IDC_SELECT_NEXT_TAB},
-      {true, false, false, false, 0, '{', IDC_SELECT_PREVIOUS_TAB},
-      {false, false, true, false, kVK_PageDown, 0, IDC_SELECT_NEXT_TAB},
-      {false, false, true, false, kVK_Tab, 0, IDC_SELECT_NEXT_TAB},
-      {false, false, true, false, kVK_PageUp, 0, IDC_SELECT_PREVIOUS_TAB},
-      {false, true, true, false, kVK_Tab, 0, IDC_SELECT_PREVIOUS_TAB},
-      // Cmd-0..8 select the Nth tab, with cmd-9 being "last tab".
-      {true, false, false, false, kVK_ANSI_1, 0, IDC_SELECT_TAB_0},
-      {true, false, false, false, kVK_ANSI_Keypad1, 0, IDC_SELECT_TAB_0},
-      {true, false, false, false, kVK_ANSI_2, 0, IDC_SELECT_TAB_1},
-      {true, false, false, false, kVK_ANSI_Keypad2, 0, IDC_SELECT_TAB_1},
-      {true, false, false, false, kVK_ANSI_3, 0, IDC_SELECT_TAB_2},
-      {true, false, false, false, kVK_ANSI_Keypad3, 0, IDC_SELECT_TAB_2},
-      {true, false, false, false, kVK_ANSI_4, 0, IDC_SELECT_TAB_3},
-      {true, false, false, false, kVK_ANSI_Keypad4, 0, IDC_SELECT_TAB_3},
-      {true, false, false, false, kVK_ANSI_5, 0, IDC_SELECT_TAB_4},
-      {true, false, false, false, kVK_ANSI_Keypad5, 0, IDC_SELECT_TAB_4},
-      {true, false, false, false, kVK_ANSI_6, 0, IDC_SELECT_TAB_5},
-      {true, false, false, false, kVK_ANSI_Keypad6, 0, IDC_SELECT_TAB_5},
-      {true, false, false, false, kVK_ANSI_7, 0, IDC_SELECT_TAB_6},
-      {true, false, false, false, kVK_ANSI_Keypad7, 0, IDC_SELECT_TAB_6},
-      {true, false, false, false, kVK_ANSI_8, 0, IDC_SELECT_TAB_7},
-      {true, false, false, false, kVK_ANSI_Keypad8, 0, IDC_SELECT_TAB_7},
-      {true, false, false, false, kVK_ANSI_9, 0, IDC_SELECT_LAST_TAB},
-      {true, false, false, false, kVK_ANSI_Keypad9, 0, IDC_SELECT_LAST_TAB},
-      {true, true, false, false, kVK_ANSI_M, 0, IDC_SHOW_AVATAR_MENU},
-      {true, false, false, true, kVK_ANSI_L, 0, IDC_SHOW_DOWNLOADS},
-  };
-
-  *num_entries = arraysize(keyboard_shortcuts);
-
-  return keyboard_shortcuts;
-}
-
-const KeyboardShortcutData* GetDelayedWindowKeyboardShortcutTable(
-    size_t* num_entries) {
-  static const KeyboardShortcutData keyboard_shortcuts[] = {
-    //cmd   shift  cntrl  option
-    //---   -----  -----  ------
-    {false, false, false, false, kVK_Escape,        0, IDC_STOP},
-  };
-
-  *num_entries = arraysize(keyboard_shortcuts);
-
-  return keyboard_shortcuts;
-}
-
-const KeyboardShortcutData* GetBrowserKeyboardShortcutTable(
-    size_t* num_entries) {
-  static const KeyboardShortcutData keyboard_shortcuts[] = {
-    //cmd   shift  cntrl  option
-    //---   -----  -----  ------
-    {true,  false, false, false, kVK_LeftArrow,    0,   IDC_BACK},
-    {true,  false, false, false, kVK_RightArrow,   0,   IDC_FORWARD},
-    {false, false, false, false, kVK_Delete,       0,   IDC_BACKSPACE_BACK},
-    {false, true,  false, false, kVK_Delete,       0,   IDC_BACKSPACE_FORWARD},
-    {true,  true,  false, false, 0,                'c', IDC_DEV_TOOLS_INSPECT},
-  };
-
-  *num_entries = arraysize(keyboard_shortcuts);
-
-  return keyboard_shortcuts;
-}
-
-static bool MatchesEventForKeyboardShortcut(
-    const KeyboardShortcutData& shortcut,
-    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
-    int vkey_code, unichar key_char) {
+bool MatchesEventForKeyboardShortcut(const KeyboardShortcutData& shortcut,
+                                     bool command_key,
+                                     bool shift_key,
+                                     bool cntrl_key,
+                                     bool opt_key,
+                                     int vkey_code,
+                                     unichar key_char) {
   // Expects that one of |key_char| or |vkey_code| is 0.
   DCHECK((shortcut.key_char == 0) ^ (shortcut.vkey_code == 0));
   if (shortcut.key_char) {
     // Shortcuts that have a |key_char| and have |opt_key| set are mistakes,
     // since |opt_key| is not checked when there is a |key_char|.
     DCHECK(!shortcut.opt_key);
     // The given shortcut key is to be matched by a keyboard character.
     // In this case we ignore shift and opt (alt) key modifiers, because
     // the character may be generated by a combination with those keys.
     if (shortcut.command_key == command_key &&
         shortcut.cntrl_key == cntrl_key &&
         shortcut.key_char == key_char)
       return true;
   } else if (shortcut.vkey_code) {
     // The given shortcut key is to be matched by a virtual key code.
     if (shortcut.command_key == command_key &&
         shortcut.shift_key == shift_key &&
         shortcut.cntrl_key == cntrl_key &&
         shortcut.opt_key == opt_key &&
         shortcut.vkey_code == vkey_code)
       return true;
   } else {
     NOTREACHED();  // Shouldn't happen.
   }
   return false;
 }
 
-static int CommandForKeyboardShortcut(
-    const KeyboardShortcutData* (*get_keyboard_shortcut_table)(size_t*),
-    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
-    int vkey_code, unichar key_char) {
-
+int CommandForKeyboardShortcut(const std::vector<KeyboardShortcutData>& table,
+                               bool command_key,
+                               bool shift_key,
+                               bool cntrl_key,
+                               bool opt_key,
+                               int vkey_code,
+                               unichar key_char) {
   // Scan through keycodes and see if it corresponds to one of the global
   // shortcuts on file.
   //
   // TODO(jeremy): Change this into a hash table once we get enough
   // entries in the array to make a difference.
   // (When turning this into a hash table, note that the current behavior
   // relies on the order of the table (see the comment for '{' / '}' above).
-  size_t num_shortcuts = 0;
-  const KeyboardShortcutData *it = get_keyboard_shortcut_table(&num_shortcuts);
-  for (size_t i = 0; i < num_shortcuts; ++i, ++it) {
-    if (MatchesEventForKeyboardShortcut(*it, command_key, shift_key, cntrl_key,
-                                        opt_key, vkey_code, key_char))
-      return it->chrome_command;
+  for (const auto& shortcut : table) {
+    if (MatchesEventForKeyboardShortcut(shortcut, command_key, shift_key,
+                                        cntrl_key, opt_key, vkey_code,
+                                        key_char))
+      return shortcut.chrome_command;
   }
 
   return -1;
 }
 
+}  // namespace
+
+// MacViews: Lists shortcuts that are impossible to migrate to
+// accelerator_table.cc (crbug.com/25946) as well as those that should work when
+// Omnibox is in focus.

[tapted, 2016/11/14 22:44:49]

This doesn't sound right.

BrowserView::LoadAccelerators() should add everything from accelerator_table.cc
into the views::FocusManager, which is associated with the entire Widget. So
those accelerators should be consulted when the omnibox has focus. We shouldn't
need to duplicate the mapping that hooks into the native NSEvent event dispatch.

Accelerators are processed in Widget::OnKeyEvent, but only if the event wasn't
marked as processed. 

Is the problem that Textfield::OnKeyPressed(..) is saying the event has been
handled, when it should not?

[themblsha, 2016/11/15 11:43:33]

No. The Views code doesn't see the event at all.

This is what happens when you press Cmd+1 while Omnibox is in focus:

1. -[NativeWidgetMacNSWindow performKeyEquivalent:]
  This is where the Cocoa and current MacViews implementation handle commands
  using global_keyboard_shortcuts_mac.mm.

2. -[BridgedContentView keyDown:]
  If the -performKeyEquivalent: returned NO the -keyDown: is called and it
  attempts to translate the key event to the Cocoa Text System command using
  -interpretKeyEvents:.

  * -[NSView interpretKeyEvents:]
    * -[NSTextInputContext handleEvent:]
      * -[NSTextInputContext
_handleEvent:options:allowingSyntheticEvent:completionHandler:]
        Internally consults NSKeyBindingManager and does some TSM stuff. Quite a
lot of code there.

-[BridgedContentView keyUp:] is not called in this case.

You could test by yourself: make sure the GetWindowKeyboardShortcutTable,
GetDelayedWindowKeyboardShortcutTable and GetBrowserKeyboardShortcutTable tables
are all empty, focus the Omnibox and try pressing Cmd+1/2/3 and see that nothing
happens.

How about trying to commit this CL as is and try to improve accelerator handling
separately?

+//
+// Cocoa: see comment in global_keyboard_shortcuts_cocoa_mac.mm.
+const std::vector<KeyboardShortcutData>& GetWindowKeyboardShortcutTable() {
+  CR_DEFINE_STATIC_LOCAL(std::vector<KeyboardShortcutData>, result, ({
+      // cmd   shift  cntrl  option
+      // ---   -----  -----  ------
+      // '{' / '}' characters should be matched earlier than virtual key code
+      // (therefore we can match alt-8 as '{' on german keyboards).
+      {true, false, false, false, 0, '}', IDC_SELECT_NEXT_TAB},
+      {true, false, false, false, 0, '{', IDC_SELECT_PREVIOUS_TAB},
+      {false, false, true, false, kVK_PageDown, 0, IDC_SELECT_NEXT_TAB},
+      {false, false, true, false, kVK_Tab, 0, IDC_SELECT_NEXT_TAB},
+      {false, false, true, false, kVK_PageUp, 0, IDC_SELECT_PREVIOUS_TAB},
+      {false, true, true, false, kVK_Tab, 0, IDC_SELECT_PREVIOUS_TAB},
+      // Cmd-0..8 select the Nth tab, with cmd-9 being "last tab".
+      {true, false, false, false, kVK_ANSI_1, 0, IDC_SELECT_TAB_0},
+      {true, false, false, false, kVK_ANSI_Keypad1, 0, IDC_SELECT_TAB_0},
+      {true, false, false, false, kVK_ANSI_2, 0, IDC_SELECT_TAB_1},
+      {true, false, false, false, kVK_ANSI_Keypad2, 0, IDC_SELECT_TAB_1},
+      {true, false, false, false, kVK_ANSI_3, 0, IDC_SELECT_TAB_2},
+      {true, false, false, false, kVK_ANSI_Keypad3, 0, IDC_SELECT_TAB_2},
+      {true, false, false, false, kVK_ANSI_4, 0, IDC_SELECT_TAB_3},
+      {true, false, false, false, kVK_ANSI_Keypad4, 0, IDC_SELECT_TAB_3},
+      {true, false, false, false, kVK_ANSI_5, 0, IDC_SELECT_TAB_4},
+      {true, false, false, false, kVK_ANSI_Keypad5, 0, IDC_SELECT_TAB_4},
+      {true, false, false, false, kVK_ANSI_6, 0, IDC_SELECT_TAB_5},
+      {true, false, false, false, kVK_ANSI_Keypad6, 0, IDC_SELECT_TAB_5},
+      {true, false, false, false, kVK_ANSI_7, 0, IDC_SELECT_TAB_6},
+      {true, false, false, false, kVK_ANSI_Keypad7, 0, IDC_SELECT_TAB_6},
+      {true, false, false, false, kVK_ANSI_8, 0, IDC_SELECT_TAB_7},
+      {true, false, false, false, kVK_ANSI_Keypad8, 0, IDC_SELECT_TAB_7},
+      {true, false, false, false, kVK_ANSI_9, 0, IDC_SELECT_LAST_TAB},
+      {true, false, false, false, kVK_ANSI_Keypad9, 0, IDC_SELECT_LAST_TAB},
+      {true, true, false, false, kVK_ANSI_M, 0, IDC_SHOW_AVATAR_MENU},
+      {true, false, false, true, kVK_ANSI_L, 0, IDC_SHOW_DOWNLOADS},
+  }));
+  return result;
+}
+
 int CommandForWindowKeyboardShortcut(
     bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
     int vkey_code, unichar key_char) {
-  return CommandForKeyboardShortcut(GetWindowKeyboardShortcutTable,
+  return CommandForKeyboardShortcut(GetWindowKeyboardShortcutTable(),
                                     command_key, shift_key,
                                     cntrl_key, opt_key, vkey_code,
                                     key_char);
 }
 
 int CommandForDelayedWindowKeyboardShortcut(
     bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
     int vkey_code, unichar key_char) {
-  return CommandForKeyboardShortcut(GetDelayedWindowKeyboardShortcutTable,
+  return CommandForKeyboardShortcut(GetDelayedWindowKeyboardShortcutTable(),
                                     command_key, shift_key,
                                     cntrl_key, opt_key, vkey_code,
                                     key_char);
 }
 
 int CommandForBrowserKeyboardShortcut(
     bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
     int vkey_code, unichar key_char) {
-  return CommandForKeyboardShortcut(GetBrowserKeyboardShortcutTable,
+  return CommandForKeyboardShortcut(GetBrowserKeyboardShortcutTable(),
                                     command_key, shift_key,
                                     cntrl_key, opt_key, vkey_code,
                                     key_char);
 }
 
 int CommandForKeyEvent(NSEvent* event) {
   if ([event type] != NSKeyDown)
     return -1;
 
   // Look in menu.
@@ skipping 91 matching lines @@
       }
     }
 
     // opt/alt modifier is set (e.g. on german layout we want '{' for opt-8).
     if ([event modifierFlags] & NSAlternateKeyMask)
       return rawChar;
   }
 
   return noModifiersChar;
 }