fix(core): treat Alt as implicit modifier for non-letter character hotkeys (#1304)

This commit is contained in:
rahim
2026-04-10 13:35:17 -07:00
committed by GitHub
parent e62f1ef267
commit 57259b342a
3 changed files with 54 additions and 10 deletions
+1 -1
View File
@@ -166,7 +166,7 @@ Only `0-9` is supported. Arbitrary ranges (e.g., `a-z`) are not — there's no u
- Patterns are parsed into `{ modifiers, key }` at registration time — no parsing per event.
- `event.key` is compared case-insensitively.
- **Exact modifier matching** — all specified modifiers must be active, all unspecified must be inactive. `k` does not fire when Ctrl+K is pressed.
- **Implicit Shift** — For single non-letter character keys (`>`, `<`, `?`, `!`, etc.), `shiftKey` is ignored during matching. These characters require Shift on some layouts but not others. Explicitly specifying `Shift+>` still requires Shift to be held.
- **Implicit modifiers (Shift, Alt)** — For single non-letter character keys (`>`, `<`, `?`, `!`, etc.), `shiftKey` and `altKey` are ignored during matching. These characters require Shift on some layouts (US: `Shift+.` produces `>`), Alt/Option on others (some Mac layouts: `Option+Shift` produces `>`), or neither (some European layouts: `>` is an unshifted key). Explicitly specifying `Shift+>` or `Alt+>` still requires that modifier to be held. `ctrlKey` and `metaKey` remain strict — they are intentional modifier shortcuts, never layout artifacts.
- `Mod` resolves at parse time based on platform detection (`navigator.userAgentData?.platform` with `navigator.platform` fallback).
- IME composition input is filtered — events where `event.key === 'Unidentified'` are skipped.
+11 -8
View File
@@ -71,12 +71,13 @@ export function parseHotkeyPattern(pattern: string): ParsedHotkeyBinding[] {
}
/**
* Single non-letter character Shift was used to produce the character itself,
* not as a deliberate modifier (e.g. Shift+. ">", Shift+/ "?").
* Single non-letter character layout-dependent modifiers (Shift, Alt/Option)
* were used to produce the character itself, not as deliberate modifiers
* (e.g. Shift+. ">", Option+Shift ">" on some Mac layouts).
* Letters excluded because Shift changes case intentionally (k vs K).
* Named keys excluded because event.key.length > 1 (ArrowLeft, Tab, etc.).
*/
function isImplicitShift(key: string): boolean {
function isImplicitModifierKey(key: string): boolean {
return key.length === 1 && !/[a-z]/i.test(key);
}
@@ -88,15 +89,17 @@ export function matchesHotkeyEvent(binding: ParsedHotkeyBinding, event: Keyboard
// Case-insensitive key comparison.
if (event.key.toLowerCase() !== binding.key) return false;
// Implicit Shift: non-letter character keys (>, <, ?, !) may require Shift to produce
// on some layouts but not others. Treat Shift as present only when the event has it,
// but ignore extra shiftKey when the binding doesn't ask for it.
const shiftKey = isImplicitShift(event.key) ? event.shiftKey && binding.modifiers.has('shift') : event.shiftKey;
// Implicit modifiers: non-letter character keys (>, <, ?, !) may require Shift or
// Alt (Option) to produce on some layouts but not others. Treat these modifiers as
// present only when the binding explicitly asks for them.
const implicit = isImplicitModifierKey(event.key);
const shiftKey = implicit ? event.shiftKey && binding.modifiers.has('shift') : event.shiftKey;
const altKey = implicit ? event.altKey && binding.modifiers.has('alt') : event.altKey;
// Exact modifier matching — all four must agree.
if (shiftKey !== binding.modifiers.has('shift')) return false;
if (event.ctrlKey !== binding.modifiers.has('ctrl')) return false;
if (event.altKey !== binding.modifiers.has('alt')) return false;
if (altKey !== binding.modifiers.has('alt')) return false;
if (event.metaKey !== binding.modifiers.has('meta')) return false;
return true;
@@ -121,7 +121,7 @@ describe('matchesHotkeyEvent', () => {
expect(matchesHotkeyEvent(binding, createEvent('j'))).toBe(false);
});
describe('implicit shift for non-letter characters', () => {
describe('implicit modifiers for non-letter characters', () => {
it('matches > when Shift is held (US keyboard: Shift+. produces >)', () => {
const binding = parseHotkeyPattern('>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { shiftKey: true }))).toBe(true);
@@ -173,6 +173,47 @@ describe('matchesHotkeyEvent', () => {
expect(matchesHotkeyEvent(binding, createEvent('>', { shiftKey: true }))).toBe(true);
expect(matchesHotkeyEvent(binding, createEvent('>'))).toBe(false);
});
it('matches > when Alt is held (Mac Option key produces >)', () => {
const binding = parseHotkeyPattern('>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { altKey: true }))).toBe(true);
});
it('matches > when Alt and Shift are held (Mac Option+Shift produces >)', () => {
const binding = parseHotkeyPattern('>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { altKey: true, shiftKey: true }))).toBe(true);
});
it('matches < when Alt is held (Mac Option key produces <)', () => {
const binding = parseHotkeyPattern('<')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('<', { altKey: true }))).toBe(true);
});
it('Alt+> binding requires Alt held', () => {
const binding = parseHotkeyPattern('Alt+>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { altKey: true }))).toBe(true);
expect(matchesHotkeyEvent(binding, createEvent('>'))).toBe(false);
});
it('rejects letter k when Alt is held (Alt is not implicit for letters)', () => {
const binding = parseHotkeyPattern('k')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('k', { altKey: true }))).toBe(false);
});
it('rejects ArrowLeft when Alt is held but binding has no Alt', () => {
const binding = parseHotkeyPattern('ArrowLeft')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('ArrowLeft', { altKey: true }))).toBe(false);
});
it('rejects > when Ctrl is held but binding has no Ctrl', () => {
const binding = parseHotkeyPattern('>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { ctrlKey: true }))).toBe(false);
});
it('rejects > when Meta is held but binding has no Meta', () => {
const binding = parseHotkeyPattern('>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { metaKey: true }))).toBe(false);
});
});
});