Skip to content

Commit

Permalink
Add tab-reordering shortcuts to the Mac
Browse files Browse the repository at this point in the history 
Chrome Linux/ChromeOS has shortcuts to reorder a tab left/right. That
would be useful on the Mac too, so steal the shortcut (control +
shift + page up/down which is unused), and bind it.

Update the non-Mac file with correct references to the Mac file, and
do minor clang-tidy-inspired cleanup.

Fixed: 1311296
Change-Id: Ibaa5bfb61004581a1164c066b2a58920b0c18332
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3572285
Auto-Submit: Avi Drissman <avi@chromium.org>
Reviewed-by: Tom Burgin <bur@chromium.org>
Commit-Queue: Tom Burgin <bur@chromium.org>
Cr-Commit-Position: refs/heads/main@{#989447}
  • Loading branch information
Avi Drissman authored and Chromium LUCI CQ committed Apr 6, 2022
1 parent f539fff commit 329a222
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 101 deletions.
134 changes: 68 additions & 66 deletions chrome/browser/global_keyboard_shortcuts_mac.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,14 @@ bool MatchesEventForKeyboardShortcut(const KeyboardShortcutData& shortcut,

const std::vector<KeyboardShortcutData>&
GetDelayedShortcutsNotPresentInMainMenu() {
// clang-format off
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},
// cmd shift cntrl option vkeycode command
// --- ----- ----- ------ -------- -------
{true, false, false, false, kVK_LeftArrow, IDC_BACK},
{true, false, false, false, kVK_RightArrow, IDC_FORWARD},
});
// clang-format on
return *keys;
}

Expand All @@ -131,73 +133,72 @@ CommandForKeyEventResult ShortcutCommand(int cmd) {
return {cmd, /*from_main_menu=*/false};
}

} // namespace

// Returns a vector of hidden keyboard shortcuts (i.e. ones that arent present
// in the menus). Note that the hidden "Cmd =" shortcut is somehow enabled by
// the ui::VKEY_OEM_PLUS entry in accelerators_cocoa.mm.
std::vector<KeyboardShortcutData> CreateKeyboardShortcutVector() {
// clang-format off
std::vector<KeyboardShortcutData> keys({
// cmd shift cntrl option vkeycode command
// --- ----- ----- ------ -------- -------
{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_PageUp, 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},
{true, false, false, false, kVK_ANSI_Keypad1, IDC_SELECT_TAB_0},
{true, false, false, false, kVK_ANSI_2, IDC_SELECT_TAB_1},
{true, false, false, false, kVK_ANSI_Keypad2, IDC_SELECT_TAB_1},
{true, false, false, false, kVK_ANSI_3, IDC_SELECT_TAB_2},
{true, false, false, false, kVK_ANSI_Keypad3, IDC_SELECT_TAB_2},
{true, false, false, false, kVK_ANSI_4, IDC_SELECT_TAB_3},
{true, false, false, false, kVK_ANSI_Keypad4, IDC_SELECT_TAB_3},
{true, false, false, false, kVK_ANSI_5, IDC_SELECT_TAB_4},
{true, false, false, false, kVK_ANSI_Keypad5, IDC_SELECT_TAB_4},
{true, false, false, false, kVK_ANSI_6, IDC_SELECT_TAB_5},
{true, false, false, false, kVK_ANSI_Keypad6, IDC_SELECT_TAB_5},
{true, false, false, false, kVK_ANSI_7, IDC_SELECT_TAB_6},
{true, false, false, false, kVK_ANSI_Keypad7, IDC_SELECT_TAB_6},
{true, false, false, false, kVK_ANSI_8, IDC_SELECT_TAB_7},
{true, false, false, false, kVK_ANSI_Keypad8, IDC_SELECT_TAB_7},
{true, false, false, false, kVK_ANSI_9, IDC_SELECT_LAST_TAB},
{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, true, false, false, kVK_ANSI_C, IDC_DEV_TOOLS_INSPECT},
{true, false, false, true, kVK_ANSI_C, IDC_DEV_TOOLS_INSPECT},
{true, false, false, true, kVK_DownArrow, IDC_FOCUS_NEXT_PANE},
{true, false, false, true, kVK_UpArrow, IDC_FOCUS_PREVIOUS_PANE},
{true, true, false, true, kVK_ANSI_A, IDC_FOCUS_INACTIVE_POPUP_FOR_ACCESSIBILITY},
});
// clang-format on

if (base::FeatureList::IsEnabled(features::kUIDebugTools)) {
keys.push_back(
{false, true, true, true, kVK_ANSI_T, IDC_DEBUG_TOGGLE_TABLET_MODE});
keys.push_back(
{false, true, true, true, kVK_ANSI_V, IDC_DEBUG_PRINT_VIEW_TREE});
keys.push_back({false, true, true, true, kVK_ANSI_M,
IDC_DEBUG_PRINT_VIEW_TREE_DETAILS});
}
return keys;
}

} // namespace

