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

* fix: correctly handle shifted char in accelerator

* test: use actual accelerator of NSMenuItem

* chore: simplify KeyboardCodeFromStr

* chore: GetAcceleratorTextAt is testing only

shell/browser/api/electron_api_menu.cc

@@ -235,11 +235,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);
@@ -288,13 +290,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

@@ -34,11 +34,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

@@ -186,10 +186,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