fix: keep shifted character in menu accelerator (#29481)

* fix: correctly handle shifted char in accelerator

* test: use actual accelerator of NSMenuItem

* chore: simplify KeyboardCodeFromStr

* chore: GetAcceleratorTextAt is testing only

Co-authored-by: Cheng Zhao <[email protected]>

patches/chromium/accelerator.patch

@@ -1,16 +1,16 @@
 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
 From: Cheng Zhao <[email protected]>
 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-<Key> accelerators show as Ctrl-Alt-<Key> instead of as Ctrl-<Key>
 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 <windows.h>
+@@ -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<std::u16string::value_type>(
-+              base::ToUpperASCII(static_cast<char16_t>(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<wchar_t>(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<std::u16string::value_type>(base::ToUpperASCII(c));
-+      shortcut = key;
-+    }
+@@ -257,6 +263,10 @@ std::u16string Accelerator::GetShortcutText() const {
+       shortcut +=
+           static_cast<std::u16string::value_type>(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 <utility>
+ 
+ #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<char16_t> shifted_char;
++
+  private:
+   std::u16string ApplyLongFormModifiers(const std::u16string& shortcut) const;
+   std::u16string ApplyShortFormModifiers(const std::u16string& shortcut) const;

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<v8::ObjectTemplate> 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();
 }
 

shell/browser/api/electron_api_menu.h

@@ -78,6 +78,9 @@ class Menu : public gin::Wrappable<Menu>,
                        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<ElectronMenuModel> model_;
   Menu* parent_ = nullptr;
@@ -111,7 +114,6 @@ class Menu : public gin::Wrappable<Menu>,
   std::u16string GetLabelAt(int index) const;
   std::u16string GetSublabelAt(int index) const;
   std::u16string GetToolTipAt(int index) const;
-  std::u16string GetAcceleratorTextAt(int index) const;
   bool IsItemCheckedAt(int index) const;
   bool IsEnabledAt(int index) const;
   bool IsVisibleAt(int index) const;

shell/browser/api/electron_api_menu_mac.h

@@ -35,11 +35,14 @@ class MenuMac : public Menu {
                  int positioning_item,
                  base::OnceClosure callback);
   void ClosePopupAt(int32_t window_id) 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<ElectronMenuController> menu_controller_;

shell/browser/api/electron_api_menu_mac.mm

@@ -127,6 +127,44 @@ void MenuMac::ClosePopupAt(int32_t window_id) {
                                                    std::move(close_popup));
 }
 
+#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<ElectronMenuController> 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;
+}
+#endif
+
 void MenuMac::ClosePopupOnUI(int32_t window_id) {
   auto controller = popup_controllers_.find(window_id);
   if (controller != popup_controllers_.end()) {

shell/browser/ui/accelerator_util.cc

@@ -31,10 +31,10 @@ 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<char16_t> shifted_char;
   for (const auto& token : tokens) {
-    bool shifted = false;
-    ui::KeyboardCode code = electron::KeyboardCodeFromStr(token, &shifted);
-    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.
@@ -65,6 +65,7 @@ bool StringToAccelerator(const std::string& shortcut,
   }
 
   *accelerator = ui::Accelerator(key, modifiers);
+  accelerator->shifted_char = shifted_char;
   return true;
 }
 

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 @@ static base::scoped_nsobject<NSMenu> recentDocumentsMenuSwap_;
     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<unichar>(*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];
     }
 

shell/common/gin_converters/blink_converter.cc

@@ -187,10 +187,10 @@ bool Converter<blink::WebKeyboardEvent>::FromV8(v8::Isolate* isolate,
   if (!dict.Get("keyCode", &str))
     return false;
 
-  bool shifted = false;
-  ui::KeyboardCode keyCode = electron::KeyboardCodeFromStr(str, &shifted);
+  base::Optional<char16_t> 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);
 

shell/common/keyboard_util.cc

@@ -15,8 +15,9 @@ 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,
+    base::Optional<char16_t>* shifted_char) {
   std::string str = base::ToLowerASCII(s);
   if (str == "ctrl" || str == "control") {
     return ui::VKEY_CONTROL;
@@ -36,7 +37,7 @@ ui::KeyboardCode KeyboardCodeFromKeyIdentifier(const std::string& s,
   } else if (str == "altgr") {
     return ui::VKEY_ALTGR;
   } else if (str == "plus") {
-    *shifted = true;
+    shifted_char->emplace('+');
     return ui::VKEY_OEM_PLUS;
   } else if (str == "capslock") {
     return ui::VKEY_CAPITAL;
@@ -319,11 +320,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,
+                                     base::Optional<char16_t>* shifted_char) {
+  if (str.size() == 1) {
+    bool shifted = false;
+    auto ret = KeyboardCodeFromCharCode(str[0], &shifted);
+    if (shifted)
+      shifted_char->emplace(str[0]);
+    return ret;
+  } else {
+    return KeyboardCodeFromKeyIdentifier(str, shifted_char);
+  }
 }
 
 }  // namespace electron

shell/common/keyboard_util.h

@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/optional.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 
 namespace electron {
@@ -15,9 +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);
+ui::KeyboardCode KeyboardCodeFromStr(const std::string& str,
+                                     base::Optional<char16_t>* shifted_char);
 
 }  // namespace electron
 

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+?');
     });
   });
 });