fix(packages): remove redundant "Shift" modifier from playback rate hotkeys (#1290)

This commit is contained in:
rahim
2026-04-08 14:47:14 -07:00
committed by GitHub
parent 1346d869ae
commit a0fd3cbda8
8 changed files with 91 additions and 21 deletions
+18 -3
View File
@@ -31,8 +31,8 @@ const MODIFIER_KEYS = new Set(['shift', 'ctrl', 'alt', 'meta']);
*
* @example
* ```ts
* parseHotkeyPattern('Shift+>');
* // [{ modifiers: Set('shift'), key: '>', originalKey: '>' }]
* parseHotkeyPattern('>');
* // [{ modifiers: Set(), key: '>', originalKey: '>' }]
*
* parseHotkeyPattern('0-9');
* // 10 bindings, one per digit
@@ -70,6 +70,16 @@ export function parseHotkeyPattern(pattern: string): ParsedHotkeyBinding[] {
return [{ modifiers, key, originalKey: rawKey }];
}
/**
* Single non-letter character — Shift was used to produce the character itself,
* not as a deliberate modifier (e.g. Shift+. → ">", Shift+/ → "?").
* 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 {
return key.length === 1 && !/[a-z]/i.test(key);
}
/** Whether a parsed binding matches a keyboard event. */
export function matchesHotkeyEvent(binding: ParsedHotkeyBinding, event: KeyboardEvent): boolean {
// IME composition filtering.
@@ -78,8 +88,13 @@ 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;
// Exact modifier matching — all four must agree.
if (event.shiftKey !== binding.modifiers.has('shift')) return false;
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 (event.metaKey !== binding.modifiers.has('meta')) return false;
@@ -13,7 +13,7 @@ describe('toAriaKeyShortcut', () => {
});
it('maps shift to Shift', () => {
expect(toAriaKeyShortcut(parseHotkeyPattern('Shift+>'))).toBe('Shift+>');
expect(toAriaKeyShortcut(parseHotkeyPattern('Shift+ArrowLeft'))).toBe('Shift+ArrowLeft');
});
it('formats multiple modifiers in consistent order', () => {
@@ -13,10 +13,10 @@ describe('parseHotkeyPattern', () => {
});
it('parses Shift modifier', () => {
const result = parseHotkeyPattern('Shift+>');
const result = parseHotkeyPattern('Shift+ArrowLeft');
expect(result).toHaveLength(1);
expect(result[0]!.key).toBe('>');
expect(result[0]!.key).toBe('arrowleft');
expect(result[0]!.modifiers.has('shift')).toBe(true);
expect(result[0]!.modifiers.size).toBe(1);
});
@@ -120,6 +120,60 @@ describe('matchesHotkeyEvent', () => {
const binding = parseHotkeyPattern('k')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('j'))).toBe(false);
});
describe('implicit shift 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);
});
it('matches < when Shift is held (US keyboard: Shift+, produces <)', () => {
const binding = parseHotkeyPattern('<')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('<', { shiftKey: true }))).toBe(true);
});
it('matches > when Shift is NOT held (European keyboard: > is unshifted)', () => {
const binding = parseHotkeyPattern('>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>'))).toBe(true);
});
it('matches ? when Shift is held', () => {
const binding = parseHotkeyPattern('?')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('?', { shiftKey: true }))).toBe(true);
});
it('matches Ctrl+> with Ctrl and Shift held', () => {
const binding = parseHotkeyPattern('Ctrl+>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { ctrlKey: true, shiftKey: true }))).toBe(true);
});
it('rejects Ctrl+> when only Shift is held (no Ctrl)', () => {
const binding = parseHotkeyPattern('Ctrl+>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { shiftKey: true }))).toBe(false);
});
it('rejects letter k when Shift is held (Shift changes letter case)', () => {
const binding = parseHotkeyPattern('k')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('K', { shiftKey: true }))).toBe(false);
});
it('requires exact Shift for named keys', () => {
const binding = parseHotkeyPattern('Shift+ArrowLeft')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('ArrowLeft', { shiftKey: true }))).toBe(true);
expect(matchesHotkeyEvent(binding, createEvent('ArrowLeft'))).toBe(false);
});
it('rejects ArrowLeft when Shift is held but binding has no Shift', () => {
const binding = parseHotkeyPattern('ArrowLeft')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('ArrowLeft', { shiftKey: true }))).toBe(false);
});
it('Shift+> binding requires Shift held', () => {
const binding = parseHotkeyPattern('Shift+>')[0]!;
expect(matchesHotkeyEvent(binding, createEvent('>', { shiftKey: true }))).toBe(true);
expect(matchesHotkeyEvent(binding, createEvent('>'))).toBe(false);
});
});
});
describe('createHotkey', () => {