Skip to content

Commit

Permalink
Get global accelerators on Mac for brave://commands (#17872)
Browse files Browse the repository at this point in the history
* Get global accelerators on Mac for brave://commands

On Mac, accelerator mechanism is different from that of other platforms.
Make a shim to fill the accelerators properly.
  • Loading branch information
sangwoo108 authored Apr 12, 2023
1 parent 3b4bd3f commit d6339cd
Show file tree
Hide file tree
Showing 9 changed files with 268 additions and 49 deletions.
2 changes: 1 addition & 1 deletion app/command_utils.cc.template
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ base::StringPiece GetCommandName(int command_id) {
CHECK(it != kCommandInfos.end())
<< "Unknown command " << command_id
<< ". This function should only be used for known "
"commands (i.e. commands in |GetCommandInfo|). "
"commands (i.e. commands in |GetCommands()|). "
"This command should probably be added.";
return it->second.data();
}
Expand Down
2 changes: 1 addition & 1 deletion app/command_utils_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#include "testing/gtest/include/gtest/gtest.h"

// Note: If this test fails because an accelerated command isn't present just
// add the missing command to |commands::GetCommandInfo| in command_utils.h.
// add the missing command to |commands::GetCommands| in command_utils.h.
TEST(CommandUtilsUnitTest, AllAcceleratedCommandsShouldBeAvailable) {
base::flat_set<int> excluded_commands = {
IDC_RUN_SCREEN_AI_VISUAL_ANNOTATIONS};
Expand Down
2 changes: 2 additions & 0 deletions browser/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,8 @@ source_set("ui") {

if (is_mac) {
sources += [
"views/commands/default_accelerators_mac.h",
"views/commands/default_accelerators_mac.mm",
"views/frame/brave_browser_frame_mac.h",
"views/frame/brave_browser_frame_mac.mm",
"views/frame/brave_browser_non_client_frame_view_mac.h",
Expand Down
22 changes: 22 additions & 0 deletions browser/ui/views/commands/default_accelerators_mac.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/* Copyright (c) 2023 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_BROWSER_UI_VIEWS_COMMANDS_DEFAULT_ACCELERATORS_MAC_H_
#define BRAVE_BROWSER_UI_VIEWS_COMMANDS_DEFAULT_ACCELERATORS_MAC_H_

#include <vector>

#include "chrome/browser/ui/views/accelerator_table.h"

namespace commands {

// Returns shortcuts mapped to global menu, which can be changed dynamically
// by the OS too. This is why we don't have a static table for these.
// (System Preference > Keyboard > Keyboard Shortcuts > App shortcuts)
std::vector<AcceleratorMapping> GetGlobalAccelerators();

} // namespace commands

#endif // BRAVE_BROWSER_UI_VIEWS_COMMANDS_DEFAULT_ACCELERATORS_MAC_H_
190 changes: 190 additions & 0 deletions browser/ui/views/commands/default_accelerators_mac.mm
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
// Copyright (c) 2023 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

#include "brave/browser/ui/views/commands/default_accelerators_mac.h"

#import <Cocoa/Cocoa.h>

#include "base/check.h"
#include "base/containers/contains.h"
#include "base/containers/flat_map.h"
#include "base/logging.h"
#include "base/strings/sys_string_conversions.h"
#include "brave/app/command_utils.h"
#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/global_keyboard_shortcuts_mac.h"
#include "chrome/browser/ui/cocoa/accelerators_cocoa.h"
#include "ui/events/cocoa/cocoa_event_utils.h"
#include "ui/events/event_constants.h"
#include "ui/events/keycodes/keyboard_code_conversion_mac.h"

static_assert(__OBJC__);
static_assert(BUILDFLAG(IS_MAC));

namespace commands {

namespace {

bool CanConvertToAcceleratorMapping(int command_id) {
if (command_id == 0) {
return false;
}

return base::Contains(commands::GetCommands(), command_id);
}

bool CanConvertToAcceleratorMapping(NSMenuItem* item) {
if (item.hasSubmenu) {
return false;
}

if (auto command_id = static_cast<int>(item.tag);
!CanConvertToAcceleratorMapping(command_id)) {
return false;
}

NSString* keyEquivalent = item.keyEquivalent;
return keyEquivalent != nil && [keyEquivalent length] > 0;
}

AcceleratorMapping ToAcceleratorMapping(NSMenuItem* item) {
NSString* keyEquivalent = item.keyEquivalent;
DVLOG(2) << __FUNCTION__ << item.tag << " > "
<< base::SysNSStringToUTF16(keyEquivalent);
NSEvent* keyEvent = [NSEvent keyEventWithType:NSEventTypeKeyDown
location:NSZeroPoint
modifierFlags:0
timestamp:0
windowNumber:0
context:nil
characters:keyEquivalent
charactersIgnoringModifiers:keyEquivalent
isARepeat:NO
keyCode:0];

auto modifiers = ui::EventFlagsFromModifiers(item.keyEquivalentModifierMask);
if (item.keyEquivalentModifierMask & NSEventModifierFlagFunction) {
// ui::EventFlagsFromModifiers doesn't handle the 'Function key.
modifiers |= ui::EF_FUNCTION_DOWN;
}

if (![keyEquivalent isEqualToString:[keyEquivalent lowercaseString]]) {
// Shift key could be omitted in |item.keyEquivalentModifierMask| even
// if it's needed. In this case |item.keyEquivalent| can be already
// shifted/capitalized. So we should add it manually.
modifiers |= ui::EF_SHIFT_DOWN;
}

// "Close Tab" and "Close Window" item's shortcuts are statically the same.
// They are dynamically changed based on the context. e.g. has tab or not,
// is pwa or not. And also the value is hard coded.
// https://github.com/chromium/chromium/blob/299385e09d41d5ce3abd434879b5f9b0a8880cd7/chrome/browser/app_controller_mac.mm#L772
// Here we're to provide brave commands, manually add shift modifier for
// the "Close Window" item.
// TODO(sko) As the shortcut is hard coded in the file above, we might need to
// forbid to adjust default key for IDC_CLOSE_TAB and IDC_CLOSE_WINDOW.
if (static_cast<int>(item.tag) == IDC_CLOSE_WINDOW &&
(modifiers == ui::EF_COMMAND_DOWN &&
[keyEquivalent isEqualToString:@"w"])) {
modifiers |= ui::EF_SHIFT_DOWN;
}

return {.keycode = ui::KeyboardCodeFromNSEvent(keyEvent),
.modifiers = modifiers,
.command_id = static_cast<int>(item.tag)};
}

AcceleratorMapping ToAcceleratorMapping(const KeyboardShortcutData& data) {
int modifiers = 0;
if (data.command_key) {
modifiers |= ui::EF_COMMAND_DOWN;
}
if (data.shift_key) {
modifiers |= ui::EF_SHIFT_DOWN;
}
if (data.cntrl_key) {
modifiers |= ui::EF_CONTROL_DOWN;
}
if (data.opt_key) {
modifiers |= ui::EF_ALT_DOWN;
}

return {.keycode = ui::KeyboardCodeFromKeyCode(data.vkey_code),
.modifiers = modifiers,
.command_id = data.chrome_command};
}

AcceleratorMapping ToAcceleratorMapping(int command_id,
const ui::Accelerator& accelerator) {
return {.keycode = accelerator.key_code(),
.modifiers = accelerator.modifiers(),
.command_id = command_id};
}

void AccumulateAcceleratorsRecursively(
base::flat_map<int /*command_id*/, std::vector<AcceleratorMapping>>*
accelerators,
NSMenu* menu) {
CHECK(menu);

for (NSMenuItem* item in [menu itemArray]) {
if (CanConvertToAcceleratorMapping(item)) {
(*accelerators)[static_cast<int>(item.tag)].push_back(
ToAcceleratorMapping(item));
}

if (NSMenu* submenu = [item submenu];
submenu && submenu != [NSApp servicesMenu]) {
AccumulateAcceleratorsRecursively(accelerators, submenu);
}
}
}

} // namespace

std::vector<AcceleratorMapping> GetGlobalAccelerators() {
base::flat_map<int /*command_id*/, std::vector<AcceleratorMapping>>
accelerator_map;

// Get dynamic items from main menu.
AccumulateAcceleratorsRecursively(&accelerator_map, [NSApp mainMenu]);

// Get items that are not displayed but global.
for (const auto& shortcut_data : GetShortcutsNotPresentInMainMenu()) {
if (!CanConvertToAcceleratorMapping(shortcut_data.chrome_command)) {
DVLOG(2) << " NOT IN COMMANDS : " << shortcut_data.chrome_command;
continue;
}

DVLOG(2) << "IN COMMANDS : " << shortcut_data.chrome_command;
accelerator_map[shortcut_data.chrome_command].push_back(
ToAcceleratorMapping(shortcut_data));
}

// Add missing accelerators from static table.
// e.g. IDC_CLOSE_TAB is missing because it's dynamically added.
for (const auto& [command_id, accelerator] :
*AcceleratorsCocoa::GetInstance()) {
if (base::Contains(accelerator_map, command_id) ||
!CanConvertToAcceleratorMapping(command_id)) {
continue;
}

accelerator_map[command_id].push_back(
ToAcceleratorMapping(command_id, accelerator));
}

std::vector<AcceleratorMapping> result;
for (auto& [command_id, accelerator_mappings] : accelerator_map) {
DCHECK_NE(command_id, 0);
for (auto& accelerator_mapping : accelerator_mappings) {
DCHECK(accelerator_mapping.modifiers);
result.push_back(std::move(accelerator_mapping));
}
}
return result;
}

} // namespace commands
19 changes: 15 additions & 4 deletions browser/ui/views/commands/default_accelerators_views.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,25 @@
#include "brave/browser/ui/commands/accelerator_service.h"
#include "chrome/browser/ui/views/accelerator_table.h"

#if BUILDFLAG(IS_MAC)
#include "brave/browser/ui/views/commands/default_accelerators_mac.h"
#endif // BUILDFLAG(IS_MAC)

namespace commands {

Accelerators GetDefaultAccelerators() {
Accelerators defaults;
for (const auto& accelerator_info : GetAcceleratorList()) {
defaults[accelerator_info.command_id].push_back(
ui::Accelerator(accelerator_info.keycode, accelerator_info.modifiers));
}
auto add_to_accelerators = [&defaults](const AcceleratorMapping& mapping) {
defaults[mapping.command_id].push_back(
ui::Accelerator(mapping.keycode, mapping.modifiers));
};

base::ranges::for_each(GetAcceleratorList(), add_to_accelerators);
#if BUILDFLAG(IS_MAC)
// TODO(sko) These accelerators should be flagged as unmodifiable unless we
// can modify the OS settings. See the comment in default_accelerator_mac.h
base::ranges::for_each(GetGlobalAccelerators(), add_to_accelerators);
#endif // BUILDFLAG(IS_MAC)
return defaults;
}

Expand Down
2 changes: 0 additions & 2 deletions browser/ui/views/frame/vertical_tab_strip_region_view_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,8 @@
// So we should look into main menus in order to get the accurate accelerator
// for new tab command.

// auto* menu = [NSApp mainMenu];
NSMenuItem* item =
findMenuItemWithCommandRecursively([NSApp mainMenu], IDC_NEW_TAB);
// NSMenuItem* item = [[NSApp mainMenu] itemWithTag:IDC_NEW_TAB];
DCHECK(item);

return base::SysNSStringToUTF16(keyCombinationForMenuItem(item));
Expand Down
5 changes: 4 additions & 1 deletion components/commands/browser/accelerator_pref_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <string>
#include <vector>

#include "base/check.h"
#include "base/containers/flat_map.h"
#include "base/notreached.h"
#include "base/strings/string_number_conversions.h"
Expand Down Expand Up @@ -74,7 +75,9 @@ void AcceleratorPrefManager::AddAccelerator(
const ui::Accelerator& accelerator) {
ScopedDictPrefUpdate update(prefs_, kAcceleratorsPrefs);
auto* list = update->EnsureList(base::NumberToString(command_id));
list->Append(ToCodesString(accelerator));
auto accelerator_string = ToCodesString(accelerator);
DCHECK(!accelerator_string.empty());
list->Append(accelerator_string);
}

void AcceleratorPrefManager::RemoveAccelerator(
Expand Down
Loading

0 comments on commit d6339cd

Please sign in to comment.