const std::vector<KeyboardShortcutData>& GetShortcutsNotPresentInMainMenu() {
static const base::NoDestructor<std::vector<KeyboardShortcutData>> keys(
CreateKeyboardShortcutVector());
static const base::NoDestructor<std::vector<KeyboardShortcutData>> keys([]() {
// clang-format off
std::vector<KeyboardShortcutData> keys({
// cmd shift cntrl option vkeycode command
// --- ----- ----- ------ -------- -------
{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_PageUp, IDC_SELECT_PREVIOUS_TAB},
{true, false, false, true, kVK_RightArrow, IDC_SELECT_NEXT_TAB},
{true, false, false, true, kVK_LeftArrow, IDC_SELECT_PREVIOUS_TAB},
{false, true, true, false, kVK_PageDown, IDC_MOVE_TAB_NEXT},
{false, true, true, false, kVK_PageUp, IDC_MOVE_TAB_PREVIOUS},

// Cmd-0..8 select the nth tab, with cmd-9 being "last tab".
{true, false, false, false, kVK_ANSI_1, IDC_SELECT_TAB_0},
{true, false, false, false, kVK_ANSI_Keypad1, IDC_SELECT_TAB_0},
{true, false, false, false, kVK_ANSI_2, IDC_SELECT_TAB_1},
{true, false, false, false, kVK_ANSI_Keypad2, IDC_SELECT_TAB_1},
{true, false, false, false, kVK_ANSI_3, IDC_SELECT_TAB_2},
{true, false, false, false, kVK_ANSI_Keypad3, IDC_SELECT_TAB_2},
{true, false, false, false, kVK_ANSI_4, IDC_SELECT_TAB_3},
{true, false, false, false, kVK_ANSI_Keypad4, IDC_SELECT_TAB_3},
{true, false, false, false, kVK_ANSI_5, IDC_SELECT_TAB_4},
{true, false, false, false, kVK_ANSI_Keypad5, IDC_SELECT_TAB_4},
{true, false, false, false, kVK_ANSI_6, IDC_SELECT_TAB_5},
{true, false, false, false, kVK_ANSI_Keypad6, IDC_SELECT_TAB_5},
{true, false, false, false, kVK_ANSI_7, IDC_SELECT_TAB_6},
{true, false, false, false, kVK_ANSI_Keypad7, IDC_SELECT_TAB_6},
{true, false, false, false, kVK_ANSI_8, IDC_SELECT_TAB_7},
{true, false, false, false, kVK_ANSI_Keypad8, IDC_SELECT_TAB_7},
{true, false, false, false, kVK_ANSI_9, IDC_SELECT_LAST_TAB},
{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, true, false, false, kVK_ANSI_C, IDC_DEV_TOOLS_INSPECT},
{true, false, false, true, kVK_ANSI_C, IDC_DEV_TOOLS_INSPECT},
{true, false, false, true, kVK_DownArrow, IDC_FOCUS_NEXT_PANE},
{true, false, false, true, kVK_UpArrow, IDC_FOCUS_PREVIOUS_PANE},
{true, true, false, true, kVK_ANSI_A, IDC_FOCUS_INACTIVE_POPUP_FOR_ACCESSIBILITY},
});
// clang-format on

if (base::FeatureList::IsEnabled(features::kUIDebugTools)) {
keys.push_back(
{false, true, true, true, kVK_ANSI_T, IDC_DEBUG_TOGGLE_TABLET_MODE});
keys.push_back(
{false, true, true, true, kVK_ANSI_V, IDC_DEBUG_PRINT_VIEW_TREE});
keys.push_back({false, true, true, true, kVK_ANSI_M,
IDC_DEBUG_PRINT_VIEW_TREE_DETAILS});
}
return keys;
}());
return *keys;
}

