macOS: Fix keyEquivalents on PT keyboard layout.

The only layout that should use -[NSEvent characters] to match keyEquivalents
rather than -[NSEvent charactersIgnoringModifiers] is DVORAK-QWERTY. This CL
updates the logic in NSMenuItem(ChromeAdditions) to make this check.

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

chrome/browser/ui/cocoa/nsmenuitem_additions.h

@@ -14,10 +14,10 @@
 // This method always returns NO if the menu item is not enabled.
 - (BOOL)cr_firesForKeyEvent:(NSEvent*)event;
 
-// Like above method, but this method matches the key equivalent regardless of
-// the menu item's enable state.
-- (BOOL)cr_firesForKeyEventIfEnabled:(NSEvent*)event;
-
 @end
 
+// Used by tests to set internal state without having to change global input
+// source.
+void SetIsInputSourceDvorakQwertyForTesting(bool is_dvorak_qwerty);
+
 #endif  // CHROME_BROWSER_UI_COCOA_NSMENUITEM_ADDITIONS_H_

chrome/browser/ui/cocoa/nsmenuitem_additions.mm

@@ -7,16 +7,59 @@
 #include <Carbon/Carbon.h>
 
 #include "base/logging.h"
+#include "base/mac/scoped_cftyperef.h"
+
+namespace {
+bool g_is_input_source_dvorak_qwerty = false;
+}  // namespace
+
+void SetIsInputSourceDvorakQwertyForTesting(bool is_dvorak_qwerty) {
+  g_is_input_source_dvorak_qwerty = is_dvorak_qwerty;
+}
+
+@interface KeyboardInputSourceListener : NSObject
+@end
+
+@implementation KeyboardInputSourceListener
+
+- (instancetype)init {
+  if (self = [super init]) {
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(inputSourceDidChange:)
+               name:NSTextInputContextKeyboardSelectionDidChangeNotification
+             object:nil];
+    [self updateInputSource];
+  }
+  return self;
+}
+
+- (void)dealloc {
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [super dealloc];
+}
+
+- (void)updateInputSource {
+  base::ScopedCFTypeRef<TISInputSourceRef> inputSource(
+      TISCopyCurrentKeyboardInputSource());
+  NSString* inputSourceID = (NSString*)TISGetInputSourceProperty(
+      inputSource.get(), kTISPropertyInputSourceID);
+  g_is_input_source_dvorak_qwerty =
+      [inputSourceID isEqualToString:@"com.apple.keylayout.DVORAK-QWERTYCMD"];
+}
+
+- (void)inputSourceDidChange:(NSNotification*)notification {
+  [self updateInputSource];
+}
+
+@end
 
 @implementation NSMenuItem(ChromeAdditions)
 
 - (BOOL)cr_firesForKeyEvent:(NSEvent*)event {
   if (![self isEnabled])
     return NO;
-  return [self cr_firesForKeyEventIfEnabled:event];
-}
 
-- (BOOL)cr_firesForKeyEventIfEnabled:(NSEvent*)event {
   DCHECK([event type] == NSKeyDown);
   // In System Preferences->Keyboard->Keyboard Shortcuts, it is possible to add
   // arbitrary keyboard shortcuts to applications. It is not documented how this
@@ -77,19 +120,20 @@
       [[event characters] characterAtIndex:0] <= 0x7f)
     eventString = [event characters];
 
+  // We intentionally leak this object.
+  static __attribute__((unused)) KeyboardInputSourceListener* listener =
+      [[KeyboardInputSourceListener alloc] init];
+
   // We typically want to compare [NSMenuItem keyEquivalent] against [NSEvent
   // charactersIgnoringModifiers]. There is a special keyboard layout "Dvorak -
   // QWERTY" which uses QWERTY-style shortcuts when the Command key is held
   // down. In this case, we want to use [NSEvent characters] instead of [NSEvent
   // charactersIgnoringModifiers]. The problem is, this has the wrong behavior
