Commit 1320aeb7 authored by Elly Fong-Jones's avatar Elly Fong-Jones Committed by Commit Bot

cocoa: allow application shortcuts to override builtin shortcuts

This change refactors the Mac command dispatcher delegate to allow application
shortcuts to override builtin shortcuts. This always worked for non-"window"
shortcuts, but window shortcuts were dispatched in prePerformKeyEquivalent:,
so they had higher priority than menu key equivalents. This change causes
prePerformKeyEquivalent: to check for a menu key equivalent and execute that
before consulting the window shortcut table. Since none of Chrome's default key
equivalents overlap the window shortcuts, any menu item whose key equivalent
matches a window shortcut is user-added and should have absolute priority.

Bug: 410357
Change-Id: I853168f1aee6c2fa82757f3fd41e04d7fc2a79be
Reviewed-on: https://chromium-review.googlesource.com/716736
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: default avatarRobert Sesek <rsesek@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508801}
parent 145660d6
......@@ -70,6 +70,9 @@ int CommandForBrowserKeyboardShortcut(
// Returns the Chrome command associated with |event|, or -1 if not found.
int CommandForKeyEvent(NSEvent* event);
// Returns the menu command associated with |event|, or -1 if not found.
int MenuCommandForKeyEvent(NSEvent* event);
// Returns a keyboard event character for the given |event|. In most cases
// this returns the first character of [NSEvent charactersIgnoringModifiers],
// but when [NSEvent character] has different printable ascii character
......
......@@ -124,21 +124,9 @@ int CommandForKeyEvent(NSEvent* event) {
if ([event type] != NSKeyDown)
return -1;
// Look in menu.
NSMenuItem* item = FindMenuItem(event, [NSApp mainMenu]);
if (item && [item action] == @selector(commandDispatch:) && [item tag] > 0)
return [item tag];
// "Close window" doesn't use the |commandDispatch:| mechanism. Menu items
// that do not correspond to IDC_ constants need no special treatment however,
// as they can't be blacklisted in
// |BrowserCommandController::IsReservedCommandOrKey()| anyhow.
if (item && [item action] == @selector(performClose:))
return IDC_CLOSE_WINDOW;
// "Exit" doesn't use the |commandDispatch:| mechanism either.
if (item && [item action] == @selector(terminate:))
return IDC_EXIT;
int cmdNum = MenuCommandForKeyEvent(event);
if (cmdNum != -1)
return cmdNum;
// Look in secondary keyboard shortcuts.
NSUInteger modifiers = [event modifierFlags];
......@@ -149,8 +137,8 @@ int CommandForKeyEvent(NSEvent* event) {
const int keyCode = [event keyCode];
const unichar keyChar = KeyCharacterForEvent(event);
int cmdNum = CommandForWindowKeyboardShortcut(
cmdKey, shiftKey, cntrlKey, optKey, keyCode, keyChar);
cmdNum = CommandForWindowKeyboardShortcut(cmdKey, shiftKey, cntrlKey, optKey,
keyCode, keyChar);
if (cmdNum != -1)
return cmdNum;
......@@ -162,6 +150,33 @@ int CommandForKeyEvent(NSEvent* event) {
return -1;
}
int MenuCommandForKeyEvent(NSEvent* event) {
if ([event type] != NSKeyDown)
return -1;
// Look in menu.
NSMenuItem* item = FindMenuItem(event, [NSApp mainMenu]);
if (!item)
return -1;
if ([item action] == @selector(commandDispatch:) && [item tag] > 0)
return [item tag];
// "Close window" doesn't use the |commandDispatch:| mechanism. Menu items
// that do not correspond to IDC_ constants need no special treatment however,
// as they can't be blacklisted in
// |BrowserCommandController::IsReservedCommandOrKey()| anyhow.
if (item && [item action] == @selector(performClose:))
return IDC_CLOSE_WINDOW;
// "Exit" doesn't use the |commandDispatch:| mechanism either.
if ([item action] == @selector(terminate:))
return IDC_EXIT;
return -1;
}
unichar KeyCharacterForEvent(NSEvent* event) {
NSString* eventString = [event charactersIgnoringModifiers];
NSString* characters = [event characters];
......
......@@ -7,6 +7,7 @@
#import <Cocoa/Cocoa.h>
#include "base/run_loop.h"
#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_commands.h"
......@@ -59,3 +60,42 @@ IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, SwitchTabsMac) {
NSShiftKeyMask | NSCommandKeyMask));
EXPECT_TRUE(tab_strip->IsTabSelected(0));
}
IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, MenuCommandPriority) {
NSWindow* ns_window = browser()->window()->GetNativeWindow();
TabStripModel* tab_strip = browser()->tab_strip_model();
// Set up window with 4 tabs.
chrome::NewTab(browser());
chrome::NewTab(browser());
chrome::NewTab(browser());
EXPECT_EQ(4, tab_strip->count());
EXPECT_TRUE(tab_strip->IsTabSelected(3));
// Use the cmd-2 hotkey to switch to the second tab.
ActivateAccelerator(ns_window, SynthesizeKeyEvent(ns_window, true, ui::VKEY_2,
NSCommandKeyMask));
EXPECT_TRUE(tab_strip->IsTabSelected(1));
// Change the "Select Next Tab" menu item's key equivalent to be cmd-2, to
// simulate what would happen if there was a user key equivalent for it. Note
// that there is a readonly "userKeyEquivalent" property on NSMenuItem, but
// this code can't modify it.
NSMenu* main_menu = [NSApp mainMenu];
ASSERT_NE(nil, main_menu);
NSMenuItem* window_menu = [main_menu itemWithTitle:@"Window"];
ASSERT_NE(nil, window_menu);
ASSERT_TRUE(window_menu.hasSubmenu);
NSMenuItem* next_item = [window_menu.submenu itemWithTag:IDC_SELECT_NEXT_TAB];
ASSERT_NE(nil, next_item);
[next_item setKeyEquivalent:@"2"];
[next_item setKeyEquivalentModifierMask:NSCommandKeyMask];
// Send cmd-2 again, and ensure the tab switches.
ActivateAccelerator(ns_window, SynthesizeKeyEvent(ns_window, true, ui::VKEY_2,
NSCommandKeyMask));
EXPECT_TRUE(tab_strip->IsTabSelected(2));
ActivateAccelerator(ns_window, SynthesizeKeyEvent(ns_window, true, ui::VKEY_2,
NSCommandKeyMask));
EXPECT_TRUE(tab_strip->IsTabSelected(3));
}
......@@ -16,9 +16,10 @@ namespace {
// Type of functions listed in global_keyboard_shortcuts_mac.h.
typedef int (*KeyToCommandMapper)(bool, bool, bool, bool, int, unichar);
// If the event is for a Browser window, and the key combination has an
// associated command, execute the command.
bool HandleExtraKeyboardShortcut(
// Returns the command that would be executed if |window| received |event|
// according to |command_for_keyboard_shortcut|, or -1 if no command would be
// executed.
int CommandForExtraKeyboardShortcut(
NSEvent* event,
NSWindow* window,
KeyToCommandMapper command_for_keyboard_shortcut) {
......@@ -34,15 +35,25 @@ bool HandleExtraKeyboardShortcut(
int cmd = command_for_keyboard_shortcut(command, shift, control, option,
key_code, key_char);
if (cmd == -1)
return false;
// Non-browser windows don't execute any commands.
if (!chrome::FindBrowserWithWindow(window))
return -1;
// Only handle event if this is a browser window.
Browser* browser = chrome::FindBrowserWithWindow(window);
if (!browser)
return cmd;
}
// If the event is for a Browser window, and the key combination has an
// associated command, execute the command.
bool HandleExtraKeyboardShortcut(
NSEvent* event,
NSWindow* window,
KeyToCommandMapper command_for_keyboard_shortcut) {
int cmd = CommandForExtraKeyboardShortcut(event, window,
command_for_keyboard_shortcut);
if (cmd == -1)
return false;
chrome::ExecuteCommand(browser, cmd);
chrome::ExecuteCommand(chrome::FindBrowserWithWindow(window), cmd);
return true;
}
......@@ -94,6 +105,24 @@ bool HandleExtraBrowserKeyboardShortcut(NSEvent* event, NSWindow* window) {
}
- (BOOL)prePerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window {
// If a command has a menu key equivalent that *replaces* one of the window
// keyboard shortcuts, the menu key equivalent needs to be executed, because
// these are user-addded keyboard shortcuts that replace builtin shortcuts.
//
// If a command has a menu key equivalent that does *not* replace a window
// keyboard shortcut, it will be handled later; only window shortcuts need
// special handling here since they happen before normal command dispatch.
int cmd = MenuCommandForKeyEvent(event);
if (cmd != -1) {
int keyCmd = CommandForExtraKeyboardShortcut(
event, window, CommandForWindowKeyboardShortcut);
Browser* browser = chrome::FindBrowserWithWindow(window);
if (keyCmd != -1 && browser) {
chrome::ExecuteCommand(browser, cmd);
return YES;
}
}
// Handle per-window shortcuts like cmd-1, but do not handle browser-level
// shortcuts like cmd-left (else, cmd-left would do history navigation even
// if e.g. the Omnibox has focus).
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment