From 93851054b8522534402e6a4ddec421186a1e8e05 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Wed, 6 Mar 2024 10:07:46 +0000 Subject: [PATCH 1/7] Allow keybindings to opt out of preventing default --- packages/commands/src/index.ts | 45 +++++++++++----- packages/commands/tests/src/index.spec.ts | 64 +++++++++++++++++++++++ 2 files changed, 96 insertions(+), 13 deletions(-) diff --git a/packages/commands/src/index.ts b/packages/commands/src/index.ts index c93077e77..ff4722638 100644 --- a/packages/commands/src/index.ts +++ b/packages/commands/src/index.ts @@ -522,7 +522,7 @@ export class CommandRegistry { } // Get the normalized keystroke for the event. - let keystroke = CommandRegistry.keystrokeForKeydownEvent(event); + const keystroke = CommandRegistry.keystrokeForKeydownEvent(event); // If the keystroke is not valid for the keyboard layout, replay // any suppressed events and clear the pending state. @@ -536,15 +536,17 @@ export class CommandRegistry { this._keystrokes.push(keystroke); // Find the exact and partial matches for the key sequence. - let { exact, partial } = Private.matchKeyBinding( + const { exact, partial } = Private.matchKeyBinding( this._keyBindings, this._keystrokes, event ); + // Whether there is any partial match. + const hasPartial = partial.length !== 0; // If there is no exact match and no partial match, replay // any suppressed events and clear the pending state. - if (!exact && !partial) { + if (!exact && !hasPartial) { this._replayKeydownEvents(); this._clearPendingState(); return; @@ -552,13 +554,15 @@ export class CommandRegistry { // Stop propagation of the event. If there is only a partial match, // the event will be replayed if a final exact match never occurs. - event.preventDefault(); - event.stopPropagation(); + if (exact?.preventDefault || partial.some(match => match.preventDefault)) { + event.preventDefault(); + event.stopPropagation(); + } // If there is an exact match but no partial match, the exact match // can be dispatched immediately. The pending state is cleared so // the next key press starts from the default state. - if (exact && !partial) { + if (exact && !hasPartial) { this._executeKeyBinding(exact); this._clearPendingState(); return; @@ -1027,6 +1031,13 @@ export namespace CommandRegistry { * If provided, this will override `keys` on Linux platforms. */ linuxKeys?: string[]; + + /** + * Whether to prevent default action of the keyboard events during sequence matching. + * + * The default value is `true`. + */ + preventDefault?: boolean; } /** @@ -1055,6 +1066,13 @@ export namespace CommandRegistry { * The arguments for the command. */ readonly args: ReadonlyPartialJSONObject; + + /** + * Whether to prevent default action of the keyboard events during sequence matching. + * + * The default value is `true`. + */ + readonly preventDefault?: boolean; } /** @@ -1372,7 +1390,8 @@ namespace Private { keys: CommandRegistry.normalizeKeys(options), selector: validateSelector(options), command: options.command, - args: options.args || JSONExt.emptyObject + args: options.args || JSONExt.emptyObject, + preventDefault: options.preventDefault ?? true }; } @@ -1386,9 +1405,9 @@ namespace Private { exact: CommandRegistry.IKeyBinding | null; /** - * Whether there are bindings which partially match the sequence. + * The key bindings which partially match the sequence. */ - partial: boolean; + partial: CommandRegistry.IKeyBinding[]; } /** @@ -1405,8 +1424,8 @@ namespace Private { // The current best exact match. let exact: CommandRegistry.IKeyBinding | null = null; - // Whether a partial match has been found. - let partial = false; + // Partial matches. + let partial = []; // The match distance for the exact match. let distance = Infinity; @@ -1430,8 +1449,8 @@ namespace Private { // If it is a partial match and no other partial match has been // found, ensure the selector matches and set the partial flag. if (sqm === SequenceMatch.Partial) { - if (!partial && targetDistance(binding.selector, event) !== -1) { - partial = true; + if (targetDistance(binding.selector, event) !== -1) { + partial.push(binding); } continue; } diff --git a/packages/commands/tests/src/index.spec.ts b/packages/commands/tests/src/index.spec.ts index 3a4862467..4d627722e 100644 --- a/packages/commands/tests/src/index.spec.ts +++ b/packages/commands/tests/src/index.spec.ts @@ -821,6 +821,70 @@ describe('@lumino/commands', () => { expect(called).to.equal(false); }); + it('should prevent default on dispatch', () => { + registry.addCommand('test', { + execute: () => void 0 + }); + registry.addKeyBinding({ + keys: ['Ctrl ;'], + selector: `#${elem.id}`, + command: 'test' + }); + const event = new KeyboardEvent('keydown', { + keyCode: 59, + ctrlKey: true + }); + let defaultPrevented = false; + event.preventDefault = () => { + defaultPrevented = true; + }; + elem.dispatchEvent(event); + expect(defaultPrevented).to.equal(true); + }); + + it('should not prevent default when sequence does not match', () => { + registry.addCommand('test', { + execute: () => void 0 + }); + registry.addKeyBinding({ + keys: ['Ctrl ;'], + selector: `#${elem.id}`, + command: 'test' + }); + const event = new KeyboardEvent('keydown', { + keyCode: 59, + ctrlKey: false + }); + let defaultPrevented = false; + event.preventDefault = () => { + defaultPrevented = true; + }; + elem.dispatchEvent(event); + expect(defaultPrevented).to.equal(false); + }); + + it('should not prevent default if keybinding opts out', () => { + registry.addCommand('test', { + execute: () => void 0 + }); + registry.addKeyBinding({ + keys: ['Ctrl ;'], + selector: `#${elem.id}`, + command: 'test', + preventDefault: false + }); + const event = new KeyboardEvent('keydown', { + keyCode: 59, + ctrlKey: true + }); + let defaultPrevented = false; + event.preventDefault = () => { + defaultPrevented = true; + }; + elem.dispatchEvent(event); + expect(defaultPrevented).to.equal(false); + }); + it('should dispatch with multiple chords in a key sequence', () => { let count = 0; registry.addCommand('test', { From a449c9cc0b699a870bd7c8b2d6070f6ba5aa3aac Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Wed, 6 Mar 2024 11:24:35 +0000 Subject: [PATCH 2/7] Allow to hold and cancel the execution of a keybinding asynchronously --- packages/commands/src/index.ts | 45 +++++++++++++++++++++-- packages/commands/tests/src/index.spec.ts | 44 ++++++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/packages/commands/src/index.ts b/packages/commands/src/index.ts index ff4722638..e6197b887 100644 --- a/packages/commands/src/index.ts +++ b/packages/commands/src/index.ts @@ -583,6 +583,21 @@ export class CommandRegistry { this._startTimer(); } + /** + * Delay the execution of any command matched against the given 'keydown' event + * until the `permission` to execute is granted. + * + * @param event - The event object for a `'keydown'` event. + * @param permission - The promise with value indicating whether to proceed with the execution. + * + * ### Note + * This enables the caller of `processKeydownEvent` to asynchronously prevent the + * execution of the command based on external events. + */ + holdKeyBindingExecution(event: KeyboardEvent, permission: Promise) { + this._holdKeyBindingPromises.set(event, permission); + } + /** * Start or restart the pending timeout. */ @@ -619,8 +634,31 @@ export class CommandRegistry { * Execute the command for the given key binding. * * If the command is missing or disabled, a warning will be logged. - */ - private _executeKeyBinding(binding: CommandRegistry.IKeyBinding): void { + * + * The execution will not proceed if any of the events leading to + * the keybinding matching were held with the permission resolving to false. + */ + private async _executeKeyBinding( + binding: CommandRegistry.IKeyBinding + ): Promise { + if (this._holdKeyBindingPromises.size !== 0) { + // Wait until all hold requests on execution are lifted. + const executionAllowed = ( + await Promise.all( + this._keydownEvents.map( + async event => + this._holdKeyBindingPromises.get(event) ?? Promise.resolve(true) + ) + ) + ).every(Boolean); + // Clear the hold requests. + this._holdKeyBindingPromises.clear(); + // Do not proceed with the execution if any of the hold requests did not get the permission to proceed. + if (!executionAllowed) { + return; + } + } + let { command, args } = binding; let newArgs: ReadonlyPartialJSONObject = { _luminoEvent: { type: 'keybinding', keys: binding.keys }, @@ -634,7 +672,7 @@ export class CommandRegistry { console.warn(`${msg1} ${msg2}`); return; } - this.execute(command, newArgs); + await this.execute(command, newArgs); } /** @@ -679,6 +717,7 @@ export class CommandRegistry { this, CommandRegistry.IKeyBindingChangedArgs >(this); + private _holdKeyBindingPromises = new Map>(); } /** diff --git a/packages/commands/tests/src/index.spec.ts b/packages/commands/tests/src/index.spec.ts index 4d627722e..87f509914 100644 --- a/packages/commands/tests/src/index.spec.ts +++ b/packages/commands/tests/src/index.spec.ts @@ -1322,6 +1322,50 @@ describe('@lumino/commands', () => { }); }); + describe('.holdKeyBindingExecution()', () => { + it('should proceed with command execution if permission of the event resolves to true', () => { + let called = false; + registry.addCommand('test', { + execute: () => { + called = true; + } + }); + registry.addKeyBinding({ + keys: ['Ctrl ;'], + selector: `#${elem.id}`, + command: 'test' + }); + const event = new KeyboardEvent('keydown', { + keyCode: 59, + ctrlKey: true + }); + registry.holdKeyBindingExecution(event, Promise.resolve(true)); + elem.dispatchEvent(event); + expect(called).to.equal(false); + }); + + it('should prevent command execution if permission of the event resolves to false', () => { + let called = false; + registry.addCommand('test', { + execute: () => { + called = true; + } + }); + registry.addKeyBinding({ + keys: ['Ctrl ;'], + selector: `#${elem.id}`, + command: 'test' + }); + const event = new KeyboardEvent('keydown', { + keyCode: 59, + ctrlKey: true + }); + registry.holdKeyBindingExecution(event, Promise.resolve(false)); + elem.dispatchEvent(event); + expect(called).to.equal(false); + }); + }); + describe('.parseKeystroke()', () => { it('should parse a keystroke into its parts', () => { let parts = CommandRegistry.parseKeystroke('Ctrl Shift Alt S'); From 198250f71c2c3ef39104dc52a2daeac958908db9 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Wed, 6 Mar 2024 11:27:28 +0000 Subject: [PATCH 3/7] Update API --- review/api/commands.api.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/review/api/commands.api.md b/review/api/commands.api.md index 81ed057ee..5b8a9e760 100644 --- a/review/api/commands.api.md +++ b/review/api/commands.api.md @@ -22,6 +22,7 @@ export class CommandRegistry { describedBy(id: string, args?: ReadonlyPartialJSONObject): Promise; execute(id: string, args?: ReadonlyPartialJSONObject): Promise; hasCommand(id: string): boolean; + holdKeyBindingExecution(event: KeyboardEvent, permission: Promise): void; icon(id: string, args?: ReadonlyPartialJSONObject): VirtualElement.IRenderer | undefined; iconClass(id: string, args?: ReadonlyPartialJSONObject): string; iconLabel(id: string, args?: ReadonlyPartialJSONObject): string; @@ -79,6 +80,7 @@ export namespace CommandRegistry { readonly args: ReadonlyPartialJSONObject; readonly command: string; readonly keys: ReadonlyArray; + readonly preventDefault?: boolean; readonly selector: string; } export interface IKeyBindingChangedArgs { @@ -91,6 +93,7 @@ export namespace CommandRegistry { keys: string[]; linuxKeys?: string[]; macKeys?: string[]; + preventDefault?: boolean; selector: string; winKeys?: string[]; } From c5c0a9c7084fd4ecf876053bfc982d6cdd3d76e8 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Mon, 18 Mar 2024 11:05:08 +0000 Subject: [PATCH 4/7] Add a timeout for keybinding hold --- packages/commands/src/index.ts | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/packages/commands/src/index.ts b/packages/commands/src/index.ts index e6197b887..e0f8c4c70 100644 --- a/packages/commands/src/index.ts +++ b/packages/commands/src/index.ts @@ -644,12 +644,17 @@ export class CommandRegistry { if (this._holdKeyBindingPromises.size !== 0) { // Wait until all hold requests on execution are lifted. const executionAllowed = ( - await Promise.all( - this._keydownEvents.map( - async event => - this._holdKeyBindingPromises.get(event) ?? Promise.resolve(true) - ) - ) + await Promise.race([ + Promise.all( + this._keydownEvents.map( + async event => + this._holdKeyBindingPromises.get(event) ?? Promise.resolve(true) + ) + ), + new Promise(resolve => { + setTimeout(() => resolve([false]), Private.KEYBINDING_HOLD_TIMEOUT); + }) + ]) ).every(Boolean); // Clear the hold requests. this._holdKeyBindingPromises.clear(); @@ -1350,6 +1355,11 @@ namespace Private { */ export const CHORD_TIMEOUT = 1000; + /** + * The timeout in ms for stopping the hold on keybinding execution. + */ + export const KEYBINDING_HOLD_TIMEOUT = 1000; + /** * A convenience type alias for a command func. */ From 8a58f16e87a8e2d9e1ef72977b7fb01e89c53d57 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Mon, 18 Mar 2024 12:31:06 +0000 Subject: [PATCH 5/7] Fix tests (prevent should fail now) --- packages/commands/tests/src/index.spec.ts | 32 +++++++++++++++-------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/packages/commands/tests/src/index.spec.ts b/packages/commands/tests/src/index.spec.ts index 87f509914..ef8cff6b3 100644 --- a/packages/commands/tests/src/index.spec.ts +++ b/packages/commands/tests/src/index.spec.ts @@ -1323,12 +1323,23 @@ describe('@lumino/commands', () => { }); describe('.holdKeyBindingExecution()', () => { - it('should proceed with command execution if permission of the event resolves to true', () => { - let called = false; + let calledPromise: Promise; + let execute: () => void; + + beforeEach(() => { + calledPromise = Promise.race([ + new Promise(_resolve => { + execute = () => _resolve(true); + }), + new Promise(resolve => + setTimeout(() => resolve(false), 1000) + ) + ]); + }); + + it('should proceed with command execution if permission of the event resolves to true', async () => { registry.addCommand('test', { - execute: () => { - called = true; - } + execute }); registry.addKeyBinding({ keys: ['Ctrl ;'], @@ -1341,15 +1352,13 @@ describe('@lumino/commands', () => { }); registry.holdKeyBindingExecution(event, Promise.resolve(true)); elem.dispatchEvent(event); - expect(called).to.equal(false); + const called = await calledPromise; + expect(called).to.equal(true); }); - it('should prevent command execution if permission of the event resolves to false', () => { - let called = false; + it('should prevent command execution if permission of the event resolves to false', async () => { registry.addCommand('test', { - execute: () => { - called = true; - } + execute }); registry.addKeyBinding({ keys: ['Ctrl ;'], @@ -1362,6 +1371,7 @@ describe('@lumino/commands', () => { }); registry.holdKeyBindingExecution(event, Promise.resolve(false)); elem.dispatchEvent(event); + const called = await calledPromise; expect(called).to.equal(false); }); }); From 341a8253a8c25c9df35cf3eaac68811c8b8b62a3 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Mon, 18 Mar 2024 12:34:05 +0000 Subject: [PATCH 6/7] Add a test for multiple key presses --- packages/commands/tests/src/index.spec.ts | 25 +++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/packages/commands/tests/src/index.spec.ts b/packages/commands/tests/src/index.spec.ts index ef8cff6b3..68c435402 100644 --- a/packages/commands/tests/src/index.spec.ts +++ b/packages/commands/tests/src/index.spec.ts @@ -1374,6 +1374,31 @@ describe('@lumino/commands', () => { const called = await calledPromise; expect(called).to.equal(false); }); + + it('should prevent command execution if permission for any of the events resolves to false', async () => { + registry.addCommand('test', { + execute + }); + registry.addKeyBinding({ + keys: ['Shift ['], + selector: `#${elem.id}`, + command: 'test' + }); + const shiftEvent = new KeyboardEvent('keydown', { + keyCode: 16, + shiftKey: true + }); + const bracketEvent = new KeyboardEvent('keydown', { + keyCode: 219, + shiftKey: true + }); + registry.holdKeyBindingExecution(shiftEvent, Promise.resolve(true)); + registry.holdKeyBindingExecution(bracketEvent, Promise.resolve(false)); + elem.dispatchEvent(shiftEvent); + elem.dispatchEvent(bracketEvent); + const called = await calledPromise; + expect(called).to.equal(false); + }); }); describe('.parseKeystroke()', () => { From d455799e8c84fd74448dfa282c912f3743a44e30 Mon Sep 17 00:00:00 2001 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: Mon, 18 Mar 2024 12:34:46 +0000 Subject: [PATCH 7/7] Fix both tests by correcting `keydownEvents` store and access sequence --- packages/commands/src/index.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/commands/src/index.ts b/packages/commands/src/index.ts index e0f8c4c70..0dd735761 100644 --- a/packages/commands/src/index.ts +++ b/packages/commands/src/index.ts @@ -559,6 +559,10 @@ export class CommandRegistry { event.stopPropagation(); } + // Store the event for possible playback in the future and for + // the use in execution hold check. + this._keydownEvents.push(event); + // If there is an exact match but no partial match, the exact match // can be dispatched immediately. The pending state is cleared so // the next key press starts from the default state. @@ -575,9 +579,6 @@ export class CommandRegistry { this._exactKeyMatch = exact; } - // Store the event for possible playback in the future. - this._keydownEvents.push(event); - // (Re)start the timer to dispatch the most recent exact match // in case the partial match fails to result in an exact match. this._startTimer(); @@ -642,11 +643,13 @@ export class CommandRegistry { binding: CommandRegistry.IKeyBinding ): Promise { if (this._holdKeyBindingPromises.size !== 0) { + // Copy keydown events list to ensure it is available in async code. + const keydownEvents = [...this._keydownEvents]; // Wait until all hold requests on execution are lifted. const executionAllowed = ( await Promise.race([ Promise.all( - this._keydownEvents.map( + keydownEvents.map( async event => this._holdKeyBindingPromises.get(event) ?? Promise.resolve(true) )