Fix the default options for keyboard shortcuts
Change 339776 has introduced a new option `do not prevent default` for
keyboard shortcuts. The default options object was set to
`{shouldSuppress: true, doNotPrevent: false}`, and the author thought
that passing `{doNotPrevent: false}` would retain the default of
`shouldSuppress`, but of course it does not.
So keyboard shortcuts `j` and `k` were not suppressed properly anymore
when the user types into input elements.
The fix is to avoid a default options object, but instead calculate
defaults individually.
Release-Notes: skip
Change-Id: I7397cbd85f215c3197754b4aea3ef963e1834f5c
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
index 09cebd0..8c17578 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -84,7 +84,8 @@
private comboKeyLastPressed: {key?: ComboKey; timestampMs?: number} = {};
/** Keeps track of the corresponding user preference. */
- private shortcutsDisabled = false;
+ // visible for testing
+ shortcutsDisabled = false;
private readonly keydownListener: (e: KeyboardEvent) => void;
@@ -155,11 +156,10 @@
element: HTMLElement,
shortcut: Binding,
listener: (e: KeyboardEvent) => void,
- options: ShortcutOptions = {
- shouldSuppress: true,
- doNotPrevent: false,
- }
+ options?: ShortcutOptions
) {
+ const optShouldSuppress = options?.shouldSuppress ?? true;
+ const optDoNotPrevent = options?.doNotPrevent ?? false;
const wrappedListener = (e: KeyboardEvent) => {
if (e.repeat && !shortcut.allowRepeat) return;
if (!eventMatchesShortcut(e, shortcut)) return;
@@ -168,13 +168,13 @@
} else {
if (this.isInComboKeyMode()) return;
}
- if (options.shouldSuppress && shouldSuppress(e)) return;
+ if (optShouldSuppress && shouldSuppress(e)) return;
// `shortcutsDisabled` refers to disabling global shortcuts like 'n'. If
// `shouldSuppress` is false (e.g.for Ctrl - ENTER), then don't disable
// the shortcut.
- if (options.shouldSuppress && this.shortcutsDisabled) return;
- if (!options.doNotPrevent) e.preventDefault();
- if (!options.doNotPrevent) e.stopPropagation();
+ if (optShouldSuppress && this.shortcutsDisabled) return;
+ if (!optDoNotPrevent) e.preventDefault();
+ if (!optDoNotPrevent) e.stopPropagation();
this.reportTriggered(e);
listener(e);
};
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
index 69c371e..35da84e 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
@@ -10,9 +10,12 @@
ShortcutsService,
} from '../../services/shortcuts/shortcuts-service';
import {Shortcut, ShortcutSection} from './shortcuts-config';
-import {SinonFakeTimers} from 'sinon';
-import {Key, Modifier} from '../../utils/dom-util';
+import {SinonFakeTimers, SinonSpy} from 'sinon';
+import {Binding, Key, Modifier} from '../../utils/dom-util';
import {getAppContext} from '../app-context';
+import {pressKey} from '../../test/test-utils';
+
+const KEY_A: Binding = {key: 'a'};
suite('shortcuts-service tests', () => {
let service: ShortcutsService;
@@ -34,6 +37,64 @@
);
});
+ suite('addShortcut()', () => {
+ let el: HTMLElement;
+ let listener: SinonSpy<[KeyboardEvent], void>;
+
+ setup(() => {
+ el = document.createElement('div');
+ listener = sinon.spy() as SinonSpy<[KeyboardEvent], void>;
+ });
+
+ test('standard call', () => {
+ service.addShortcut(el, KEY_A, listener);
+ pressKey(el, KEY_A.key);
+ assert.isTrue(listener.calledOnce);
+ });
+
+ test('doNotPrevent option default false', () => {
+ service.addShortcut(el, KEY_A, listener);
+ pressKey(el, KEY_A.key);
+ assert.isTrue(listener.calledOnce);
+ assert.isTrue(listener.lastCall.firstArg?.defaultPrevented);
+ });
+
+ test('doNotPrevent option force false', () => {
+ service.addShortcut(el, KEY_A, listener, {doNotPrevent: false});
+ pressKey(el, KEY_A.key);
+ assert.isTrue(listener.calledOnce);
+ assert.isTrue(listener.lastCall.firstArg?.defaultPrevented);
+ });
+
+ test('doNotPrevent option force true', () => {
+ service.addShortcut(el, KEY_A, listener, {doNotPrevent: true});
+ pressKey(el, KEY_A.key);
+ assert.isTrue(listener.calledOnce);
+ assert.isFalse(listener.lastCall.firstArg?.defaultPrevented);
+ });
+
+ test('shouldSuppress option default true', () => {
+ service.shortcutsDisabled = true;
+ service.addShortcut(el, KEY_A, listener);
+ pressKey(el, KEY_A.key);
+ assert.isTrue(listener.notCalled);
+ });
+
+ test('shouldSuppress option force true', () => {
+ service.shortcutsDisabled = true;
+ service.addShortcut(el, KEY_A, listener, {shouldSuppress: true});
+ pressKey(el, KEY_A.key);
+ assert.isTrue(listener.notCalled);
+ });
+
+ test('shouldSuppress option force false', () => {
+ service.shortcutsDisabled = true;
+ service.addShortcut(el, KEY_A, listener, {shouldSuppress: false});
+ pressKey(el, KEY_A.key);
+ assert.isTrue(listener.calledOnce);
+ });
+ });
+
suite('binding descriptions', () => {
function mapToObject<K, V>(m: Map<K, V>) {
const o: any = {};
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index f7e6672..98c7c14 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -272,6 +272,7 @@
const eventOptions = {
key,
bubbles: true,
+ cancelable: true,
composed: true,
altKey: modifiers.includes(Modifier.ALT_KEY),
ctrlKey: modifiers.includes(Modifier.CTRL_KEY),
