fix: don't append Shift modifier text twice to accelerators (#15400)

* fix: don't append Shift modifier text twice to accelerators

* style: use the new way of creating patches

* test: add menu item accelerator display tests

* fix: allocate accelerator on the stack

* fix: adjust tests to match expected behavior on mac

atom/browser/api/atom_api_menu.cc

@@ -155,6 +155,12 @@ base::string16 Menu::GetSublabelAt(int index) const {
   return model_->GetSublabelAt(index);
 }
 
+base::string16 Menu::GetAcceleratorTextAt(int index) const {
+  ui::Accelerator accelerator;
+  model_->GetAcceleratorAtWithParams(index, true, &accelerator);
+  return accelerator.GetShortcutText();
+}
+
 bool Menu::IsItemCheckedAt(int index) const {
   return model_->IsItemCheckedAt(index);
 }
@@ -195,6 +201,7 @@ void Menu::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("getCommandIdAt", &Menu::GetCommandIdAt)
       .SetMethod("getLabelAt", &Menu::GetLabelAt)
       .SetMethod("getSublabelAt", &Menu::GetSublabelAt)
+      .SetMethod("getAcceleratorTextAt", &Menu::GetAcceleratorTextAt)
       .SetMethod("isItemCheckedAt", &Menu::IsItemCheckedAt)
       .SetMethod("isEnabledAt", &Menu::IsEnabledAt)
       .SetMethod("isVisibleAt", &Menu::IsVisibleAt)

atom/browser/api/atom_api_menu.h

@@ -91,6 +91,7 @@ class Menu : public mate::TrackableObject<Menu>,
   int GetCommandIdAt(int index) const;
   base::string16 GetLabelAt(int index) const;
   base::string16 GetSublabelAt(int index) const;
+  base::string16 GetAcceleratorTextAt(int index) const;
   bool IsItemCheckedAt(int index) const;
   bool IsEnabledAt(int index) const;
   bool IsVisibleAt(int index) const;

patches/common/chromium/accelerator.patch

@@ -5,7 +5,7 @@ Subject: accelerator.patch
 
 
 diff --git a/ui/base/accelerators/accelerator.cc b/ui/base/accelerators/accelerator.cc
-index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff184628bba1 100644
+index 7e55ef366ac8320f730cdcb268453b1fa2710887..9b000ab1dfb85c097e879eacbfe69dc052960766 100644
 --- a/ui/base/accelerators/accelerator.cc
 +++ b/ui/base/accelerators/accelerator.cc
 @@ -11,6 +11,7 @@
@@ -42,7 +42,7 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18
  #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
-@@ -148,17 +155,20 @@ base::string16 Accelerator::GetShortcutText() const {
+@@ -148,18 +155,14 @@ base::string16 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;
@@ -60,20 +60,14 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18
 -          static_cast<base::string16::value_type>(base::ToUpperASCII(c));
 +      shortcut = key;
 +    }
-+#endif
+ #endif
 +    if (key_code_ > VKEY_F1 && key_code_ <= VKEY_F24)
 +      shortcut = base::UTF8ToUTF16(
 +          base::StringPrintf("F%d", key_code_ - VKEY_F1 + 1));
-+  } else if (IsShiftDown()) {
-+#if defined(OS_MACOSX)
-+    const base::char16 kShiftSymbol[] = {0x21e7, 0};
-+    shortcut = kShiftSymbol;
-+#else
-+    shortcut = l10n_util::GetStringFUTF16(IDS_APP_SHIFT_MODIFIER, shortcut);
- #endif
    }
 
