[Reland #1] macOS: Display [ctr + tab] and [ctr + shift + tab] as hotkeys for tab switching.

The first CL caused test failures because the browser_test was ignoring the
"shift" key when searching for menu items. The IDC_SELECT_NEXT_TAB and
IDC_SELECT_PREVIOUS_TAB only differ by the presence of "shift", so it cannot be
ignored.

> This matches other macOS application like Safari, Terminal, etc. All existing
> hotkeys will still work.
>
> This CL uses the same hotkey for "previous tab" that Safari and Terminal use,
> which is [ctr + shift + "Horizontal Tab"]. This also causes tests to pass.
> However, pressing that key combination actually generates [ctr + shift + "End of
> Medium"], which renderers in the Main Menu as a backwards tab. This CL updates
> NSMenuItem(ChromeAdditions) to check for this special case.
>
> This CL removes two incorrect DCHECKs from render_widget_host_view_cocoa.mm. I
> regularly hit both of them when running Chromium on a local build. They both
> rely on the false assumption that keyEquivalents in the main menu must have the
> command modifier.
>
> Bug: 851714
> Reviewed-on: https://chromium-review.googlesource.com/1106659
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#574211}

Change-Id: Id87f364f5c5b303e8b9668e79f813a1c23376c98
TBR: avi@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1134203
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574628}

chrome/app/nibs/MainMenu.xib

@@ -481,14 +481,20 @@ CA
                                 </connections>
                             </menuItem>
                             <menuItem isSeparatorItem="YES" id="453"/>
-                            <menuItem title="^IDS_NEXT_TAB_MAC" tag="34016" keyEquivalent="" id="455">
-                                <modifierMask key="keyEquivalentModifierMask" option="YES" command="YES"/>
+                            <menuItem title="^IDS_NEXT_TAB_MAC" tag="34016" id="455">
+                                <string key="keyEquivalent" base64-UTF8="YES">
+CQ
+</string>
+                                <modifierMask key="keyEquivalentModifierMask" control="YES"/>
                                 <connections>
                                     <action selector="commandDispatch:" target="-1" id="529"/>
                                 </connections>
                             </menuItem>
-                            <menuItem title="^IDS_PREV_TAB_MAC" tag="34017" keyEquivalent="" id="454">
-                                <modifierMask key="keyEquivalentModifierMask" option="YES" command="YES"/>
+                            <menuItem title="^IDS_PREV_TAB_MAC" tag="34017" id="454">
+                                <string key="keyEquivalent" base64-UTF8="YES">
+CQ
+</string>
+                                <modifierMask key="keyEquivalentModifierMask" shift="YES" control="YES"/>
                                 <connections>
                                     <action selector="commandDispatch:" target="-1" id="530"/>
                                 </connections>

chrome/browser/global_keyboard_shortcuts_mac.mm

@@ -123,9 +123,9 @@ const std::vector<KeyboardShortcutData>& GetShortcutsNotPresentInMainMenu() {
     {true,  true,  false, false, kVK_ANSI_RightBracket, IDC_SELECT_NEXT_TAB},
     {true,  true,  false, false, kVK_ANSI_LeftBracket,  IDC_SELECT_PREVIOUS_TAB},
     {false, false, true,  false, kVK_PageDown,          IDC_SELECT_NEXT_TAB},
-    {false, false, true,  false, kVK_Tab,               IDC_SELECT_NEXT_TAB},
     {false, false, true,  false, kVK_PageUp,            IDC_SELECT_PREVIOUS_TAB},
-    {false, true,  true,  false, kVK_Tab,               IDC_SELECT_PREVIOUS_TAB},
+    {true,  false, false, true,  kVK_RightArrow,        IDC_SELECT_NEXT_TAB},
+    {true,  false, false, true,  kVK_LeftArrow,         IDC_SELECT_PREVIOUS_TAB},
 
     // Cmd-0..8 select the nth tab, with cmd-9 being "last tab".
     {true, false, false, false, kVK_ANSI_1,             IDC_SELECT_TAB_0},

chrome/browser/ui/cocoa/accelerators_cocoa.mm

@@ -99,9 +99,8 @@ const struct AcceleratorMapping {
     {IDC_FORWARD, NSCommandKeyMask, ui::VKEY_OEM_6},
     {IDC_BOOKMARK_ALL_TABS, NSCommandKeyMask | NSShiftKeyMask, ui::VKEY_D},
     {IDC_MINIMIZE_WINDOW, NSCommandKeyMask, ui::VKEY_M},
-  {IDC_SELECT_NEXT_TAB, NSCommandKeyMask | NSAlternateKeyMask, ui::VKEY_RIGHT},
-  {IDC_SELECT_PREVIOUS_TAB, NSCommandKeyMask | NSAlternateKeyMask,
-   ui::VKEY_LEFT},
+    {IDC_SELECT_NEXT_TAB, NSControlKeyMask, ui::VKEY_TAB},
+    {IDC_SELECT_PREVIOUS_TAB, NSControlKeyMask | NSShiftKeyMask, ui::VKEY_TAB},
     {IDC_HELP_PAGE_VIA_MENU, NSCommandKeyMask | NSShiftKeyMask, ui::VKEY_OEM_2},
 };
 

chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm

@@ -45,6 +45,15 @@ NSMenuItem* MenuContainsAccelerator(NSMenu* menu,
     }
 
     if ([item.keyEquivalent isEqual:key_equivalent]) {
+      // We don't want to ignore shift for [cmd + shift + tab] and [cmd + tab],
+      // which are special.
+      if (item.tag == IDC_SELECT_NEXT_TAB ||
+          item.tag == IDC_SELECT_PREVIOUS_TAB) {
+        if (modifiers == item.keyEquivalentModifierMask)
+          return item;
+        continue;
+      }
+
       BOOL maskEqual =
           (modifiers == item.keyEquivalentModifierMask) ||
           ((modifiers & (~NSShiftKeyMask)) == item.keyEquivalentModifierMask);

chrome/browser/ui/cocoa/nsmenuitem_additions.mm

@@ -147,10 +147,20 @@ void SetIsInputSourceDvorakQwertyForTesting(bool is_dvorak_qwerty) {
     }
   }
 
-  // Clear shift key for printable characters.
+  // [ctr + shift + tab] generates the "End of Medium" keyEquivalent rather than
+  // "Horizontal Tab". We still use "Horizontal Tab" in the main menu to match
+  // the behavior of Safari and Terminal. Thus, we need to explicitly check for
+  // this case.
+  if ((eventModifiers & NSShiftKeyMask) &&
+      [eventString isEqualToString:@"\x19"]) {
+    eventString = @"\x9";
+  } else {
+    // Clear shift key for printable characters, excluding tab.
     if ((eventModifiers & (NSNumericPadKeyMask | NSFunctionKeyMask)) == 0 &&
-      [[self keyEquivalent] characterAtIndex:0] != '\r')
+        [[self keyEquivalent] characterAtIndex:0] != '\r') {
       eventModifiers &= ~NSShiftKeyMask;
+    }
+  }
 
   // Clear all non-interesting modifiers
   eventModifiers &= NSCommandKeyMask |

chrome/browser/ui/cocoa/nsmenuitem_additions_unittest.mm

@@ -291,6 +291,13 @@ TEST(NSMenuItemAdditionsTest, TestFiresForKeyEvent) {
   key = KeyEvent(0x100108, @"s", @"\u0441", 1);
   ExpectKeyFiresItem(key, MenuItem(@"s", 0x100000), false);
   ExpectKeyDoesntFireItem(key, MenuItem(@"c", 0x100000));
+
+  // ctr + shift + tab produces the "End of Medium" keyEquivalent, even though
+  // it should produce the "Horizontal Tab" keyEquivalent. Check to make sure
+  // it matches anyways.
+  key = KeyEvent(0x60103, @"\x19", @"\x19", 1);
+  ExpectKeyFiresItem(key, MenuItem(@"\x9", NSShiftKeyMask | NSControlKeyMask),
+                     false);
 }
 
 NSString* keyCodeToCharacter(NSUInteger keyCode,

content/browser/renderer_host/render_widget_host_view_cocoa.mm

@@ -593,20 +593,6 @@ void ExtractUnderlines(NSAttributedString* string,
   if (EventIsReservedBySystem(theEvent))
     return NO;
 
-  // If we return |NO| from this function, cocoa will send the key event to
-  // the menu and only if the menu does not process the event to |keyDown:|. We
-  // want to send the event to a renderer _before_ sending it to the menu, so
-  // we need to return |YES| for all events that might be swallowed by the menu.
-  // We do not return |YES| for every keypress because we don't get |keyDown:|
-  // events for keys that we handle this way.
-  NSUInteger modifierFlags = [theEvent modifierFlags];
-  if ((modifierFlags & NSCommandKeyMask) == 0) {
-    // Make sure the menu does not contain key equivalents that don't
-    // contain cmd.
-    DCHECK(![[NSApp mainMenu] performKeyEquivalent:theEvent]);
-    return NO;
-  }
-
   // Command key combinations are sent via performKeyEquivalent rather than
   // keyDown:. We just forward this on and if WebCore doesn't want to handle
   // it, we let the WebContentsView figure out how to reinject it.
@@ -656,9 +642,6 @@ void ExtractUnderlines(NSAttributedString* string,
   if (EventIsReservedBySystem(theEvent))
     return;
 
-  DCHECK(eventType != NSKeyDown ||
-         !equiv == !(modifierFlags & NSCommandKeyMask));
-
   if (eventType == NSFlagsChanged) {
     // Ignore NSFlagsChanged events from the NumLock and Fn keys as
     // Safari does in -[WebHTMLView flagsChanged:] (of "WebHTMLView.mm").

ui/events/test/cocoa_test_event_utils.mm

@@ -345,6 +345,16 @@ NSEvent* SynthesizeKeyEvent(NSWindow* window,
   NSString* charactersIgnoringModifiers =
       [[[NSString alloc] initWithCharacters:&shifted_character
                                      length:1] autorelease];
+
+  // Control + [Shift] Tab is special.
+  if (keycode == ui::VKEY_TAB && (flags & NSControlKeyMask)) {
+    if (flags & NSShiftKeyMask) {
+      charactersIgnoringModifiers = @"\x19";
+    } else {
+      charactersIgnoringModifiers = @"\x9";
+    }
+  }
+
   NSString* characters;
   // The following were determined empirically on OSX 10.9.
   if (flags & NSControlKeyMask) {