diff --git a/src/vs/base/common/keyCodes.ts b/src/vs/base/common/keyCodes.ts index 5dc94ce0324..e2aff2ec8d7 100644 --- a/src/vs/base/common/keyCodes.ts +++ b/src/vs/base/common/keyCodes.ts @@ -378,6 +378,33 @@ export namespace KeyCodeUtils { export function fromUserSettings(key: string): KeyCode { return userSettingsUSMap.strToKeyCode(key) || userSettingsGeneralMap.strToKeyCode(key); } + + export function toElectronAccelerator(keyCode: KeyCode): string | null { + if (keyCode >= KeyCode.NUMPAD_0 && keyCode <= KeyCode.NUMPAD_DIVIDE) { + // [Electron Accelerators] Electron is able to parse numpad keys, but unfortunately it + // renders them just as regular keys in menus. For example, num0 is rendered as "0", + // numdiv is rendered as "/", numsub is rendered as "-". + // + // This can lead to incredible confusion, as it makes numpad based keybindings indistinguishable + // from keybindings based on regular keys. + // + // We therefore need to fall back to custom rendering for numpad keys. + return null; + } + + switch (keyCode) { + case KeyCode.UpArrow: + return 'Up'; + case KeyCode.DownArrow: + return 'Down'; + case KeyCode.LeftArrow: + return 'Left'; + case KeyCode.RightArrow: + return 'Right'; + } + + return uiMap.keyCodeToStr(keyCode); + } } /** @@ -442,7 +469,18 @@ export function createSimpleKeybinding(keybinding: number, OS: OperatingSystem): return new SimpleKeybinding(ctrlKey, shiftKey, altKey, metaKey, keyCode); } -export class SimpleKeybinding { +export interface Modifiers { + readonly ctrlKey: boolean; + readonly shiftKey: boolean; + readonly altKey: boolean; + readonly metaKey: boolean; +} + +export interface IBaseKeybinding extends Modifiers { + isDuplicateModifierCase(): boolean; +} + +export class SimpleKeybinding implements IBaseKeybinding { public readonly ctrlKey: boolean; public readonly shiftKey: boolean; public readonly altKey: boolean; diff --git a/src/vs/base/common/keybindingLabels.ts b/src/vs/base/common/keybindingLabels.ts index 2c36c13d7aa..1ee066014f2 100644 --- a/src/vs/base/common/keybindingLabels.ts +++ b/src/vs/base/common/keybindingLabels.ts @@ -3,6 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ +import { Modifiers } from 'vs/base/common/keyCodes'; import { OperatingSystem } from 'vs/base/common/platform'; import * as nls from 'vs/nls'; @@ -14,13 +15,6 @@ export interface ModifierLabels { readonly separator: string; } -export interface Modifiers { - readonly ctrlKey: boolean; - readonly shiftKey: boolean; - readonly altKey: boolean; - readonly metaKey: boolean; -} - export interface KeyLabelProvider { (keybinding: T): string | null; } diff --git a/src/vs/base/common/scanCode.ts b/src/vs/base/common/scanCode.ts index b0547e05bd4..15738143416 100644 --- a/src/vs/base/common/scanCode.ts +++ b/src/vs/base/common/scanCode.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { KeyCode } from 'vs/base/common/keyCodes'; +import { IBaseKeybinding, KeyCode } from 'vs/base/common/keyCodes'; /** * keyboardEvent.code @@ -228,7 +228,7 @@ export const IMMUTABLE_CODE_TO_KEY_CODE: KeyCode[] = []; */ export const IMMUTABLE_KEY_CODE_TO_CODE: ScanCode[] = []; -export class ScanCodeBinding { +export class ScanCodeBinding implements IBaseKeybinding { public readonly ctrlKey: boolean; public readonly shiftKey: boolean; public readonly altKey: boolean; diff --git a/src/vs/platform/keybinding/common/abstractKeybindingService.ts b/src/vs/platform/keybinding/common/abstractKeybindingService.ts index f3d57e2e8db..be482403630 100644 --- a/src/vs/platform/keybinding/common/abstractKeybindingService.ts +++ b/src/vs/platform/keybinding/common/abstractKeybindingService.ts @@ -173,8 +173,11 @@ export abstract class AbstractKeybindingService extends Disposable implements IK } public dispatchByUserSettingsLabel(userSettingsLabel: string, target: IContextKeyServiceTarget): void { + this._log(`/ Dispatching keybinding triggered via menu entry accelerator - ${userSettingsLabel}`); const keybindings = this.resolveUserBinding(userSettingsLabel); - if (keybindings.length >= 1) { + if (keybindings.length === 0) { + this._log(`\\ Could not resolve - ${userSettingsLabel}`); + } else { this._doDispatch(keybindings[0], target, /*isSingleModiferChord*/false); } } diff --git a/src/vs/platform/keybinding/common/baseResolvedKeybinding.ts b/src/vs/platform/keybinding/common/baseResolvedKeybinding.ts index 9b9ce33d972..4f4b8ba910a 100644 --- a/src/vs/platform/keybinding/common/baseResolvedKeybinding.ts +++ b/src/vs/platform/keybinding/common/baseResolvedKeybinding.ts @@ -4,11 +4,11 @@ *--------------------------------------------------------------------------------------------*/ import { illegalArgument } from 'vs/base/common/errors'; -import { AriaLabelProvider, ElectronAcceleratorLabelProvider, Modifiers, UILabelProvider, UserSettingsLabelProvider } from 'vs/base/common/keybindingLabels'; -import { KeybindingModifier, ResolvedKeybinding, ResolvedKeybindingPart } from 'vs/base/common/keyCodes'; +import { AriaLabelProvider, ElectronAcceleratorLabelProvider, UILabelProvider, UserSettingsLabelProvider } from 'vs/base/common/keybindingLabels'; +import { IBaseKeybinding, KeybindingModifier, ResolvedKeybinding, ResolvedKeybindingPart } from 'vs/base/common/keyCodes'; import { OperatingSystem } from 'vs/base/common/platform'; -export abstract class BaseResolvedKeybinding extends ResolvedKeybinding { +export abstract class BaseResolvedKeybinding extends ResolvedKeybinding { protected readonly _os: OperatingSystem; protected readonly _parts: T[]; @@ -32,7 +32,12 @@ export abstract class BaseResolvedKeybinding extends Resolv public getElectronAccelerator(): string | null { if (this._parts.length > 1) { - // Electron cannot handle chords + // [Electron Accelerators] Electron cannot handle chords + return null; + } + if (this._parts[0].isDuplicateModifierCase()) { + // [Electron Accelerators] Electron cannot handle modifier only keybindings + // e.g. "shift shift" return null; } return ElectronAcceleratorLabelProvider.toLabel(this._os, this._parts, (keybinding) => this._getElectronAccelerator(keybinding)); diff --git a/src/vs/platform/keybinding/common/usLayoutResolvedKeybinding.ts b/src/vs/platform/keybinding/common/usLayoutResolvedKeybinding.ts index acb485ec6cb..1b0d5744833 100644 --- a/src/vs/platform/keybinding/common/usLayoutResolvedKeybinding.ts +++ b/src/vs/platform/keybinding/common/usLayoutResolvedKeybinding.ts @@ -46,31 +46,8 @@ export class USLayoutResolvedKeybinding extends BaseResolvedKeybinding= KeyCode.NUMPAD_0 && keyCode <= KeyCode.NUMPAD_DIVIDE) { - // Electron cannot handle numpad keys - return null; - } - - switch (keyCode) { - case KeyCode.UpArrow: - return 'Up'; - case KeyCode.DownArrow: - return 'Down'; - case KeyCode.LeftArrow: - return 'Left'; - case KeyCode.RightArrow: - return 'Right'; - } - - return KeyCodeUtils.toString(keyCode); - } - protected _getElectronAccelerator(keybinding: SimpleKeybinding): string | null { - if (keybinding.isDuplicateModifierCase()) { - return null; - } - return this._keyCodeToElectronAccelerator(keybinding.keyCode); + return KeyCodeUtils.toElectronAccelerator(keybinding.keyCode); } protected _getUserSettingsLabel(keybinding: SimpleKeybinding): string | null { diff --git a/src/vs/workbench/services/keybinding/common/macLinuxKeyboardMapper.ts b/src/vs/workbench/services/keybinding/common/macLinuxKeyboardMapper.ts index f151fc5adf4..081ab4698eb 100644 --- a/src/vs/workbench/services/keybinding/common/macLinuxKeyboardMapper.ts +++ b/src/vs/workbench/services/keybinding/common/macLinuxKeyboardMapper.ts @@ -872,45 +872,24 @@ export class MacLinuxKeyboardMapper implements IKeyboardMapper { return this._scanCodeToDispatch[binding.scanCode]; } - private _getElectronLabelForKeyCode(keyCode: KeyCode): string | null { - if (keyCode >= KeyCode.NUMPAD_0 && keyCode <= KeyCode.NUMPAD_DIVIDE) { - // Electron cannot handle numpad keys - return null; - } - - switch (keyCode) { - case KeyCode.UpArrow: - return 'Up'; - case KeyCode.DownArrow: - return 'Down'; - case KeyCode.LeftArrow: - return 'Left'; - case KeyCode.RightArrow: - return 'Right'; - } - - // electron menus always do the correct rendering on Windows - return KeyCodeUtils.toString(keyCode); - } - public getElectronAcceleratorLabelForScanCodeBinding(binding: ScanCodeBinding | null): string | null { if (!binding) { return null; } - if (binding.isDuplicateModifierCase()) { - return null; - } const immutableKeyCode = IMMUTABLE_CODE_TO_KEY_CODE[binding.scanCode]; if (immutableKeyCode !== KeyCode.DependsOnKbLayout) { - return this._getElectronLabelForKeyCode(immutableKeyCode); + return KeyCodeUtils.toElectronAccelerator(immutableKeyCode); } // Check if this scanCode always maps to the same keyCode and back const constantKeyCode: KeyCode = this._scanCodeKeyCodeMapper.guessStableKeyCode(binding.scanCode); - if (!this._isUSStandard) { - // Electron cannot handle these key codes on anything else than standard US + if (this._OS === OperatingSystem.Linux && !this._isUSStandard) { + // [Electron Accelerators] On Linux, Electron does not handle correctly OEM keys. + // when using a different keyboard layout than US Standard. + // See https://github.com/microsoft/vscode/issues/23706 + // See https://github.com/microsoft/vscode/pull/134890#issuecomment-941671791 const isOEMKey = ( constantKeyCode === KeyCode.US_SEMICOLON || constantKeyCode === KeyCode.US_EQUAL @@ -929,14 +908,8 @@ export class MacLinuxKeyboardMapper implements IKeyboardMapper { } } - // See https://github.com/microsoft/vscode/issues/108880 - if (this._OS === OperatingSystem.Macintosh && binding.ctrlKey && !binding.metaKey && !binding.altKey && constantKeyCode === KeyCode.US_MINUS) { - // ctrl+- and ctrl+shift+- render very similarly in native macOS menus, leading to confusion - return null; - } - if (constantKeyCode !== KeyCode.DependsOnKbLayout) { - return this._getElectronLabelForKeyCode(constantKeyCode); + return KeyCodeUtils.toElectronAccelerator(constantKeyCode); } return null; diff --git a/src/vs/workbench/services/keybinding/common/windowsKeyboardMapper.ts b/src/vs/workbench/services/keybinding/common/windowsKeyboardMapper.ts index 3a8bbce2677..425b24d4c6e 100644 --- a/src/vs/workbench/services/keybinding/common/windowsKeyboardMapper.ts +++ b/src/vs/workbench/services/keybinding/common/windowsKeyboardMapper.ts @@ -67,9 +67,6 @@ export class WindowsNativeResolvedKeybinding extends BaseResolvedKeybinding= KeyCode.NUMPAD_0 && keyCode <= KeyCode.NUMPAD_DIVIDE) { - // Electron cannot handle numpad keys - return null; - } - - switch (keyCode) { - case KeyCode.UpArrow: - return 'Up'; - case KeyCode.DownArrow: - return 'Down'; - case KeyCode.LeftArrow: - return 'Left'; - case KeyCode.RightArrow: - return 'Right'; - } - - // electron menus always do the correct rendering on Windows - return KeyCodeUtils.toString(keyCode); + return KeyCodeUtils.toElectronAccelerator(keybinding.keyCode); } private _getLabelForKeyCode(keyCode: KeyCode): string { diff --git a/src/vs/workbench/services/keybinding/test/electron-browser/mac_de_ch.txt b/src/vs/workbench/services/keybinding/test/electron-browser/mac_de_ch.txt index d4274eed3e0..973d6a28603 100644 --- a/src/vs/workbench/services/keybinding/test/electron-browser/mac_de_ch.txt +++ b/src/vs/workbench/services/keybinding/test/electron-browser/mac_de_ch.txt @@ -469,14 +469,14 @@ isUSStandard: false | Shift+Alt+Period | : | Shift+Alt+; | | Shift+Alt+. | shift+alt+[Period] | null | shift+alt+[Period] | NO | | Ctrl+Shift+Alt+Period | ÷ | Ctrl+Shift+Alt+; | | Ctrl+Shift+Alt+. | ctrl+shift+alt+[Period] | null | ctrl+shift+alt+[Period] | NO | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- -| Slash | - | - | | - | - | null | [Slash] | | -| Ctrl+Slash | - | Ctrl+- | | Ctrl+- | ctrl+- | null | ctrl+[Slash] | | -| Shift+Slash | _ | Shift+- | | Shift+- | shift+- | null | shift+[Slash] | | -| Ctrl+Shift+Slash | _ | Ctrl+Shift+- | | Ctrl+Shift+- | ctrl+shift+- | null | ctrl+shift+[Slash] | | -| Alt+Slash | - | Alt+- | | Alt+- | alt+- | null | alt+[Slash] | | -| Ctrl+Alt+Slash | – | Ctrl+Alt+- | | Ctrl+Alt+- | ctrl+alt+- | null | ctrl+alt+[Slash] | | -| Shift+Alt+Slash | _ | Shift+Alt+- | | Shift+Alt+- | shift+alt+- | null | shift+alt+[Slash] | | -| Ctrl+Shift+Alt+Slash | — | Ctrl+Shift+Alt+- | | Ctrl+Shift+Alt+- | ctrl+shift+alt+- | null | ctrl+shift+alt+[Slash] | | +| Slash | - | - | | - | - | - | [Slash] | | +| Ctrl+Slash | - | Ctrl+- | | Ctrl+- | ctrl+- | Ctrl+- | ctrl+[Slash] | | +| Shift+Slash | _ | Shift+- | | Shift+- | shift+- | Shift+- | shift+[Slash] | | +| Ctrl+Shift+Slash | _ | Ctrl+Shift+- | | Ctrl+Shift+- | ctrl+shift+- | Ctrl+Shift+- | ctrl+shift+[Slash] | | +| Alt+Slash | - | Alt+- | | Alt+- | alt+- | Alt+- | alt+[Slash] | | +| Ctrl+Alt+Slash | – | Ctrl+Alt+- | | Ctrl+Alt+- | ctrl+alt+- | Ctrl+Alt+- | ctrl+alt+[Slash] | | +| Shift+Alt+Slash | _ | Shift+Alt+- | | Shift+Alt+- | shift+alt+- | Shift+Alt+- | shift+alt+[Slash] | | +| Ctrl+Shift+Alt+Slash | — | Ctrl+Shift+Alt+- | | Ctrl+Shift+Alt+- | ctrl+shift+alt+- | Ctrl+Shift+Alt+- | ctrl+shift+alt+[Slash] | | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | HW Code combination | Key | KeyCode combination | Pri | UI label | User settings | Electron accelerator | Dispatching string | WYSIWYG | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/src/vs/workbench/services/keybinding/test/electron-browser/mac_en_us.txt b/src/vs/workbench/services/keybinding/test/electron-browser/mac_en_us.txt index 5881abce1cf..7a83477293f 100644 --- a/src/vs/workbench/services/keybinding/test/electron-browser/mac_en_us.txt +++ b/src/vs/workbench/services/keybinding/test/electron-browser/mac_en_us.txt @@ -345,9 +345,9 @@ isUSStandard: true | HW Code combination | Key | KeyCode combination | Pri | UI label | User settings | Electron accelerator | Dispatching string | WYSIWYG | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | Minus | - | - | | - | - | - | [Minus] | | -| Ctrl+Minus | - | Ctrl+- | | Ctrl+- | ctrl+- | null | ctrl+[Minus] | | +| Ctrl+Minus | - | Ctrl+- | | Ctrl+- | ctrl+- | Ctrl+- | ctrl+[Minus] | | | Shift+Minus | _ | Shift+- | | Shift+- | shift+- | Shift+- | shift+[Minus] | | -| Ctrl+Shift+Minus | _ | Ctrl+Shift+- | | Ctrl+Shift+- | ctrl+shift+- | null | ctrl+shift+[Minus] | | +| Ctrl+Shift+Minus | _ | Ctrl+Shift+- | | Ctrl+Shift+- | ctrl+shift+- | Ctrl+Shift+- | ctrl+shift+[Minus] | | | Alt+Minus | - | Alt+- | | Alt+- | alt+- | Alt+- | alt+[Minus] | | | Ctrl+Alt+Minus | – | Ctrl+Alt+- | | Ctrl+Alt+- | ctrl+alt+- | Ctrl+Alt+- | ctrl+alt+[Minus] | | | Shift+Alt+Minus | _ | Shift+Alt+- | | Shift+Alt+- | shift+alt+- | Shift+Alt+- | shift+alt+[Minus] | | diff --git a/src/vs/workbench/services/keybinding/test/electron-browser/mac_zh_hant.txt b/src/vs/workbench/services/keybinding/test/electron-browser/mac_zh_hant.txt index 225d7411760..a1064d4b2af 100644 --- a/src/vs/workbench/services/keybinding/test/electron-browser/mac_zh_hant.txt +++ b/src/vs/workbench/services/keybinding/test/electron-browser/mac_zh_hant.txt @@ -353,14 +353,14 @@ isUSStandard: false | Shift+Alt+Minus | _ | Shift+Alt+- | | Shift+Alt+ㄦ | shift+alt+[Minus] | null | shift+alt+[Minus] | NO | | Ctrl+Shift+Alt+Minus | _ | Ctrl+Shift+Alt+- | | Ctrl+Shift+Alt+ㄦ | ctrl+shift+alt+[Minus] | null | ctrl+shift+alt+[Minus] | NO | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- -| Equal | = | = | | = | = | null | [Equal] | | -| Ctrl+Equal | = | Ctrl+= | | Ctrl+= | ctrl+= | null | ctrl+[Equal] | | -| Shift+Equal | + | Shift+= | | Shift+= | shift+= | null | shift+[Equal] | | -| Ctrl+Shift+Equal | + | Ctrl+Shift+= | | Ctrl+Shift+= | ctrl+shift+= | null | ctrl+shift+[Equal] | | -| Alt+Equal | = | Alt+= | | Alt+= | alt+= | null | alt+[Equal] | | -| Ctrl+Alt+Equal | = | Ctrl+Alt+= | | Ctrl+Alt+= | ctrl+alt+= | null | ctrl+alt+[Equal] | | -| Shift+Alt+Equal | + | Shift+Alt+= | | Shift+Alt+= | shift+alt+= | null | shift+alt+[Equal] | | -| Ctrl+Shift+Alt+Equal | + | Ctrl+Shift+Alt+= | | Ctrl+Shift+Alt+= | ctrl+shift+alt+= | null | ctrl+shift+alt+[Equal] | | +| Equal | = | = | | = | = | = | [Equal] | | +| Ctrl+Equal | = | Ctrl+= | | Ctrl+= | ctrl+= | Ctrl+= | ctrl+[Equal] | | +| Shift+Equal | + | Shift+= | | Shift+= | shift+= | Shift+= | shift+[Equal] | | +| Ctrl+Shift+Equal | + | Ctrl+Shift+= | | Ctrl+Shift+= | ctrl+shift+= | Ctrl+Shift+= | ctrl+shift+[Equal] | | +| Alt+Equal | = | Alt+= | | Alt+= | alt+= | Alt+= | alt+[Equal] | | +| Ctrl+Alt+Equal | = | Ctrl+Alt+= | | Ctrl+Alt+= | ctrl+alt+= | Ctrl+Alt+= | ctrl+alt+[Equal] | | +| Shift+Alt+Equal | + | Shift+Alt+= | | Shift+Alt+= | shift+alt+= | Shift+Alt+= | shift+alt+[Equal] | | +| Ctrl+Shift+Alt+Equal | + | Ctrl+Shift+Alt+= | | Ctrl+Shift+Alt+= | ctrl+shift+alt+= | Ctrl+Shift+Alt+= | ctrl+shift+alt+[Equal] | | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | BracketLeft | 「 | | | 「 | [BracketLeft] | null | [BracketLeft] | NO | | Ctrl+BracketLeft | 「 | | | Ctrl+「 | ctrl+[BracketLeft] | null | ctrl+[BracketLeft] | NO | diff --git a/src/vs/workbench/services/keybinding/test/electron-browser/windowsKeyboardMapper.test.ts b/src/vs/workbench/services/keybinding/test/electron-browser/windowsKeyboardMapper.test.ts index eb1b9c350b3..f63c3892478 100644 --- a/src/vs/workbench/services/keybinding/test/electron-browser/windowsKeyboardMapper.test.ts +++ b/src/vs/workbench/services/keybinding/test/electron-browser/windowsKeyboardMapper.test.ts @@ -100,7 +100,7 @@ suite('keyboardMapper - WINDOWS de_ch', () => { [{ label: 'Ctrl+^', ariaLabel: 'Control+^', - electronAccelerator: null, + electronAccelerator: 'Ctrl+]', userSettingsLabel: 'ctrl+oem_6', isWYSIWYG: false, isChord: false, @@ -125,7 +125,7 @@ suite('keyboardMapper - WINDOWS de_ch', () => { { label: 'Ctrl+^', ariaLabel: 'Control+^', - electronAccelerator: null, + electronAccelerator: 'Ctrl+]', userSettingsLabel: 'ctrl+oem_6', isWYSIWYG: false, isChord: false, @@ -142,7 +142,7 @@ suite('keyboardMapper - WINDOWS de_ch', () => { [{ label: 'Shift+^', ariaLabel: 'Shift+^', - electronAccelerator: null, + electronAccelerator: 'Shift+]', userSettingsLabel: 'shift+oem_6', isWYSIWYG: false, isChord: false, @@ -159,7 +159,7 @@ suite('keyboardMapper - WINDOWS de_ch', () => { [{ label: 'Ctrl+§', ariaLabel: 'Control+§', - electronAccelerator: null, + electronAccelerator: 'Ctrl+/', userSettingsLabel: 'ctrl+oem_2', isWYSIWYG: false, isChord: false, @@ -176,7 +176,7 @@ suite('keyboardMapper - WINDOWS de_ch', () => { [{ label: 'Ctrl+Shift+§', ariaLabel: 'Control+Shift+§', - electronAccelerator: null, + electronAccelerator: 'Ctrl+Shift+/', userSettingsLabel: 'ctrl+shift+oem_2', isWYSIWYG: false, isChord: false,