From f4ae5fa38107393502031c299810d5f074bd63ce Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 17 May 2021 10:08:23 +0900 Subject: [PATCH 1/4] fix: correctly handle shifted char in accelerator --- patches/chromium/accelerator.patch | 89 +++++++++---------- shell/browser/ui/accelerator_util.cc | 5 +- .../ui/cocoa/electron_menu_controller.mm | 32 +++++-- shell/common/keyboard_util.cc | 24 +++-- shell/common/keyboard_util.h | 6 +- 5 files changed, 96 insertions(+), 60 deletions(-) diff --git a/patches/chromium/accelerator.patch b/patches/chromium/accelerator.patch index 52b93466261ef..dfd2ec2c282ac 100644 --- a/patches/chromium/accelerator.patch +++ b/patches/chromium/accelerator.patch @@ -1,16 +1,16 @@ From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 4 Oct 2018 14:57:02 -0700 -Subject: accelerator.patch +Subject: fix: improve shortcut text of Accelerator This patch makes three changes to Accelerator::GetShortcutText to improve shortcut display text in menus: 1. Ctrl-Alt- accelerators show as Ctrl-Alt- instead of as Ctrl- 2. F2-F24 accelerators show up as such -3. Ctrl-Shift-= should show as Ctrl-+ +3. Ctrl-Shift-= and Ctrl-Plus show up as such diff --git a/ui/base/accelerators/accelerator.cc b/ui/base/accelerators/accelerator.cc -index d6913b15149f773cad28b5e2278b0f80df3d2896..15f944c4bb2fde7241b643f6a979a81ebce844b1 100644 +index d6913b15149f773cad28b5e2278b0f80df3d2896..25342f62acdc28806a0e6ae0bd129c59083ccf06 100644 --- a/ui/base/accelerators/accelerator.cc +++ b/ui/base/accelerators/accelerator.cc @@ -11,6 +11,7 @@ @@ -21,61 +21,39 @@ index d6913b15149f773cad28b5e2278b0f80df3d2896..15f944c4bb2fde7241b643f6a979a81e #include "base/strings/utf_string_conversions.h" #include "build/build_config.h" #include "build/chromeos_buildflags.h" -@@ -27,9 +28,7 @@ - #include +@@ -234,6 +235,11 @@ std::u16string Accelerator::GetShortcutText() const { #endif --#if !defined(OS_WIN) && (defined(USE_AURA) || defined(OS_MAC)) - #include "ui/events/keycodes/keyboard_code_conversion.h" --#endif - - #if defined(OS_CHROMEOS) - #include "ui/base/ui_base_features.h" -@@ -233,7 +232,15 @@ std::u16string Accelerator::GetShortcutText() const { - shortcut = KeyCodeToName(); - #endif - -+ unsigned int flags = 0; if (shortcut.empty()) { -+ const uint16_t c = DomCodeToUsLayoutCharacter( -+ UsLayoutKeyboardCodeToDomCode(key_code_), flags); -+ if (c != 0) { -+ shortcut = -+ static_cast( -+ base::ToUpperASCII(static_cast(c))); -+ } ++ // When a shifted char is explicitly specified, for example Ctrl+Plus, ++ // use the shifted char directly. ++ if (shifted_char) { ++ shortcut += *shifted_char; ++ } else { #if defined(OS_WIN) // Our fallback is to try translate the key code to a regular character // unless it is one of digits (VK_0 to VK_9). Some keyboard -@@ -242,21 +249,14 @@ std::u16string Accelerator::GetShortcutText() const { - // accent' for '0'). For display in the menu (e.g. Ctrl-0 for the - // default zoom level), we leave VK_[0-9] alone without translation. - wchar_t key; -- if (base::IsAsciiDigit(key_code_)) -+ if (base::IsAsciiDigit(key_code_)) { - key = static_cast(key_code_); -- else -- key = LOWORD(::MapVirtualKeyW(key_code_, MAPVK_VK_TO_CHAR)); -- // If there is no translation for the given |key_code_| (e.g. -- // VKEY_UNKNOWN), |::MapVirtualKeyW| returns 0. -- if (key != 0) -- shortcut += key; --#elif defined(USE_AURA) || defined(OS_MAC) || defined(OS_ANDROID) -- const uint16_t c = DomCodeToUsLayoutCharacter( -- UsLayoutKeyboardCodeToDomCode(key_code_), false); -- if (c != 0) -- shortcut += -- static_cast(base::ToUpperASCII(c)); -+ shortcut = key; -+ } +@@ -257,6 +263,10 @@ std::u16string Accelerator::GetShortcutText() const { + shortcut += + static_cast(base::ToUpperASCII(c)); #endif ++ } + if (key_code_ > VKEY_F1 && key_code_ <= VKEY_F24) + shortcut = base::UTF8ToUTF16( + base::StringPrintf("F%d", key_code_ - VKEY_F1 + 1)); } #if defined(OS_MAC) -@@ -452,7 +452,7 @@ std::u16string Accelerator::ApplyLongFormModifiers( +@@ -444,7 +454,7 @@ std::u16string Accelerator::ApplyLongFormModifiers( + const std::u16string& shortcut) const { + std::u16string result = shortcut; + +- if (IsShiftDown()) ++ if (!shifted_char && IsShiftDown()) + result = ApplyModifierToAcceleratorString(result, IDS_APP_SHIFT_KEY); + + // Note that we use 'else-if' in order to avoid using Ctrl+Alt as a shortcut. +@@ -452,7 +462,7 @@ std::u16string Accelerator::ApplyLongFormModifiers( // more information. if (IsCtrlDown()) result = ApplyModifierToAcceleratorString(result, IDS_APP_CTRL_KEY); @@ -84,3 +62,24 @@ index d6913b15149f773cad28b5e2278b0f80df3d2896..15f944c4bb2fde7241b643f6a979a81e result = ApplyModifierToAcceleratorString(result, IDS_APP_ALT_KEY); if (IsCmdDown()) { +diff --git a/ui/base/accelerators/accelerator.h b/ui/base/accelerators/accelerator.h +index 780a45f9ca2dd60e0deac27cc6e8f69e72cd8435..b740fbbfb14b5737b18b84c07c8e9f79cfc645c0 100644 +--- a/ui/base/accelerators/accelerator.h ++++ b/ui/base/accelerators/accelerator.h +@@ -16,6 +16,7 @@ + #include + + #include "base/component_export.h" ++#include "base/optional.h" + #include "base/time/time.h" + #include "build/build_config.h" + #include "ui/events/event_constants.h" +@@ -129,6 +130,8 @@ class COMPONENT_EXPORT(UI_BASE) Accelerator { + return interrupted_by_mouse_event_; + } + ++ base::Optional shifted_char; ++ + private: + std::u16string ApplyLongFormModifiers(const std::u16string& shortcut) const; + std::u16string ApplyShortFormModifiers(const std::u16string& shortcut) const; diff --git a/shell/browser/ui/accelerator_util.cc b/shell/browser/ui/accelerator_util.cc index 05ec3dafe1cab..f8a4a2f0f7ce3 100644 --- a/shell/browser/ui/accelerator_util.cc +++ b/shell/browser/ui/accelerator_util.cc @@ -31,9 +31,11 @@ bool StringToAccelerator(const std::string& shortcut, // Now, parse it into an accelerator. int modifiers = ui::EF_NONE; ui::KeyboardCode key = ui::VKEY_UNKNOWN; + base::Optional shifted_char; for (const auto& token : tokens) { bool shifted = false; - ui::KeyboardCode code = electron::KeyboardCodeFromStr(token, &shifted); + ui::KeyboardCode code = + electron::KeyboardCodeFromStr(token, &shifted, &shifted_char); if (shifted) modifiers |= ui::EF_SHIFT_DOWN; switch (code) { @@ -65,6 +67,7 @@ bool StringToAccelerator(const std::string& shortcut, } *accelerator = ui::Accelerator(key, modifiers); + accelerator->shifted_char = shifted_char; return true; } diff --git a/shell/browser/ui/cocoa/electron_menu_controller.mm b/shell/browser/ui/cocoa/electron_menu_controller.mm index fb4f8dd5ebba2..e2de4d048739c 100644 --- a/shell/browser/ui/cocoa/electron_menu_controller.mm +++ b/shell/browser/ui/cocoa/electron_menu_controller.mm @@ -21,9 +21,9 @@ #include "shell/browser/ui/electron_menu_model.h" #include "shell/browser/window_list.h" #include "ui/base/accelerators/accelerator.h" -#include "ui/base/accelerators/platform_accelerator_cocoa.h" #include "ui/base/l10n/l10n_util_mac.h" #include "ui/events/cocoa/cocoa_event_utils.h" +#include "ui/events/keycodes/keyboard_code_conversion_mac.h" #include "ui/gfx/image/image.h" #include "ui/strings/grit/ui_strings.h" @@ -392,11 +392,31 @@ - (void)addItemToMenu:(NSMenu*)menu ui::Accelerator accelerator; if (model->GetAcceleratorAtWithParams(index, useDefaultAccelerator_, &accelerator)) { - NSString* key_equivalent; - NSUInteger modifier_mask; - GetKeyEquivalentAndModifierMaskFromAccelerator( - accelerator, &key_equivalent, &modifier_mask); - [item setKeyEquivalent:key_equivalent]; + // Note that we are not using Chromium's + // GetKeyEquivalentAndModifierMaskFromAccelerator API, + // because it will convert Shift+Character to ShiftedCharacter, for + // example Shift+/ would be converted to ?, which is against macOS HIG. + // See also https://github.com/electron/electron/issues/21790. + NSUInteger modifier_mask = 0; + if (accelerator.IsCtrlDown()) + modifier_mask |= NSEventModifierFlagControl; + if (accelerator.IsAltDown()) + modifier_mask |= NSEventModifierFlagOption; + if (accelerator.IsCmdDown()) + modifier_mask |= NSEventModifierFlagCommand; + unichar character; + if (accelerator.shifted_char) { + // When a shifted char is explicitly specified, for example Ctrl+Plus, + // use the shifted char directly. + character = static_cast(*accelerator.shifted_char); + } else { + // Otherwise use the unshifted combinations, for example Ctrl+Shift+=. + if (accelerator.IsShiftDown()) + modifier_mask |= NSEventModifierFlagShift; + ui::MacKeyCodeForWindowsKeyCode(accelerator.key_code(), modifier_mask, + nullptr, &character); + } + [item setKeyEquivalent:[NSString stringWithFormat:@"%C", character]]; [item setKeyEquivalentModifierMask:modifier_mask]; } diff --git a/shell/common/keyboard_util.cc b/shell/common/keyboard_util.cc index c5dc82c540dcd..27782ff620cb5 100644 --- a/shell/common/keyboard_util.cc +++ b/shell/common/keyboard_util.cc @@ -15,8 +15,10 @@ namespace electron { namespace { // Return key code represented by |str|. -ui::KeyboardCode KeyboardCodeFromKeyIdentifier(const std::string& s, - bool* shifted) { +ui::KeyboardCode KeyboardCodeFromKeyIdentifier( + const std::string& s, + bool* shifted, + base::Optional* shifted_char) { std::string str = base::ToLowerASCII(s); if (str == "ctrl" || str == "control") { return ui::VKEY_CONTROL; @@ -37,6 +39,8 @@ ui::KeyboardCode KeyboardCodeFromKeyIdentifier(const std::string& s, return ui::VKEY_ALTGR; } else if (str == "plus") { *shifted = true; + if (shifted_char) + shifted_char->emplace('+'); return ui::VKEY_OEM_PLUS; } else if (str == "capslock") { return ui::VKEY_CAPITAL; @@ -319,11 +323,17 @@ ui::KeyboardCode KeyboardCodeFromCharCode(char16_t c, bool* shifted) { } } -ui::KeyboardCode KeyboardCodeFromStr(const std::string& str, bool* shifted) { - if (str.size() == 1) - return KeyboardCodeFromCharCode(str[0], shifted); - else - return KeyboardCodeFromKeyIdentifier(str, shifted); +ui::KeyboardCode KeyboardCodeFromStr(const std::string& str, + bool* shifted, + base::Optional* shifted_char) { + if (str.size() == 1) { + auto ret = KeyboardCodeFromCharCode(str[0], shifted); + if (*shifted && shifted_char) + shifted_char->emplace(str[0]); + return ret; + } else { + return KeyboardCodeFromKeyIdentifier(str, shifted, shifted_char); + } } } // namespace electron diff --git a/shell/common/keyboard_util.h b/shell/common/keyboard_util.h index 28f2a69637aad..ad5101b7e37b2 100644 --- a/shell/common/keyboard_util.h +++ b/shell/common/keyboard_util.h @@ -7,6 +7,7 @@ #include +#include "base/optional.h" #include "ui/events/keycodes/keyboard_codes.h" namespace electron { @@ -17,7 +18,10 @@ ui::KeyboardCode KeyboardCodeFromCharCode(char16_t c, bool* shifted); // Return key code of the |str|, and also determine whether the SHIFT key is // pressed. -ui::KeyboardCode KeyboardCodeFromStr(const std::string& str, bool* shifted); +ui::KeyboardCode KeyboardCodeFromStr( + const std::string& str, + bool* shifted, + base::Optional* shifted_char = nullptr); } // namespace electron From 32b591648e73a3e008909e85edef539ed300467b Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Mon, 17 May 2021 22:18:48 +0900 Subject: [PATCH 2/4] test: use actual accelerator of NSMenuItem --- shell/browser/api/electron_api_menu.h | 2 +- shell/browser/api/electron_api_menu_mac.h | 1 + shell/browser/api/electron_api_menu_mac.mm | 36 ++++++++++++++++++++++ spec-main/api-menu-item-spec.ts | 32 +++++++++++-------- 4 files changed, 58 insertions(+), 13 deletions(-) diff --git a/shell/browser/api/electron_api_menu.h b/shell/browser/api/electron_api_menu.h index 89150f5bd341a..d4e391b95e59a 100644 --- a/shell/browser/api/electron_api_menu.h +++ b/shell/browser/api/electron_api_menu.h @@ -111,7 +111,7 @@ class Menu : public gin::Wrappable, std::u16string GetLabelAt(int index) const; std::u16string GetSublabelAt(int index) const; std::u16string GetToolTipAt(int index) const; - std::u16string GetAcceleratorTextAt(int index) const; + virtual std::u16string GetAcceleratorTextAt(int index) const; bool IsItemCheckedAt(int index) const; bool IsEnabledAt(int index) const; bool IsVisibleAt(int index) const; diff --git a/shell/browser/api/electron_api_menu_mac.h b/shell/browser/api/electron_api_menu_mac.h index 5a1802563b28f..16497dbda50fa 100644 --- a/shell/browser/api/electron_api_menu_mac.h +++ b/shell/browser/api/electron_api_menu_mac.h @@ -35,6 +35,7 @@ class MenuMac : public Menu { int positioning_item, base::OnceClosure callback); void ClosePopupAt(int32_t window_id) override; + std::u16string GetAcceleratorTextAt(int index) const override; void ClosePopupOnUI(int32_t window_id); private: diff --git a/shell/browser/api/electron_api_menu_mac.mm b/shell/browser/api/electron_api_menu_mac.mm index 49d7934187680..202b34b409d19 100644 --- a/shell/browser/api/electron_api_menu_mac.mm +++ b/shell/browser/api/electron_api_menu_mac.mm @@ -127,6 +127,42 @@ std::move(close_popup)); } +std::u16string MenuMac::GetAcceleratorTextAt(int index) const { + // A least effort to get the real shortcut text of NSMenuItem, the code does + // not need to be perfect since it is test only. + base::scoped_nsobject controller( + [[ElectronMenuController alloc] initWithModel:model() + useDefaultAccelerator:NO]); + NSMenuItem* item = [[controller menu] itemAtIndex:index]; + std::u16string text; + NSEventModifierFlags modifiers = [item keyEquivalentModifierMask]; + if (modifiers & NSEventModifierFlagControl) + text += u"Ctrl"; + if (modifiers & NSEventModifierFlagShift) { + if (!text.empty()) + text += u"+"; + text += u"Shift"; + } + if (modifiers & NSEventModifierFlagOption) { + if (!text.empty()) + text += u"+"; + text += u"Alt"; + } + if (modifiers & NSEventModifierFlagCommand) { + if (!text.empty()) + text += u"+"; + text += u"Command"; + } + if (!text.empty()) + text += u"+"; + auto key = base::ToUpperASCII(base::SysNSStringToUTF16([item keyEquivalent])); + if (key == u"\t") + text += u"Tab"; + else + text += key; + return text; +} + void MenuMac::ClosePopupOnUI(int32_t window_id) { auto controller = popup_controllers_.find(window_id); if (controller != popup_controllers_.end()) { diff --git a/spec-main/api-menu-item-spec.ts b/spec-main/api-menu-item-spec.ts index afdd368b09923..9039cf855f0ac 100644 --- a/spec-main/api-menu-item-spec.ts +++ b/spec-main/api-menu-item-spec.ts @@ -462,9 +462,9 @@ describe('MenuItems', () => { { label: 'text', accelerator: 'Alt+A' } ]); - expect(menu.getAcceleratorTextAt(0)).to.equal(isDarwin() ? '⌘A' : 'Ctrl+A'); - expect(menu.getAcceleratorTextAt(1)).to.equal(isDarwin() ? '⇧A' : 'Shift+A'); - expect(menu.getAcceleratorTextAt(2)).to.equal(isDarwin() ? '⌥A' : 'Alt+A'); + expect(menu.getAcceleratorTextAt(0)).to.equal(isDarwin() ? 'Command+A' : 'Ctrl+A'); + expect(menu.getAcceleratorTextAt(1)).to.equal('Shift+A'); + expect(menu.getAcceleratorTextAt(2)).to.equal('Alt+A'); }); it('should display modifiers correctly for special keys', () => { @@ -474,9 +474,9 @@ describe('MenuItems', () => { { label: 'text', accelerator: 'Alt+Tab' } ]); - expect(menu.getAcceleratorTextAt(0)).to.equal(isDarwin() ? '⌘⇥' : 'Ctrl+Tab'); - expect(menu.getAcceleratorTextAt(1)).to.equal(isDarwin() ? '⇧⇥' : 'Shift+Tab'); - expect(menu.getAcceleratorTextAt(2)).to.equal(isDarwin() ? '⌥⇥' : 'Alt+Tab'); + expect(menu.getAcceleratorTextAt(0)).to.equal(isDarwin() ? 'Command+Tab' : 'Ctrl+Tab'); + expect(menu.getAcceleratorTextAt(1)).to.equal('Shift+Tab'); + expect(menu.getAcceleratorTextAt(2)).to.equal('Alt+Tab'); }); it('should not display modifiers twice', () => { @@ -485,18 +485,26 @@ describe('MenuItems', () => { { label: 'text', accelerator: 'Shift+Shift+Tab' } ]); - expect(menu.getAcceleratorTextAt(0)).to.equal(isDarwin() ? '⇧A' : 'Shift+A'); - expect(menu.getAcceleratorTextAt(1)).to.equal(isDarwin() ? '⇧⇥' : 'Shift+Tab'); + expect(menu.getAcceleratorTextAt(0)).to.equal('Shift+A'); + expect(menu.getAcceleratorTextAt(1)).to.equal('Shift+Tab'); }); - it('should display correctly for edge cases', () => { + it('should display correctly for shifted keys', () => { const menu = Menu.buildFromTemplate([ { label: 'text', accelerator: 'Control+Shift+=' }, - { label: 'text', accelerator: 'Control+Plus' } + { label: 'text', accelerator: 'Control+Plus' }, + { label: 'text', accelerator: 'Control+Shift+3' }, + { label: 'text', accelerator: 'Control+#' }, + { label: 'text', accelerator: 'Control+Shift+/' }, + { label: 'text', accelerator: 'Control+?' } ]); - expect(menu.getAcceleratorTextAt(0)).to.equal(isDarwin() ? '⌃⇧=' : 'Ctrl+Shift+='); - expect(menu.getAcceleratorTextAt(1)).to.equal(isDarwin() ? '⌃⇧=' : 'Ctrl+Shift+='); + expect(menu.getAcceleratorTextAt(0)).to.equal('Ctrl+Shift+='); + expect(menu.getAcceleratorTextAt(1)).to.equal('Ctrl++'); + expect(menu.getAcceleratorTextAt(2)).to.equal('Ctrl+Shift+3'); + expect(menu.getAcceleratorTextAt(3)).to.equal('Ctrl+#'); + expect(menu.getAcceleratorTextAt(4)).to.equal('Ctrl+Shift+/'); + expect(menu.getAcceleratorTextAt(5)).to.equal('Ctrl+?'); }); }); }); From 172c5fa7d751f5fd73eb3fac71a18b2b243dbec7 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 18 May 2021 09:32:24 +0900 Subject: [PATCH 3/4] chore: simplify KeyboardCodeFromStr --- shell/browser/ui/accelerator_util.cc | 6 ++---- shell/common/gin_converters/blink_converter.cc | 6 +++--- shell/common/keyboard_util.cc | 13 +++++-------- shell/common/keyboard_util.h | 9 ++++----- 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/shell/browser/ui/accelerator_util.cc b/shell/browser/ui/accelerator_util.cc index f8a4a2f0f7ce3..89f431f01f505 100644 --- a/shell/browser/ui/accelerator_util.cc +++ b/shell/browser/ui/accelerator_util.cc @@ -33,10 +33,8 @@ bool StringToAccelerator(const std::string& shortcut, ui::KeyboardCode key = ui::VKEY_UNKNOWN; base::Optional shifted_char; for (const auto& token : tokens) { - bool shifted = false; - ui::KeyboardCode code = - electron::KeyboardCodeFromStr(token, &shifted, &shifted_char); - if (shifted) + ui::KeyboardCode code = electron::KeyboardCodeFromStr(token, &shifted_char); + if (shifted_char) modifiers |= ui::EF_SHIFT_DOWN; switch (code) { // The token can be a modifier. diff --git a/shell/common/gin_converters/blink_converter.cc b/shell/common/gin_converters/blink_converter.cc index 2e6b49f0b6938..94ce709b3e70e 100644 --- a/shell/common/gin_converters/blink_converter.cc +++ b/shell/common/gin_converters/blink_converter.cc @@ -187,10 +187,10 @@ bool Converter::FromV8(v8::Isolate* isolate, if (!dict.Get("keyCode", &str)) return false; - bool shifted = false; - ui::KeyboardCode keyCode = electron::KeyboardCodeFromStr(str, &shifted); + base::Optional shifted_char; + ui::KeyboardCode keyCode = electron::KeyboardCodeFromStr(str, &shifted_char); out->windows_key_code = keyCode; - if (shifted) + if (shifted_char) out->SetModifiers(out->GetModifiers() | blink::WebInputEvent::Modifiers::kShiftKey); diff --git a/shell/common/keyboard_util.cc b/shell/common/keyboard_util.cc index 27782ff620cb5..597a10f040810 100644 --- a/shell/common/keyboard_util.cc +++ b/shell/common/keyboard_util.cc @@ -17,7 +17,6 @@ namespace { // Return key code represented by |str|. ui::KeyboardCode KeyboardCodeFromKeyIdentifier( const std::string& s, - bool* shifted, base::Optional* shifted_char) { std::string str = base::ToLowerASCII(s); if (str == "ctrl" || str == "control") { @@ -38,9 +37,7 @@ ui::KeyboardCode KeyboardCodeFromKeyIdentifier( } else if (str == "altgr") { return ui::VKEY_ALTGR; } else if (str == "plus") { - *shifted = true; - if (shifted_char) - shifted_char->emplace('+'); + shifted_char->emplace('+'); return ui::VKEY_OEM_PLUS; } else if (str == "capslock") { return ui::VKEY_CAPITAL; @@ -324,15 +321,15 @@ ui::KeyboardCode KeyboardCodeFromCharCode(char16_t c, bool* shifted) { } ui::KeyboardCode KeyboardCodeFromStr(const std::string& str, - bool* shifted, base::Optional* shifted_char) { if (str.size() == 1) { - auto ret = KeyboardCodeFromCharCode(str[0], shifted); - if (*shifted && shifted_char) + bool shifted = false; + auto ret = KeyboardCodeFromCharCode(str[0], &shifted); + if (shifted) shifted_char->emplace(str[0]); return ret; } else { - return KeyboardCodeFromKeyIdentifier(str, shifted, shifted_char); + return KeyboardCodeFromKeyIdentifier(str, shifted_char); } } diff --git a/shell/common/keyboard_util.h b/shell/common/keyboard_util.h index ad5101b7e37b2..29b740152e808 100644 --- a/shell/common/keyboard_util.h +++ b/shell/common/keyboard_util.h @@ -16,12 +16,11 @@ namespace electron { // pressed. ui::KeyboardCode KeyboardCodeFromCharCode(char16_t c, bool* shifted); -// Return key code of the |str|, and also determine whether the SHIFT key is +// Return key code of the |str|, if the original key is a shifted character, +// for example + and /, set it in |shifted_char|. // pressed. -ui::KeyboardCode KeyboardCodeFromStr( - const std::string& str, - bool* shifted, - base::Optional* shifted_char = nullptr); +ui::KeyboardCode KeyboardCodeFromStr(const std::string& str, + base::Optional* shifted_char); } // namespace electron From e374724b16b17bead43afc8605b31897aae2cf7e Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 2 Jun 2021 10:40:28 +0900 Subject: [PATCH 4/4] chore: GetAcceleratorTextAt is testing only --- shell/browser/api/electron_api_menu.cc | 8 ++++++-- shell/browser/api/electron_api_menu.h | 4 +++- shell/browser/api/electron_api_menu_mac.h | 6 ++++-- shell/browser/api/electron_api_menu_mac.mm | 4 +++- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/shell/browser/api/electron_api_menu.cc b/shell/browser/api/electron_api_menu.cc index c992168e77893..a5806167f37bd 100644 --- a/shell/browser/api/electron_api_menu.cc +++ b/shell/browser/api/electron_api_menu.cc @@ -236,11 +236,13 @@ std::u16string Menu::GetToolTipAt(int index) const { return model_->GetToolTipAt(index); } -std::u16string Menu::GetAcceleratorTextAt(int index) const { +#ifdef DCHECK_IS_ON +std::u16string Menu::GetAcceleratorTextAtForTesting(int index) const { ui::Accelerator accelerator; model_->GetAcceleratorAtWithParams(index, true, &accelerator); return accelerator.GetShortcutText(); } +#endif bool Menu::IsItemCheckedAt(int index) const { return model_->IsItemCheckedAt(index); @@ -289,13 +291,15 @@ v8::Local Menu::FillObjectTemplate( .SetMethod("getLabelAt", &Menu::GetLabelAt) .SetMethod("getSublabelAt", &Menu::GetSublabelAt) .SetMethod("getToolTipAt", &Menu::GetToolTipAt) - .SetMethod("getAcceleratorTextAt", &Menu::GetAcceleratorTextAt) .SetMethod("isItemCheckedAt", &Menu::IsItemCheckedAt) .SetMethod("isEnabledAt", &Menu::IsEnabledAt) .SetMethod("worksWhenHiddenAt", &Menu::WorksWhenHiddenAt) .SetMethod("isVisibleAt", &Menu::IsVisibleAt) .SetMethod("popupAt", &Menu::PopupAt) .SetMethod("closePopupAt", &Menu::ClosePopupAt) +#ifdef DCHECK_IS_ON + .SetMethod("getAcceleratorTextAt", &Menu::GetAcceleratorTextAtForTesting) +#endif .Build(); } diff --git a/shell/browser/api/electron_api_menu.h b/shell/browser/api/electron_api_menu.h index d4e391b95e59a..3fd2be0c30069 100644 --- a/shell/browser/api/electron_api_menu.h +++ b/shell/browser/api/electron_api_menu.h @@ -78,6 +78,9 @@ class Menu : public gin::Wrappable, int positioning_item, base::OnceClosure callback) = 0; virtual void ClosePopupAt(int32_t window_id) = 0; +#ifdef DCHECK_IS_ON + virtual std::u16string GetAcceleratorTextAtForTesting(int index) const; +#endif std::unique_ptr model_; Menu* parent_ = nullptr; @@ -111,7 +114,6 @@ class Menu : public gin::Wrappable, std::u16string GetLabelAt(int index) const; std::u16string GetSublabelAt(int index) const; std::u16string GetToolTipAt(int index) const; - virtual std::u16string GetAcceleratorTextAt(int index) const; bool IsItemCheckedAt(int index) const; bool IsEnabledAt(int index) const; bool IsVisibleAt(int index) const; diff --git a/shell/browser/api/electron_api_menu_mac.h b/shell/browser/api/electron_api_menu_mac.h index 16497dbda50fa..dc52e2c45b803 100644 --- a/shell/browser/api/electron_api_menu_mac.h +++ b/shell/browser/api/electron_api_menu_mac.h @@ -35,12 +35,14 @@ class MenuMac : public Menu { int positioning_item, base::OnceClosure callback); void ClosePopupAt(int32_t window_id) override; - std::u16string GetAcceleratorTextAt(int index) const override; - void ClosePopupOnUI(int32_t window_id); +#ifdef DCHECK_IS_ON + std::u16string GetAcceleratorTextAtForTesting(int index) const override; +#endif private: friend class Menu; + void ClosePopupOnUI(int32_t window_id); void OnClosed(int32_t window_id, base::OnceClosure callback); scoped_nsobject menu_controller_; diff --git a/shell/browser/api/electron_api_menu_mac.mm b/shell/browser/api/electron_api_menu_mac.mm index 202b34b409d19..0d3d5eff41e7b 100644 --- a/shell/browser/api/electron_api_menu_mac.mm +++ b/shell/browser/api/electron_api_menu_mac.mm @@ -127,7 +127,8 @@ std::move(close_popup)); } -std::u16string MenuMac::GetAcceleratorTextAt(int index) const { +#ifdef DCHECK_IS_ON +std::u16string MenuMac::GetAcceleratorTextAtForTesting(int index) const { // A least effort to get the real shortcut text of NSMenuItem, the code does // not need to be perfect since it is test only. base::scoped_nsobject controller( @@ -162,6 +163,7 @@ text += key; return text; } +#endif void MenuMac::ClosePopupOnUI(int32_t window_id) { auto controller = popup_controllers_.find(window_id);