-  // for every other keyboard layout when the "Shift" key is held down, since
-  // [NSEvent characters] does not reflect the effects of the "Shift" key.
+  // for every other keyboard layout.
   //
-  // When the "Shift" key is not held down, we use [NSEvent characters], since
-  // that has the right behavior for all keyboard layouts. When the "Shift" key
-  // is held down, we use [NSEvent charactersWithoutModifiers], which has the
-  // right behavior everywhere but "Dvorak - QWERTY".
-  if (!(eventModifiers & NSShiftKeyMask)) {
+  // The documentation for -[NSEvent charactersIgnoringModifiers] states that
+  // it is the appropriate method to use for implementing keyEquivalents.
+  if (g_is_input_source_dvorak_qwerty) {
     // When both |characters| and |charactersIgnoringModifiers| are ascii, we
     // want to use |characters| if it's a character and
     // |charactersIgnoringModifiers| else (on dvorak, cmd-shift-z should fire

chrome/browser/ui/cocoa/nsmenuitem_additions_unittest.mm

@@ -269,15 +269,22 @@ TEST(NSMenuItemAdditionsTest, TestFiresForKeyEvent) {
 
   // cmd-z on dvorak qwerty layout (so that the key produces ';', but 'z' if
   // cmd is down)
+  SetIsInputSourceDvorakQwertyForTesting(true);
   key = KeyEvent(0x100108, @"z", @";", 6);
   ExpectKeyFiresItem(key, MenuItem(@"z", 0x100000), false);
   ExpectKeyDoesntFireItem(key, MenuItem(@";", 0x100000), false);
+  SetIsInputSourceDvorakQwertyForTesting(false);
 
   // cmd-shift-z on dvorak layout (so that we get a ':')
   key = KeyEvent(0x12010a, @";", @":", 6);
   ExpectKeyFiresItem(key, MenuItem(@":", 0x100000));
   ExpectKeyDoesntFireItem(key, MenuItem(@";", 0x100000));
 
+  // On PT layout, caps lock should not affect the keyEquivalent.
+  key = KeyEvent(0x110108, @"T", @"t", 17);
+  ExpectKeyFiresItem(key, MenuItem(@"t", 0x100000));
+  ExpectKeyDoesntFireItem(key, MenuItem(@"T", 0x100000));
+
   // cmd-s with a serbian layout (just "s" produces something that looks a lot
   // like "c" in some fonts, but is actually \u0441. cmd-s activates a menu item
   // with key equivalent "s", not "c")
@@ -334,6 +341,17 @@ TEST(NSMenuItemAdditionsTest, TestMOnDifferentLayouts) {
       keyCode = 0x16;
     } else if ([layoutId isEqualToString:@"com.apple.keylayout.Dvorak-Right"]) {
       keyCode = 0x1a;
+    } else if ([layoutId
+                   isEqualToString:@"com.apple.keylayout.Tibetan-Wylie"]) {
+      // In Tibetan-Wylie, the only way to type the "m" character is with cmd +
+      // key_code=0x2e. As such, it doesn't make sense for this same combination
+      // to trigger a keyEquivalent, since then it won't be possible to type
+      // "m".
+      continue;
+    }
+
+    if ([layoutId isEqualToString:@"com.apple.keylayout.DVORAK-QWERTYCMD"]) {
+      SetIsInputSourceDvorakQwertyForTesting(true);
     }
 
     EventModifiers modifiers = cmdKey >> 8;
@@ -341,6 +359,10 @@ TEST(NSMenuItemAdditionsTest, TestMOnDifferentLayouts) {
     NSString* charsIgnoringMods = keyCodeToCharacter(keyCode, 0, ref);
     NSEvent* key = KeyEvent(0x100000, chars, charsIgnoringMods, keyCode);
     ExpectKeyFiresItem(key, item, false);
+
+    if ([layoutId isEqualToString:@"com.apple.keylayout.DVORAK-QWERTYCMD"]) {
+      SetIsInputSourceDvorakQwertyForTesting(false);
+    }
   }
   CFRelease(list);
 }