-@@ -223,7 +233,7 @@ base::string16 Accelerator::ApplyLongFormModifiers(
+   // Checking whether the character used for the accelerator is alphanumeric.
+@@ -223,7 +226,7 @@ base::string16 Accelerator::ApplyLongFormModifiers(
    // more information.
    if (IsCtrlDown())
      shortcut = l10n_util::GetStringFUTF16(IDS_APP_CONTROL_MODIFIER, shortcut);
@@ -82,18 +76,3 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18
      shortcut = l10n_util::GetStringFUTF16(IDS_APP_ALT_MODIFIER, shortcut);
 
    if (IsCmdDown()) {
-@@ -243,14 +253,12 @@ base::string16 Accelerator::ApplyShortFormModifiers(
-     base::string16 shortcut) const {
-   const base::char16 kCommandSymbol[] = {0x2318, 0};
-   const base::char16 kCtrlSymbol[] = {0x2303, 0};
--  const base::char16 kShiftSymbol[] = {0x21e7, 0};
-   const base::char16 kOptionSymbol[] = {0x2325, 0};
-   const base::char16 kNoSymbol[] = {0};
-
-   std::vector<base::string16> parts;
-   parts.push_back(base::string16(IsCtrlDown() ? kCtrlSymbol : kNoSymbol));
-   parts.push_back(base::string16(IsAltDown() ? kOptionSymbol : kNoSymbol));
--  parts.push_back(base::string16(IsShiftDown() ? kShiftSymbol : kNoSymbol));
-   parts.push_back(base::string16(IsCmdDown() ? kCommandSymbol : kNoSymbol));
-   parts.push_back(shortcut);
-   return base::StrCat(parts);

spec/api-menu-item-spec.js

@@ -6,7 +6,7 @@ const { BrowserWindow, app, Menu, MenuItem } = remote
 const roles = require('../lib/browser/api/menu-item-roles')
 const { closeWindow } = require('./window-helpers')
 
-const { expect } = chai
+const { expect, assert } = chai
 chai.use(dirtyChai)
 
 describe('MenuItems', () => {
@@ -389,4 +389,68 @@ describe('MenuItems', () => {
       expect(menu.items[0].submenu.items[0].overrideProperty).to.be.a('function')
     })
   })
+
+  describe('MenuItem accelerators', () => {
+    const isDarwin = () => {
+      return (process.platform === 'darwin')
+    }
+
+    it('should display modifiers correctly for simple keys', done => {
+      const menu = Menu.buildFromTemplate([
+        { label: 'text', accelerator: 'CmdOrCtrl+A' },
+        { label: 'text', accelerator: 'Shift+A' },
+        { label: 'text', accelerator: 'Alt+A' }
+      ])
+
+      assert.strictEqual(menu.getAcceleratorTextAt(0),
+        isDarwin() ? '⌘A' : 'Ctrl+A')
+      assert.strictEqual(menu.getAcceleratorTextAt(1),
+        isDarwin() ? '⇧A' : 'Shift+A')
+      assert.strictEqual(menu.getAcceleratorTextAt(2),
+        isDarwin() ? '⌥A' : 'Alt+A')
+      done()
+    })
+
+    it('should display modifiers correctly for special keys', done => {
+      const menu = Menu.buildFromTemplate([
+        { label: 'text', accelerator: 'CmdOrCtrl+Tab' },
+        { label: 'text', accelerator: 'Shift+Tab' },
+        { label: 'text', accelerator: 'Alt+Tab' }
+      ])
+
+      assert.strictEqual(menu.getAcceleratorTextAt(0),
+        isDarwin() ? '⌘⇥\u0000' : 'Ctrl+Tab')
+      assert.strictEqual(menu.getAcceleratorTextAt(1),
+        isDarwin() ? '⇧⇥\u0000' : 'Shift+Tab')
+      assert.strictEqual(menu.getAcceleratorTextAt(2),
+        isDarwin() ? '⌥⇥\u0000' : 'Alt+Tab')
+      done()
+    })
+
+    it('should not display modifiers twice', done => {
+      const menu = Menu.buildFromTemplate([
+        { label: 'text', accelerator: 'Shift+Shift+A' },
+        { label: 'text', accelerator: 'Shift+Shift+Tab' }
+      ])
+
+      assert.strictEqual(menu.getAcceleratorTextAt(0),
+        isDarwin() ? '⇧A' : 'Shift+A')
+      assert.strictEqual(menu.getAcceleratorTextAt(1),
+        isDarwin() ? '⇧⇥\u0000' : 'Shift+Tab')
+      done()
+    })
+
+    it('should display correctly for edge cases', done => {
+      const menu = Menu.buildFromTemplate([
+        { label: 'text', accelerator: 'Control+Shift+=' },
+        { label: 'text', accelerator: 'Control+Plus' }
+      ])
+
+      assert.strictEqual(menu.getAcceleratorTextAt(0),
+        isDarwin() ? '⌃⇧=' : 'Ctrl+Shift+=')
+      assert.strictEqual(menu.getAcceleratorTextAt(1),
+        isDarwin() ? '⌃⇧=' : 'Ctrl+Shift+=')
+      done()
+    })
+  })
 })