const std::vector<NSMenuItem*>& GetMenuItemsNotPresentInMainMenu() {
static base::NoDestructor<std::vector<NSMenuItem*>> menu_items;
if (menu_items->empty()) {
static base::NoDestructor<std::vector<NSMenuItem*>> menu_items([]() {
std::vector<NSMenuItem*> menu_items;
for (const auto& shortcut : GetShortcutsNotPresentInMainMenu()) {
ui::Accelerator accelerator = AcceleratorFromShortcut(shortcut);
NSString* key_equivalent = nil;
Expand All @@ -207,15 +208,16 @@ CommandForKeyEventResult ShortcutCommand(int cmd) {

// Intentionally leaked!
NSMenuItem* item = [[NSMenuItem alloc] initWithTitle:@""
action:NULL
action:nullptr
keyEquivalent:key_equivalent];
item.keyEquivalentModifierMask = modifier_mask;

// We store the command in the tag.
item.tag = shortcut.chrome_command;
menu_items->push_back(item);
menu_items.push_back(item);
}
}
return menu_items;
}());
return *menu_items;
}

Expand Down
41 changes: 25 additions & 16 deletions chrome/browser/global_keyboard_shortcuts_mac_unittest.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,24 +40,33 @@ int CommandForKeys(int vkey_code,
ShiftKeyState shift = ShiftKeyState::kUp,
OptionKeyState option = OptionKeyState::kUp,
ControlKeyState control = ControlKeyState::kUp) {
NSUInteger modifierFlags = 0;
NSUInteger modifier_flags = 0;
if (command == CommandKeyState::kDown)
modifierFlags |= NSCommandKeyMask;
modifier_flags |= NSCommandKeyMask;
if (shift == ShiftKeyState::kDown)
modifierFlags |= NSShiftKeyMask;
modifier_flags |= NSShiftKeyMask;
if (option == OptionKeyState::kDown)
modifierFlags |= NSAlternateKeyMask;
modifier_flags |= NSAlternateKeyMask;
if (control == ControlKeyState::kDown)
modifierFlags |= NSControlKeyMask;
modifier_flags |= NSControlKeyMask;

switch (vkey_code) {
case kVK_UpArrow:
case kVK_DownArrow:
case kVK_LeftArrow:
case kVK_RightArrow:
// Docs say this is set whenever a key came from the numpad *or* the arrow
// keys.
modifierFlags |= NSEventModifierFlagNumericPad;
// Docs say that this is set for numpad *and* arrow keys.
modifier_flags |= NSEventModifierFlagNumericPad;
[[fallthrough]];
case kVK_Help:
case kVK_ForwardDelete:
case kVK_Home:
case kVK_End:
case kVK_PageUp:
case kVK_PageDown:
// Docs say that this is set for function keys *and* the cluster of six
// navigation keys in the center of the keyboard *and* arrow keys.
modifier_flags |= NSEventModifierFlagFunction;
break;
default:
break;
Expand All @@ -66,14 +75,14 @@ int CommandForKeys(int vkey_code,
unichar shifted_character;
unichar character;
int result = ui::MacKeyCodeForWindowsKeyCode(
ui::KeyboardCodeFromKeyCode(vkey_code), modifierFlags, &shifted_character,
&character);
ui::KeyboardCodeFromKeyCode(vkey_code), modifier_flags,
&shifted_character, &character);
DCHECK_NE(result, -1);

NSEvent* event = [NSEvent
keyEventWithType:NSKeyDown
location:NSZeroPoint
modifierFlags:modifierFlags
modifierFlags:modifier_flags
timestamp:0.0
windowNumber:0
context:nil
Expand Down Expand Up @@ -142,17 +151,17 @@ int CommandForKeys(int vkey_code,
// We only consider unshifted keys. A shifted numpad key gives a different
// keyEquivalent than a shifted number key.
const ShiftKeyState shift = ShiftKeyState::kUp;
for (unsigned int i = 0; i < std::size(equivalents); ++i) {
for (auto equivalent : equivalents) {
for (CommandKeyState command :
{CommandKeyState::kUp, CommandKeyState::kDown}) {
for (OptionKeyState option :
{OptionKeyState::kUp, OptionKeyState::kDown}) {
for (ControlKeyState control :
{ControlKeyState::kUp, ControlKeyState::kDown}) {
EXPECT_EQ(CommandForKeys(equivalents[i].keycode, command, shift,
option, control),
CommandForKeys(equivalents[i].keypad_keycode, command,
shift, option, control));
EXPECT_EQ(CommandForKeys(equivalent.keycode, command, shift, option,
control),
CommandForKeys(equivalent.keypad_keycode, command, shift,
option, control));
}
}
}
Expand Down
29 changes: 10 additions & 19 deletions chrome/browser/ui/views/accelerator_table.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <stddef.h>

#include "base/containers/contains.h"
#include "base/feature_list.h"
#include "base/no_destructor.h"
#include "base/notreached.h"
Expand All @@ -27,8 +28,8 @@ namespace {
// http://blogs.msdn.com/b/oldnewthing/archive/2004/03/29/101121.aspx
const AcceleratorMapping kAcceleratorMap[] = {
// To add an accelerator to macOS that uses modifier keys, either:
// 1) Update MainMenu.xib to include a new menu item with the appropriate
// modifier.
// 1) Update the main menu built in main_menu_builder.mm to include a new menu
// item with the appropriate modifier.
// 2) Update GetShortcutsNotPresentInMainMenu() in
// global_keyboard_shortcuts_mac.mm.
#if !BUILDFLAG(IS_CHROMEOS_ASH)
Expand Down Expand Up @@ -261,16 +262,10 @@ constexpr AcceleratorMapping kUIDebugAcceleratorMap[] = {
};

const int kRepeatableCommandIds[] = {
IDC_FIND_NEXT,
IDC_FIND_PREVIOUS,
IDC_FOCUS_NEXT_PANE,
IDC_FOCUS_PREVIOUS_PANE,
IDC_MOVE_TAB_NEXT,
IDC_MOVE_TAB_PREVIOUS,
IDC_SELECT_NEXT_TAB,
IDC_SELECT_PREVIOUS_TAB,
IDC_FIND_NEXT, IDC_FIND_PREVIOUS, IDC_FOCUS_NEXT_PANE,
IDC_FOCUS_PREVIOUS_PANE, IDC_MOVE_TAB_NEXT, IDC_MOVE_TAB_PREVIOUS,
IDC_SELECT_NEXT_TAB, IDC_SELECT_PREVIOUS_TAB,
};
const size_t kRepeatableCommandIdsLength = std::size(kRepeatableCommandIds);

} // namespace

Expand Down Expand Up @@ -306,9 +301,9 @@ std::vector<AcceleratorMapping> GetAcceleratorList() {
bool GetStandardAcceleratorForCommandId(int command_id,
ui::Accelerator* accelerator) {
#if BUILDFLAG(IS_MAC)
// On macOS, the cut/copy/paste accelerators are defined in MainMenu.xib and
// the accelerator is user configurable. All of this is handled by
// CommandDispatcher.
// On macOS, the cut/copy/paste accelerators are defined in the main menu
// built in main_menu_builder.mm and the accelerator is user configurable. All
// of this is handled by CommandDispatcher.
NOTREACHED();
return false;
#else
Expand All @@ -330,9 +325,5 @@ bool GetStandardAcceleratorForCommandId(int command_id,
}

bool IsCommandRepeatable(int command_id) {
for (size_t i = 0; i < kRepeatableCommandIdsLength; ++i) {
if (kRepeatableCommandIds[i] == command_id)
return true;
}
return false;
return base::Contains(kRepeatableCommandIds, command_id);
}

0 comments on commit 329a222

Please sign in to comment.