From 83b36ae5cf8b095f32aad17e44a8f57a4c993b6c Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Thu, 25 Oct 2018 23:39:03 +0200 Subject: [PATCH 1/5] fix: don't append Shift modifier text twice to accelerators --- patches/common/chromium/accelerator.patch | 41 ++++++----------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/patches/common/chromium/accelerator.patch b/patches/common/chromium/accelerator.patch index 6ae250645ea56..970e43330ab45 100644 --- a/patches/common/chromium/accelerator.patch +++ b/patches/common/chromium/accelerator.patch @@ -19,17 +19,17 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18 @@ -21,9 +22,7 @@ #include #endif - + -#if !defined(OS_WIN) && (defined(USE_AURA) || defined(OS_MACOSX)) #include "ui/events/keycodes/keyboard_code_conversion.h" -#endif - + namespace ui { - + @@ -139,7 +138,15 @@ base::string16 Accelerator::GetShortcutText() const { shortcut = KeyCodeToName(key_code_); #endif - + + unsigned int flags = 0; if (shortcut.empty()) { + const uint16_t c = DomCodeToUsLayoutCharacter( @@ -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,40 +60,19 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18 - static_cast(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); - else if (IsAltDown()) + if (IsAltDown()) 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 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); From af2c6f3e98b04cf0104bad8b5ce8e74bdd98be9f Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Fri, 26 Oct 2018 02:44:16 +0200 Subject: [PATCH 2/5] style: use the new way of creating patches --- patches/common/chromium/accelerator.patch | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/patches/common/chromium/accelerator.patch b/patches/common/chromium/accelerator.patch index 970e43330ab45..80991576307b0 100644 --- a/patches/common/chromium/accelerator.patch +++ b/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 @@ @@ -19,17 +19,17 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18 @@ -21,9 +22,7 @@ #include #endif - + -#if !defined(OS_WIN) && (defined(USE_AURA) || defined(OS_MACOSX)) #include "ui/events/keycodes/keyboard_code_conversion.h" -#endif - + namespace ui { - + @@ -139,7 +138,15 @@ base::string16 Accelerator::GetShortcutText() const { shortcut = KeyCodeToName(key_code_); #endif - + + unsigned int flags = 0; if (shortcut.empty()) { + const uint16_t c = DomCodeToUsLayoutCharacter( @@ -65,7 +65,7 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18 + shortcut = base::UTF8ToUTF16( + base::StringPrintf("F%d", key_code_ - VKEY_F1 + 1)); } - + // Checking whether the character used for the accelerator is alphanumeric. @@ -223,7 +226,7 @@ base::string16 Accelerator::ApplyLongFormModifiers( // more information. @@ -74,5 +74,5 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18 - else if (IsAltDown()) + if (IsAltDown()) shortcut = l10n_util::GetStringFUTF16(IDS_APP_ALT_MODIFIER, shortcut); - + if (IsCmdDown()) { From 7fae6dbcfd4d801d6066638f005f491e527de83a Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Tue, 30 Oct 2018 17:27:54 +0100 Subject: [PATCH 3/5] test: add menu item accelerator display tests --- atom/browser/api/atom_api_menu.cc | 8 +++++ atom/browser/api/atom_api_menu.h | 1 + spec/api-menu-item-spec.js | 54 ++++++++++++++++++++++++++++++- 3 files changed, 62 insertions(+), 1 deletion(-) diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index 1c4f4fcf41e55..393468bec78e3 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/atom/browser/api/atom_api_menu.cc @@ -155,6 +155,13 @@ base::string16 Menu::GetSublabelAt(int index) const { return model_->GetSublabelAt(index); } +base::string16 Menu::GetAcceleratorTextAt(int index) const { + auto* accelerator = new ui::Accelerator(); + model_->GetAcceleratorAtWithParams(index, true, accelerator); + + return accelerator ? accelerator->GetShortcutText() : base::string16(); +} + bool Menu::IsItemCheckedAt(int index) const { return model_->IsItemCheckedAt(index); } @@ -195,6 +202,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) diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 26f2b679b6b7f..499263bf492b7 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -91,6 +91,7 @@ class Menu : public mate::TrackableObject, 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; diff --git a/spec/api-menu-item-spec.js b/spec/api-menu-item-spec.js index 5978f23d210f2..f2ea6e796d593 100644 --- a/spec/api-menu-item-spec.js +++ b/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,56 @@ describe('MenuItems', () => { expect(menu.items[0].submenu.items[0].overrideProperty).to.be.a('function') }) }) + + describe('MenuItem accelerators', () => { + 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), + `${(process.platform === 'darwin') ? 'Cmd' : 'Ctrl'}+A`) + assert.strictEqual(menu.getAcceleratorTextAt(1), 'Shift+A') + assert.strictEqual(menu.getAcceleratorTextAt(2), '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), + `${(process.platform === 'darwin') ? 'Cmd' : 'Ctrl'}+Tab`) + assert.strictEqual(menu.getAcceleratorTextAt(1), 'Shift+Tab') + assert.strictEqual(menu.getAcceleratorTextAt(2), '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), 'Shift+A') + assert.strictEqual(menu.getAcceleratorTextAt(1), '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), 'Ctrl+Shift+=') + assert.strictEqual(menu.getAcceleratorTextAt(1), 'Ctrl+Shift+=') + done() + }) + }) }) From 880b9bbb536141262b15a20a6ba728c70b2298e1 Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Tue, 30 Oct 2018 19:29:42 +0100 Subject: [PATCH 4/5] fix: allocate accelerator on the stack --- atom/browser/api/atom_api_menu.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index 393468bec78e3..c378e17f87297 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/atom/browser/api/atom_api_menu.cc @@ -156,10 +156,9 @@ base::string16 Menu::GetSublabelAt(int index) const { } base::string16 Menu::GetAcceleratorTextAt(int index) const { - auto* accelerator = new ui::Accelerator(); - model_->GetAcceleratorAtWithParams(index, true, accelerator); - - return accelerator ? accelerator->GetShortcutText() : base::string16(); + ui::Accelerator accelerator; + model_->GetAcceleratorAtWithParams(index, true, &accelerator); + return accelerator.GetShortcutText(); } bool Menu::IsItemCheckedAt(int index) const { From 2b0e4be5499c0bbea136f794f2c8cad2f2d788f4 Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Tue, 30 Oct 2018 22:25:12 +0100 Subject: [PATCH 5/5] fix: adjust tests to match expected behavior on mac --- spec/api-menu-item-spec.js | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/spec/api-menu-item-spec.js b/spec/api-menu-item-spec.js index f2ea6e796d593..6eb1a867eed12 100644 --- a/spec/api-menu-item-spec.js +++ b/spec/api-menu-item-spec.js @@ -391,6 +391,10 @@ describe('MenuItems', () => { }) 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' }, @@ -399,9 +403,11 @@ describe('MenuItems', () => { ]) assert.strictEqual(menu.getAcceleratorTextAt(0), - `${(process.platform === 'darwin') ? 'Cmd' : 'Ctrl'}+A`) - assert.strictEqual(menu.getAcceleratorTextAt(1), 'Shift+A') - assert.strictEqual(menu.getAcceleratorTextAt(2), 'Alt+A') + isDarwin() ? '⌘A' : 'Ctrl+A') + assert.strictEqual(menu.getAcceleratorTextAt(1), + isDarwin() ? '⇧A' : 'Shift+A') + assert.strictEqual(menu.getAcceleratorTextAt(2), + isDarwin() ? '⌥A' : 'Alt+A') done() }) @@ -413,9 +419,11 @@ describe('MenuItems', () => { ]) assert.strictEqual(menu.getAcceleratorTextAt(0), - `${(process.platform === 'darwin') ? 'Cmd' : 'Ctrl'}+Tab`) - assert.strictEqual(menu.getAcceleratorTextAt(1), 'Shift+Tab') - assert.strictEqual(menu.getAcceleratorTextAt(2), 'Alt+Tab') + isDarwin() ? '⌘⇥\u0000' : 'Ctrl+Tab') + assert.strictEqual(menu.getAcceleratorTextAt(1), + isDarwin() ? '⇧⇥\u0000' : 'Shift+Tab') + assert.strictEqual(menu.getAcceleratorTextAt(2), + isDarwin() ? '⌥⇥\u0000' : 'Alt+Tab') done() }) @@ -425,8 +433,10 @@ describe('MenuItems', () => { { label: 'text', accelerator: 'Shift+Shift+Tab' } ]) - assert.strictEqual(menu.getAcceleratorTextAt(0), 'Shift+A') - assert.strictEqual(menu.getAcceleratorTextAt(1), 'Shift+Tab') + assert.strictEqual(menu.getAcceleratorTextAt(0), + isDarwin() ? '⇧A' : 'Shift+A') + assert.strictEqual(menu.getAcceleratorTextAt(1), + isDarwin() ? '⇧⇥\u0000' : 'Shift+Tab') done() }) @@ -436,8 +446,10 @@ describe('MenuItems', () => { { label: 'text', accelerator: 'Control+Plus' } ]) - assert.strictEqual(menu.getAcceleratorTextAt(0), 'Ctrl+Shift+=') - assert.strictEqual(menu.getAcceleratorTextAt(1), 'Ctrl+Shift+=') + assert.strictEqual(menu.getAcceleratorTextAt(0), + isDarwin() ? '⌃⇧=' : 'Ctrl+Shift+=') + assert.strictEqual(menu.getAcceleratorTextAt(1), + isDarwin() ? '⌃⇧=' : 'Ctrl+Shift+=') done() }) })