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) {