Move most of shortcut mixin features into service
Make the keyboard shortcut mixin (and thus all of Gerrit) independent
from IronA11yKeysBehavior. The major change needed for this was
introducing `addShortcut` to the ShortcutsService as a replacement for
`addOwnKeyBinding`.
The views implementing `keyboardShortcuts()` needed to be changed to
return a proper callback instead of just a string.
We don't use `keyUp` events anymore. Everything is just `keyDown`.
`keyUp` was only introduced because of troubles handling combo keys,
which is now properly dealt with.
Along with `IronA11yKeysBehavior` the `IronKeyboardEvent` is also gone.
All code is just using the standard `KeyboardEvent` instead. Also the
`keyEventTarget` does not have any meaning anymore. The service adds
all listeners to `document.body` by default. If the need arises, then
we can introduce an option later for allowing shortcuts to also be
registered on a more "local" level.
The hundreds of `modifierPressed` could be removed, because of how
`eventMatchesShortcut` is implemented. If the modifiers are not matched
properly, then the shortcut will not be triggered. That also means that
some `shiftKey` checks could be removed.
We are generally suppressing `repeat` keydown events. If processing them
is desired, then we can bring them back as an explicit option. But
ignoring them seems like the best default.
If we are triggering a shortcut callback, then we are always calling
`preventDefault()` and `stopPropagation()` on the keyboard event. That
also seems a great default and allows to remove tons of individual
`preventDefault()` calls. If that turns out to be undesired in specific
scenarios, then we can introduce a dedicated option.
We are also always checking `shouldSuppress()` for every shortcut, so
the tons of individual checks could also be removed.
Some handlers were handling lowercase and uppercase chars in one method,
e.g. 'n' and 'N'. We have split those up in two dedicated handlers.
The combo key handling could be completely moved into the service. Those
special shortcuts are treated as normal shortcuts, but when checking a
shortcut against an event we also check whether the current combo state
matches the shortcut definition. This simplifies matters a lot.
Instead of calling addShortcut() directly for every shortcut, the mixin
just passes all information into `attachHost`, and then the service can
iterate over all the shortcuts and bindings. And thus also be
responsible for the cleanups (i.e. calling removeEventListener).
There are still a lot of potential follow-up cleanups and refactorings
that are staring into your face, but obviously this change is already
fairly large, so let's leave them for later. The major accomplishment
here is that replicating the mixin's functionality in a Lit controller
has become trivial. So the Lit migration of Polymer elements that
depend on the keyboard mixin is unblocked. Also note that the entire
"observer" related code in the mixin is something that we probably want
to get rid of altogether and replace it by something clearer and
simpler, e.g. the service knowing which view is current.
Change-Id: I5cf82feb1c8af16c579eab2120381c85e9607969
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
index bd004d7..3c9e058 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
@@ -16,6 +16,8 @@
*/
/** Enum for all special shortcuts */
+import {ComboKey, Key, Modifier, Binding} from '../../utils/dom-util';
+
export enum SPECIAL_SHORTCUT {
DOC_ONLY = 'DOC_ONLY',
GO_KEY = 'GO_KEY',
@@ -115,7 +117,7 @@
export interface ShortcutHelpItem {
shortcut: Shortcut;
text: string;
- bindings: string[];
+ bindings: Binding[];
}
export const config = new Map<ShortcutSection, ShortcutHelpItem[]>();
@@ -124,8 +126,8 @@
shortcut: Shortcut,
section: ShortcutSection,
text: string,
- binding: string,
- ...moreBindings: string[]
+ binding: Binding,
+ ...moreBindings: Binding[]
) {
if (!config.has(section)) {
config.set(section, []);
@@ -136,417 +138,388 @@
}
}
-describe(Shortcut.SEARCH, ShortcutSection.EVERYWHERE, 'Search', '/');
+describe(Shortcut.SEARCH, ShortcutSection.EVERYWHERE, 'Search', {key: '/'});
describe(
Shortcut.OPEN_SHORTCUT_HELP_DIALOG,
ShortcutSection.EVERYWHERE,
'Show this dialog',
- '?'
+ {key: '?'}
);
describe(
Shortcut.GO_TO_USER_DASHBOARD,
ShortcutSection.EVERYWHERE,
'Go to User Dashboard',
- SPECIAL_SHORTCUT.GO_KEY,
- 'i'
+ {key: 'i', combo: ComboKey.G}
);
describe(
Shortcut.GO_TO_OPENED_CHANGES,
ShortcutSection.EVERYWHERE,
'Go to Opened Changes',
- SPECIAL_SHORTCUT.GO_KEY,
- 'o'
+ {key: 'o', combo: ComboKey.G}
);
describe(
Shortcut.GO_TO_MERGED_CHANGES,
ShortcutSection.EVERYWHERE,
'Go to Merged Changes',
- SPECIAL_SHORTCUT.GO_KEY,
- 'm'
+ {key: 'm', combo: ComboKey.G}
);
describe(
Shortcut.GO_TO_ABANDONED_CHANGES,
ShortcutSection.EVERYWHERE,
'Go to Abandoned Changes',
- SPECIAL_SHORTCUT.GO_KEY,
- 'a'
+ {key: 'a', combo: ComboKey.G}
);
describe(
Shortcut.GO_TO_WATCHED_CHANGES,
ShortcutSection.EVERYWHERE,
'Go to Watched Changes',
- SPECIAL_SHORTCUT.GO_KEY,
- 'w'
+ {key: 'w', combo: ComboKey.G}
);
describe(
Shortcut.CURSOR_NEXT_CHANGE,
ShortcutSection.ACTIONS,
'Select next change',
- 'j'
+ {key: 'j'}
);
describe(
Shortcut.CURSOR_PREV_CHANGE,
ShortcutSection.ACTIONS,
'Select previous change',
- 'k'
+ {key: 'k'}
);
describe(
Shortcut.OPEN_CHANGE,
ShortcutSection.ACTIONS,
'Show selected change',
- 'o'
+ {key: 'o'}
);
describe(
Shortcut.NEXT_PAGE,
ShortcutSection.ACTIONS,
'Go to next page',
- 'n',
- ']'
+ {key: 'n'},
+ {key: ']'}
);
describe(
Shortcut.PREV_PAGE,
ShortcutSection.ACTIONS,
'Go to previous page',
- 'p',
- '['
+ {key: 'p'},
+ {key: '['}
);
describe(
Shortcut.OPEN_REPLY_DIALOG,
ShortcutSection.ACTIONS,
'Open reply dialog to publish comments and add reviewers',
- 'a:keyup'
+ {key: 'a'}
);
describe(
Shortcut.OPEN_DOWNLOAD_DIALOG,
ShortcutSection.ACTIONS,
'Open download overlay',
- 'd:keyup'
+ {key: 'd'}
);
describe(
Shortcut.EXPAND_ALL_MESSAGES,
ShortcutSection.ACTIONS,
'Expand all messages',
- 'x'
+ {key: 'x'}
);
describe(
Shortcut.COLLAPSE_ALL_MESSAGES,
ShortcutSection.ACTIONS,
'Collapse all messages',
- 'z'
+ {key: 'z'}
);
describe(
Shortcut.REFRESH_CHANGE,
ShortcutSection.ACTIONS,
'Reload the change at the latest patch',
- 'shift+r:keyup'
+ {key: 'R'}
);
describe(
Shortcut.TOGGLE_CHANGE_REVIEWED,
ShortcutSection.ACTIONS,
'Mark/unmark change as reviewed',
- 'r:keyup'
+ {key: 'r'}
);
describe(
Shortcut.TOGGLE_FILE_REVIEWED,
ShortcutSection.ACTIONS,
'Toggle review flag on selected file',
- 'r:keyup'
+ {key: 'r'}
);
describe(
Shortcut.REFRESH_CHANGE_LIST,
ShortcutSection.ACTIONS,
'Refresh list of changes',
- 'shift+r:keyup'
+ {key: 'R'}
);
describe(
Shortcut.TOGGLE_CHANGE_STAR,
ShortcutSection.ACTIONS,
'Star/unstar change',
- 's:keydown'
+ {key: 's'}
);
describe(
Shortcut.OPEN_SUBMIT_DIALOG,
ShortcutSection.ACTIONS,
'Open submit dialog',
- 'shift+s'
+ {key: 'S'}
);
describe(
Shortcut.TOGGLE_ATTENTION_SET,
ShortcutSection.ACTIONS,
'Toggle attention set status',
- 'shift+t'
+ {key: 'T'}
);
-describe(
- Shortcut.EDIT_TOPIC,
- ShortcutSection.ACTIONS,
- 'Add a change topic',
- 't'
-);
+describe(Shortcut.EDIT_TOPIC, ShortcutSection.ACTIONS, 'Add a change topic', {
+ key: 't',
+});
describe(
Shortcut.DIFF_AGAINST_BASE,
ShortcutSection.DIFFS,
'Diff against base',
- SPECIAL_SHORTCUT.V_KEY,
- 'down',
- 's'
+ {key: Key.DOWN, combo: ComboKey.V},
+ {key: 's', combo: ComboKey.V}
);
describe(
Shortcut.DIFF_AGAINST_LATEST,
ShortcutSection.DIFFS,
'Diff against latest patchset',
- SPECIAL_SHORTCUT.V_KEY,
- 'up',
- 'w'
+ {key: Key.UP, combo: ComboKey.V},
+ {key: 'w', combo: ComboKey.V}
);
describe(
Shortcut.DIFF_BASE_AGAINST_LEFT,
ShortcutSection.DIFFS,
'Diff base against left',
- SPECIAL_SHORTCUT.V_KEY,
- 'left',
- 'a'
+ {key: Key.LEFT, combo: ComboKey.V},
+ {key: 'a', combo: ComboKey.V}
);
describe(
Shortcut.DIFF_RIGHT_AGAINST_LATEST,
ShortcutSection.DIFFS,
'Diff right against latest',
- SPECIAL_SHORTCUT.V_KEY,
- 'right',
- 'd'
+ {key: Key.RIGHT, combo: ComboKey.V},
+ {key: 'd', combo: ComboKey.V}
);
describe(
Shortcut.DIFF_BASE_AGAINST_LATEST,
ShortcutSection.DIFFS,
'Diff base against latest',
- SPECIAL_SHORTCUT.V_KEY,
- 'b'
+ {key: 'b', combo: ComboKey.V}
);
describe(
Shortcut.NEXT_LINE,
ShortcutSection.DIFFS,
'Go to next line',
- 'j',
- 'down'
+ {key: 'j'},
+ {key: Key.DOWN}
);
describe(
Shortcut.PREV_LINE,
ShortcutSection.DIFFS,
'Go to previous line',
- 'k',
- 'up'
+ {key: 'k'},
+ {key: Key.UP}
);
describe(
Shortcut.VISIBLE_LINE,
ShortcutSection.DIFFS,
'Move cursor to currently visible code',
- '.'
+ {key: '.'}
);
-describe(
- Shortcut.NEXT_CHUNK,
- ShortcutSection.DIFFS,
- 'Go to next diff chunk',
- 'n'
-);
+describe(Shortcut.NEXT_CHUNK, ShortcutSection.DIFFS, 'Go to next diff chunk', {
+ key: 'n',
+});
describe(
Shortcut.PREV_CHUNK,
ShortcutSection.DIFFS,
'Go to previous diff chunk',
- 'p'
+ {key: 'p'}
);
describe(
Shortcut.TOGGLE_ALL_DIFF_CONTEXT,
ShortcutSection.DIFFS,
'Toggle all diff context',
- 'shift+x'
+ {key: 'X'}
);
describe(
Shortcut.NEXT_COMMENT_THREAD,
ShortcutSection.DIFFS,
'Go to next comment thread',
- 'shift+n'
+ {key: 'N'}
);
describe(
Shortcut.PREV_COMMENT_THREAD,
ShortcutSection.DIFFS,
'Go to previous comment thread',
- 'shift+p'
+ {key: 'P'}
);
describe(
Shortcut.EXPAND_ALL_COMMENT_THREADS,
ShortcutSection.DIFFS,
'Expand all comment threads',
- SPECIAL_SHORTCUT.DOC_ONLY,
- 'e'
+ {key: 'e', docOnly: true}
);
describe(
Shortcut.COLLAPSE_ALL_COMMENT_THREADS,
ShortcutSection.DIFFS,
'Collapse all comment threads',
- SPECIAL_SHORTCUT.DOC_ONLY,
- 'shift+e'
+ {key: 'E', docOnly: true}
);
describe(
Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS,
ShortcutSection.DIFFS,
'Hide/Display all comment threads',
- 'h'
+ {key: 'h'}
);
-describe(
- Shortcut.LEFT_PANE,
- ShortcutSection.DIFFS,
- 'Select left pane',
- 'shift+left'
-);
-describe(
- Shortcut.RIGHT_PANE,
- ShortcutSection.DIFFS,
- 'Select right pane',
- 'shift+right'
-);
+describe(Shortcut.LEFT_PANE, ShortcutSection.DIFFS, 'Select left pane', {
+ key: Key.LEFT,
+ modifiers: [Modifier.SHIFT_KEY],
+});
+describe(Shortcut.RIGHT_PANE, ShortcutSection.DIFFS, 'Select right pane', {
+ key: Key.RIGHT,
+ modifiers: [Modifier.SHIFT_KEY],
+});
describe(
Shortcut.TOGGLE_LEFT_PANE,
ShortcutSection.DIFFS,
'Hide/show left diff',
- 'shift+a'
+ {key: 'A'}
);
-describe(Shortcut.NEW_COMMENT, ShortcutSection.DIFFS, 'Draft new comment', 'c');
+describe(Shortcut.NEW_COMMENT, ShortcutSection.DIFFS, 'Draft new comment', {
+ key: 'c',
+});
describe(
Shortcut.SAVE_COMMENT,
ShortcutSection.DIFFS,
'Save comment',
- 'ctrl+enter',
- 'meta+enter',
- 'ctrl+s',
- 'meta+s'
+ {key: Key.ENTER, modifiers: [Modifier.CTRL_KEY]},
+ {key: Key.ENTER, modifiers: [Modifier.META_KEY]},
+ {key: 's', modifiers: [Modifier.CTRL_KEY]},
+ {key: 's', modifiers: [Modifier.META_KEY]}
);
describe(
Shortcut.OPEN_DIFF_PREFS,
ShortcutSection.DIFFS,
'Show diff preferences',
- ','
+ {key: ','}
);
describe(
Shortcut.TOGGLE_DIFF_REVIEWED,
ShortcutSection.DIFFS,
'Mark/unmark file as reviewed',
- 'r:keyup'
+ {key: 'r'}
);
describe(
Shortcut.TOGGLE_DIFF_MODE,
ShortcutSection.DIFFS,
'Toggle unified/side-by-side diff',
- 'm:keyup'
+ {key: 'm'}
);
describe(
Shortcut.NEXT_UNREVIEWED_FILE,
ShortcutSection.DIFFS,
'Mark file as reviewed and go to next unreviewed file',
- 'shift+m'
+ {key: 'M'}
);
-describe(
- Shortcut.TOGGLE_BLAME,
- ShortcutSection.DIFFS,
- 'Toggle blame',
- 'b:keyup'
-);
-describe(Shortcut.OPEN_FILE_LIST, ShortcutSection.DIFFS, 'Open file list', 'f');
-describe(
- Shortcut.NEXT_FILE,
- ShortcutSection.NAVIGATION,
- 'Go to next file',
- ']'
-);
+describe(Shortcut.TOGGLE_BLAME, ShortcutSection.DIFFS, 'Toggle blame', {
+ key: 'b',
+});
+describe(Shortcut.OPEN_FILE_LIST, ShortcutSection.DIFFS, 'Open file list', {
+ key: 'f',
+});
+describe(Shortcut.NEXT_FILE, ShortcutSection.NAVIGATION, 'Go to next file', {
+ key: ']',
+});
describe(
Shortcut.PREV_FILE,
ShortcutSection.NAVIGATION,
'Go to previous file',
- '['
+ {key: '['}
);
describe(
Shortcut.NEXT_FILE_WITH_COMMENTS,
ShortcutSection.NAVIGATION,
'Go to next file that has comments',
- 'shift+j'
+ {key: 'J'}
);
describe(
Shortcut.PREV_FILE_WITH_COMMENTS,
ShortcutSection.NAVIGATION,
'Go to previous file that has comments',
- 'shift+k'
+ {key: 'K'}
);
describe(
Shortcut.OPEN_FIRST_FILE,
ShortcutSection.NAVIGATION,
'Go to first file',
- ']'
+ {key: ']'}
);
describe(
Shortcut.OPEN_LAST_FILE,
ShortcutSection.NAVIGATION,
'Go to last file',
- '['
+ {key: '['}
);
describe(
Shortcut.UP_TO_DASHBOARD,
ShortcutSection.NAVIGATION,
'Up to dashboard',
- 'u'
+ {key: 'u'}
);
-describe(
- Shortcut.UP_TO_CHANGE,
- ShortcutSection.NAVIGATION,
- 'Up to change',
- 'u'
-);
+describe(Shortcut.UP_TO_CHANGE, ShortcutSection.NAVIGATION, 'Up to change', {
+ key: 'u',
+});
describe(
Shortcut.CURSOR_NEXT_FILE,
ShortcutSection.FILE_LIST,
'Select next file',
- 'j',
- 'down'
+ {key: 'j'},
+ {key: Key.DOWN}
);
describe(
Shortcut.CURSOR_PREV_FILE,
ShortcutSection.FILE_LIST,
'Select previous file',
- 'k',
- 'up'
+ {key: 'k'},
+ {key: Key.UP}
);
describe(
Shortcut.OPEN_FILE,
ShortcutSection.FILE_LIST,
'Go to selected file',
- 'o',
- 'enter'
+ {key: 'o'},
+ {key: Key.ENTER}
);
describe(
Shortcut.TOGGLE_ALL_INLINE_DIFFS,
ShortcutSection.FILE_LIST,
'Show/hide all inline diffs',
- 'shift+i'
+ {key: 'I'}
);
describe(
Shortcut.TOGGLE_INLINE_DIFF,
ShortcutSection.FILE_LIST,
'Show/hide selected inline diff',
- 'i'
+ {key: 'i'}
);
describe(
Shortcut.SEND_REPLY,
ShortcutSection.REPLY_DIALOG,
'Send reply',
- SPECIAL_SHORTCUT.DOC_ONLY,
- 'ctrl+enter',
- 'meta+enter'
+ {key: Key.ENTER, modifiers: [Modifier.CTRL_KEY], docOnly: true},
+ {key: Key.ENTER, modifiers: [Modifier.META_KEY], docOnly: true}
);
describe(
Shortcut.EMOJI_DROPDOWN,
ShortcutSection.REPLY_DIALOG,
'Emoji dropdown',
- SPECIAL_SHORTCUT.DOC_ONLY,
- ':'
+ {key: ':', docOnly: true}
);
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
index edc31a4..19f12c5 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -19,27 +19,39 @@
Shortcut,
ShortcutHelpItem,
ShortcutSection,
- SPECIAL_SHORTCUT,
} from './shortcuts-config';
import {disableShortcuts$} from '../user/user-model';
-import {IronKeyboardEvent, isIronKeyboardEvent} from '../../types/events';
-import {isElementTarget, isModifierPressed} from '../../utils/dom-util';
+import {
+ ComboKey,
+ eventMatchesShortcut,
+ isElementTarget,
+ Key,
+ Modifier,
+ Binding,
+} from '../../utils/dom-util';
import {ReportingService} from '../gr-reporting/gr-reporting';
export type SectionView = Array<{binding: string[][]; text: string}>;
+export interface ShortcutListener {
+ shortcut: Shortcut;
+ listener: (e: KeyboardEvent) => void;
+}
+
+export function listen(
+ shortcut: Shortcut,
+ listener: (e: KeyboardEvent) => void
+): ShortcutListener {
+ return {shortcut, listener};
+}
+
/**
* The interface for listener for shortcut events.
*/
-export type ShortcutListener = (
+export type ShortcutViewListener = (
viewMap?: Map<ShortcutSection, SectionView>
) => void;
-export enum ComboKey {
- G = 'g',
- V = 'v',
-}
-
function isComboKey(key: string): key is ComboKey {
return Object.values(ComboKey).includes(key as ComboKey);
}
@@ -55,12 +67,18 @@
* show a shortcut help dialog that only shows the shortcuts that are
* currently relevant.
*/
- private readonly activeHosts = new Map<unknown, Map<string, string>>();
+ private readonly activeShortcuts = new Map<HTMLElement, Shortcut[]>();
+
+ /**
+ * Keeps track of cleanup callbacks (which remove keyboard listeners) that
+ * have to be invoked when a component unregisters itself.
+ */
+ private readonly cleanupsPerHost = new Map<HTMLElement, (() => void)[]>();
/** Static map built in the constructor by iterating over the config. */
- private readonly bindings = new Map<Shortcut, string[]>();
+ private readonly bindings = new Map<Shortcut, Binding[]>();
- private readonly listeners = new Set<ShortcutListener>();
+ private readonly listeners = new Set<ShortcutViewListener>();
/**
* Stores the timestamp of the last combo key being pressed.
@@ -89,7 +107,7 @@
}
public _testOnly_isEmpty() {
- return this.activeHosts.size === 0 && this.listeners.size === 0;
+ return this.activeShortcuts.size === 0 && this.listeners.size === 0;
}
isInComboKeyMode() {
@@ -107,13 +125,35 @@
);
}
- modifierPressed(e: IronKeyboardEvent) {
- return isModifierPressed(e) || this.isInComboKeyMode();
+ /**
+ * TODO(brohlfs): Reconcile with the addShortcut() function in dom-util.
+ * Most likely we will just keep this one here, but that is something for a
+ * follow-up change.
+ */
+ addShortcut(
+ element: HTMLElement,
+ shortcut: Binding,
+ listener: (e: KeyboardEvent) => void
+ ) {
+ const wrappedListener = (e: KeyboardEvent) => {
+ if (e.repeat) return;
+ if (!eventMatchesShortcut(e, shortcut)) return;
+ if (shortcut.combo) {
+ if (!this.isInSpecificComboKeyMode(shortcut.combo)) return;
+ } else {
+ if (this.isInComboKeyMode()) return;
+ }
+ if (this.shouldSuppress(e)) return;
+ e.preventDefault();
+ e.stopPropagation();
+ listener(e);
+ };
+ element.addEventListener('keydown', wrappedListener);
+ return () => element.removeEventListener('keydown', wrappedListener);
}
- shouldSuppress(event: IronKeyboardEvent | KeyboardEvent) {
+ shouldSuppress(e: KeyboardEvent) {
if (this.shortcutsDisabled) return true;
- const e = isIronKeyboardEvent(event) ? event.detail.keyboardEvent : event;
// Note that when you listen on document, then `e.currentTarget` will be the
// document and `e.target` will be `<gr-app>` due to shadow dom, but by
@@ -168,23 +208,37 @@
return this.bindings.get(shortcut);
}
- attachHost(host: unknown, shortcuts: Map<string, string>) {
- this.activeHosts.set(host, shortcuts);
- this.notifyListeners();
+ attachHost(host: HTMLElement, shortcuts: ShortcutListener[]) {
+ this.activeShortcuts.set(
+ host,
+ shortcuts.map(s => s.shortcut)
+ );
+ const cleanups: (() => void)[] = [];
+ for (const s of shortcuts) {
+ const bindings = this.getBindingsForShortcut(s.shortcut);
+ for (const binding of bindings ?? []) {
+ if (binding.docOnly) continue;
+ cleanups.push(this.addShortcut(document.body, binding, s.listener));
+ }
+ }
+ this.cleanupsPerHost.set(host, cleanups);
+ this.notifyViewListeners();
}
- detachHost(host: unknown) {
- if (!this.activeHosts.delete(host)) return false;
- this.notifyListeners();
+ detachHost(host: HTMLElement) {
+ this.activeShortcuts.delete(host);
+ const cleanups = this.cleanupsPerHost.get(host);
+ for (const cleanup of cleanups ?? []) cleanup();
+ this.notifyViewListeners();
return true;
}
- addListener(listener: ShortcutListener) {
+ addListener(listener: ShortcutViewListener) {
this.listeners.add(listener);
listener(this.directoryView());
}
- removeListener(listener: ShortcutListener) {
+ removeListener(listener: ShortcutViewListener) {
return this.listeners.delete(listener);
}
@@ -199,15 +253,17 @@
const bindings = this.bindings.get(shortcutName);
if (!bindings) return '';
return bindings
- .map(binding => this.describeBinding(binding).join('+'))
+ .map(binding => describeBinding(binding).join('+'))
.join(',');
}
activeShortcutsBySection() {
- const activeShortcuts = new Set<string>();
- this.activeHosts.forEach(shortcuts => {
- shortcuts.forEach((_, shortcut) => activeShortcuts.add(shortcut));
- });
+ const activeShortcuts = new Set<Shortcut>();
+ for (const shortcuts of this.activeShortcuts.values()) {
+ for (const shortcut of shortcuts) {
+ activeShortcuts.add(shortcut);
+ }
+ }
const activeShortcutsBySection = new Map<
ShortcutSection,
@@ -219,8 +275,6 @@
if (!activeShortcutsBySection.has(section)) {
activeShortcutsBySection.set(section, []);
}
- // From previous condition, the `get(section)`
- // should always return a valid result
activeShortcutsBySection.get(section)!.push(shortcutHelp);
}
});
@@ -282,63 +336,53 @@
describeBindings(shortcut: Shortcut): string[][] | null {
const bindings = this.bindings.get(shortcut);
- if (!bindings) {
- return null;
- }
- if (bindings[0] === SPECIAL_SHORTCUT.GO_KEY) {
- return bindings
- .slice(1)
- .map(binding => this._describeKey(binding))
- .map(binding => ['g'].concat(binding));
- }
- if (bindings[0] === SPECIAL_SHORTCUT.V_KEY) {
- return bindings
- .slice(1)
- .map(binding => this._describeKey(binding))
- .map(binding => ['v'].concat(binding));
- }
-
+ if (!bindings) return null;
return bindings
- .filter(binding => binding !== SPECIAL_SHORTCUT.DOC_ONLY)
- .map(binding => this.describeBinding(binding));
+ .filter(binding => !binding.docOnly)
+ .map(binding => describeBinding(binding));
}
- _describeKey(key: string) {
- switch (key) {
- case 'shift':
- return 'Shift';
- case 'meta':
- return 'Meta';
- case 'ctrl':
- return 'Ctrl';
- case 'enter':
- return 'Enter';
- case 'up':
- return '\u2191'; // ↑
- case 'down':
- return '\u2193'; // ↓
- case 'left':
- return '\u2190'; // ←
- case 'right':
- return '\u2192'; // →
- default:
- return key;
- }
- }
-
- describeBinding(binding: string) {
- // single key bindings
- if (binding.length === 1) {
- return [binding];
- }
- return binding
- .split(':')[0]
- .split('+')
- .map(part => this._describeKey(part));
- }
-
- notifyListeners() {
+ notifyViewListeners() {
const view = this.directoryView();
this.listeners.forEach(listener => listener(view));
}
}
+
+function describeKey(key: string | Key) {
+ switch (key) {
+ case Key.UP:
+ return '\u2191'; // ↑
+ case Key.DOWN:
+ return '\u2193'; // ↓
+ case Key.LEFT:
+ return '\u2190'; // ←
+ case Key.RIGHT:
+ return '\u2192'; // →
+ default:
+ return key;
+ }
+}
+
+export function describeBinding(binding: Binding): string[] {
+ const description: string[] = [];
+ if (binding.combo === ComboKey.G) {
+ description.push('g');
+ }
+ if (binding.combo === ComboKey.V) {
+ description.push('v');
+ }
+ if (binding.modifiers?.includes(Modifier.SHIFT_KEY)) {
+ description.push('Shift');
+ }
+ if (binding.modifiers?.includes(Modifier.ALT_KEY)) {
+ description.push('Alt');
+ }
+ if (binding.modifiers?.includes(Modifier.CTRL_KEY)) {
+ description.push('Ctrl');
+ }
+ if (binding.modifiers?.includes(Modifier.META_KEY)) {
+ description.push('Meta/Cmd');
+ }
+ description.push(describeKey(binding.key));
+ return description;
+}
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
index d8aa11e..05c4f53 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
@@ -16,12 +16,14 @@
*/
import '../../test/common-test-setup-karma';
import {
- ShortcutsService,
COMBO_TIMEOUT_MS,
+ describeBinding,
+ ShortcutsService,
} from '../../services/shortcuts/shortcuts-service';
import {Shortcut, ShortcutSection} from './shortcuts-config';
import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions';
import {SinonFakeTimers} from 'sinon';
+import {Key, Modifier} from '../../utils/dom-util';
async function keyEventOn(
el: HTMLElement,
@@ -96,13 +98,12 @@
});
test('getShortcut', () => {
- const NEXT_FILE = Shortcut.NEXT_FILE;
- assert.equal(service.getShortcut(NEXT_FILE), ']');
- });
-
- test('getShortcut with modifiers', () => {
- const NEXT_FILE = Shortcut.TOGGLE_LEFT_PANE;
- assert.equal(service.getShortcut(NEXT_FILE), 'Shift+a');
+ assert.equal(service.getShortcut(Shortcut.NEXT_FILE), ']');
+ assert.equal(service.getShortcut(Shortcut.TOGGLE_LEFT_PANE), 'A');
+ assert.equal(
+ service.getShortcut(Shortcut.SEND_REPLY),
+ 'Ctrl+Enter,Meta/Cmd+Enter'
+ );
});
suite('binding descriptions', () => {
@@ -113,14 +114,18 @@
}
test('single combo description', () => {
- assert.deepEqual(service.describeBinding('a'), ['a']);
- assert.deepEqual(service.describeBinding('a:keyup'), ['a']);
- assert.deepEqual(service.describeBinding('ctrl+a'), ['Ctrl', 'a']);
- assert.deepEqual(service.describeBinding('ctrl+shift+up:keyup'), [
- 'Ctrl',
- 'Shift',
- '↑',
- ]);
+ assert.deepEqual(describeBinding({key: 'a'}), ['a']);
+ assert.deepEqual(
+ describeBinding({key: 'a', modifiers: [Modifier.CTRL_KEY]}),
+ ['Ctrl', 'a']
+ );
+ assert.deepEqual(
+ describeBinding({
+ key: Key.UP,
+ modifiers: [Modifier.CTRL_KEY, Modifier.SHIFT_KEY],
+ }),
+ ['Shift', 'Ctrl', '↑']
+ );
});
test('combo set description', () => {
@@ -130,9 +135,9 @@
);
assert.deepEqual(service.describeBindings(Shortcut.SAVE_COMMENT), [
['Ctrl', 'Enter'],
- ['Meta', 'Enter'],
+ ['Meta/Cmd', 'Enter'],
['Ctrl', 's'],
- ['Meta', 's'],
+ ['Meta/Cmd', 's'],
]);
assert.deepEqual(service.describeBindings(Shortcut.PREV_FILE), [['[']]);
});
@@ -187,67 +192,68 @@
test('active shortcuts by section', () => {
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {});
- service.attachHost({}, new Map([[Shortcut.NEXT_FILE, 'null']]));
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.NEXT_FILE, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
- service.attachHost({}, new Map([[Shortcut.NEXT_LINE, 'null']]));
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.NEXT_LINE, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.DIFFS]: [
{
shortcut: Shortcut.NEXT_LINE,
text: 'Go to next line',
- bindings: ['j', 'down'],
+ bindings: [{key: 'j'}, {key: 'ArrowDown'}],
},
],
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
- service.attachHost(
- {},
- new Map([
- [Shortcut.SEARCH, 'null'],
- [Shortcut.GO_TO_OPENED_CHANGES, 'null'],
- ])
- );
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.SEARCH, listener: _ => {}},
+ {shortcut: Shortcut.GO_TO_OPENED_CHANGES, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.DIFFS]: [
{
shortcut: Shortcut.NEXT_LINE,
text: 'Go to next line',
- bindings: ['j', 'down'],
+ bindings: [{key: 'j'}, {key: 'ArrowDown'}],
},
],
[ShortcutSection.EVERYWHERE]: [
{
shortcut: Shortcut.SEARCH,
text: 'Search',
- bindings: ['/'],
+ bindings: [{key: '/'}],
},
{
shortcut: Shortcut.GO_TO_OPENED_CHANGES,
text: 'Go to Opened Changes',
- bindings: ['GO_KEY', 'o'],
+ bindings: [{key: 'o', combo: 'g'}],
},
],
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
@@ -256,33 +262,31 @@
test('directory view', () => {
assert.deepEqual(mapToObject(service.directoryView()), {});
- service.attachHost(
- {},
- new Map([
- [Shortcut.GO_TO_OPENED_CHANGES, 'null'],
- [Shortcut.NEXT_FILE, 'null'],
- [Shortcut.NEXT_LINE, 'null'],
- [Shortcut.SAVE_COMMENT, 'null'],
- [Shortcut.SEARCH, 'null'],
- ])
- );
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.GO_TO_OPENED_CHANGES, listener: _ => {}},
+ {shortcut: Shortcut.NEXT_FILE, listener: _ => {}},
+ {shortcut: Shortcut.NEXT_LINE, listener: _ => {}},
+ {shortcut: Shortcut.SAVE_COMMENT, listener: _ => {}},
+ {shortcut: Shortcut.SEARCH, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.directoryView()), {
[ShortcutSection.DIFFS]: [
{binding: [['j'], ['↓']], text: 'Go to next line'},
{
- binding: [
- ['Ctrl', 'Enter'],
- ['Meta', 'Enter'],
- ],
+ binding: [['Ctrl', 'Enter']],
text: 'Save comment',
},
{
binding: [
+ ['Meta/Cmd', 'Enter'],
['Ctrl', 's'],
- ['Meta', 's'],
],
text: 'Save comment',
},
+ {
+ binding: [['Meta/Cmd', 's']],
+ text: 'Save comment',
+ },
],
[ShortcutSection.EVERYWHERE]: [
{binding: [['/']], text: 'Search'},
