From 16e16fe3f050cd6317b32838e8b73ecd97259b42 Mon Sep 17 00:00:00 2001 From: Radoslav Karaivanov Date: Tue, 7 Apr 2026 13:24:05 +0300 Subject: [PATCH] fix(key-bindings): Handle overlapping key bindings correctly - When multiple keys are pressed in quick succession, it's possible for their keydown events to overlap before the corresponding keyup events fire. This can lead to incorrect binding matches if the logic doesn't account for the current state of pressed keys. The fix involves ensuring that the key event handling logic correctly identifies which keys are currently pressed and matches them against the defined bindings, even when overlaps occur. - Additionally, the skip logic is improved to prevent keys from getting stuck if the skip condition changes between keydown and keyup events. --- .../common/controllers/key-bindings.spec.ts | 64 +++++++++++++++++++ .../common/controllers/key-bindings.ts | 50 +++++++++++---- src/components/common/utils.spec.ts | 4 +- 3 files changed, 104 insertions(+), 14 deletions(-) diff --git a/src/components/common/controllers/key-bindings.spec.ts b/src/components/common/controllers/key-bindings.spec.ts index ba6193d2c..a7f4b19bd 100644 --- a/src/components/common/controllers/key-bindings.spec.ts +++ b/src/components/common/controllers/key-bindings.spec.ts @@ -10,6 +10,7 @@ import { simulateKeyboard } from '../utils.spec.js'; import { addKeybindings, altKey, + arrowDown, arrowUp, enterKey, shiftKey, @@ -245,6 +246,69 @@ describe('Key bindings controller', () => { dispatch(repeatInstance, 'keydown', 'b', true); expect(repeatInstance.callCount).to.equal(1); }); + + it('should fire the correct binding when keys overlap (keyup not yet fired for previous key)', () => { + // Simulate pressing ArrowDown a couple of times, then pressing ArrowUp before + // ArrowDown's keyup fires — mimicking rapid navigation where keys overlap. + let repeatInstance2: LitElement & { + lastKey: string | undefined; + downCount: number; + upCount: number; + }; + + const repeatTag2 = defineCE( + class extends LitElement { + public lastKey: string | undefined = undefined; + public downCount = 0; + public upCount = 0; + + constructor() { + super(); + addKeybindings(this) + .set( + arrowDown, + () => { + this.lastKey = arrowDown; + this.downCount++; + }, + { repeat: true } + ) + .set( + arrowUp, + () => { + this.lastKey = arrowUp; + this.upCount++; + }, + { repeat: true } + ); + } + } + ); + + return fixture( + html`<${unsafeStatic(repeatTag2)}>` + ).then((el) => { + repeatInstance2 = el; + + // Press ArrowDown twice (with repeat) + dispatch(repeatInstance2, 'keydown', arrowDown); + dispatch(repeatInstance2, 'keydown', arrowDown, true); + + // Press ArrowUp while ArrowDown is still "held" (no keyup yet) + dispatch(repeatInstance2, 'keydown', arrowUp); + + expect(repeatInstance2.upCount).to.equal(1); + expect(repeatInstance2.lastKey).to.equal(arrowUp); + + // Release both keys + dispatch(repeatInstance2, 'keyup', arrowDown); + dispatch(repeatInstance2, 'keyup', arrowUp); + + // Press ArrowDown again — should still work cleanly after the overlap + dispatch(repeatInstance2, 'keydown', arrowDown); + expect(repeatInstance2.downCount).to.equal(3); + }); + }); }); }); diff --git a/src/components/common/controllers/key-bindings.ts b/src/components/common/controllers/key-bindings.ts index 990cb32bf..620638159 100644 --- a/src/components/common/controllers/key-bindings.ts +++ b/src/components/common/controllers/key-bindings.ts @@ -27,7 +27,7 @@ export const tabKey = 'Tab' as const; /* Modifiers */ export const altKey = 'Alt' as const; -export const ctrlKey = 'Ctrl' as const; +export const ctrlKey = 'Control' as const; export const metaKey = 'Meta' as const; export const shiftKey = 'Shift' as const; @@ -135,9 +135,20 @@ interface KeyBinding { //#region Internal functions and constants -const MODIFIERS = new Set(['alt', 'ctrl', 'meta', 'shift']); +const MODIFIERS = new Set(['alt', 'control', 'meta', 'shift']); const ALL_MODIFIER_VALUES = Array.from(MODIFIERS).sort(); +/** + * Maps internal modifier names to their corresponding `KeyboardEvent` boolean property names. + * Needed because `ctrlKey` in the DOM is the property for the `'control'` modifier (not `'controlKey'`). + */ +export const MODIFIER_EVENT_KEYS: Record = { + alt: 'altKey', + control: 'ctrlKey', + meta: 'metaKey', + shift: 'shiftKey', +}; + function normalizeKeys(keys: string | string[]): string[] { return asArray(keys).map((key) => key.toLowerCase()); } @@ -285,11 +296,9 @@ class KeyBindingController implements ReactiveController { return Boolean(getElementFromPath(this._skipSelector, event)); } - if (isFunction(skip)) { - return skip.call(this._host, event.target as Element, event); - } - - return false; + return isFunction(skip) + ? skip.call(this._host, event.target as Element, event) + : false; } //#endregion @@ -335,6 +344,11 @@ class KeyBindingController implements ReactiveController { */ private _handleKeyEvent(event: KeyboardEvent): void { if (this._shouldSkip(event)) { + // Always clean up on keyup regardless of whether the event is otherwise skipped. + const key = event.key.toLowerCase(); + if (!MODIFIERS.has(key) && isKeyup(event)) { + this._pressedKeys.delete(key); + } return; } @@ -345,14 +359,24 @@ class KeyBindingController implements ReactiveController { } const activeModifiers = ALL_MODIFIER_VALUES.filter( - (mod) => event[`${mod}Key` as keyof KeyboardEvent] + (mod) => event[MODIFIER_EVENT_KEYS[mod] as keyof KeyboardEvent] ); const combination = createCombinationKey( Array.from(this._pressedKeys), activeModifiers ); - const binding = this._bindings.get(combination); + let binding = this._bindings.get(combination); + + // When multiple non-modifier keys are simultaneously in _pressedKeys (due to overlapping + // key presses, e.g. pressing ArrowUp before ArrowDown's keyup fires), the full combination + // won't match any single-key binding. Fall back to just the current key + modifiers so that + // single-key bindings continue to fire even when other keys are still "held". + if (!binding && this._pressedKeys.size > 1) { + binding = this._bindings.get( + createCombinationKey([key], activeModifiers) + ); + } if (binding && this._bindingMatches(binding, event)) { this._applyEventModifiers(binding, event); @@ -445,9 +469,11 @@ class KeyBindingController implements ReactiveController { return { unsubscribe: () => { - this._observedElement?.removeEventListener('keydown', this); - this._observedElement?.removeEventListener('keyup', this); - this._observedElement = undefined; + element.removeEventListener('keydown', this); + element.removeEventListener('keyup', this); + if (this._observedElement === element) { + this._observedElement = undefined; + } }, }; } diff --git a/src/components/common/utils.spec.ts b/src/components/common/utils.spec.ts index 168267606..4a782beb4 100644 --- a/src/components/common/utils.spec.ts +++ b/src/components/common/utils.spec.ts @@ -7,7 +7,7 @@ import { } from '@open-wc/testing'; import type { TemplateResult } from 'lit'; import { type CalendarDay, toCalendarDay } from '../calendar/model.js'; -import { parseKeys } from './controllers/key-bindings.js'; +import { MODIFIER_EVENT_KEYS, parseKeys } from './controllers/key-bindings.js'; import type { IgcFormControl } from './mixins/forms/types.js'; import { toKebabCase } from './util.js'; @@ -348,7 +348,7 @@ export function simulateKeyboard( const eventOptions: Record = {}; for (const each of modifiers) { - eventOptions[`${each}Key`] = true; + eventOptions[MODIFIER_EVENT_KEYS[each]] = true; } for (const key of keys) {