summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorBen Rohlfs <brohlfs@google.com>2021-10-19 23:44:02 +0200
committerBen Rohlfs <brohlfs@google.com>2021-10-21 16:28:13 +0200
commit410f5224801d84425ef0270df0ca0055dd78bbeb (patch)
tree4cb467b35f7861d4b2d6d92d6522bb3860ec7d4e
parent771d18a0162fd37f3fae010eeb7a569e57cfb271 (diff)
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
-rw-r--r--polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts101
-rw-r--r--polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js129
-rw-r--r--polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts200
-rw-r--r--polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts38
-rw-r--r--polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts259
-rw-r--r--polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js112
-rw-r--r--polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts4
-rw-r--r--polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts25
-rw-r--r--polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts339
-rw-r--r--polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js43
-rw-r--r--polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts3
-rw-r--r--polygerrit-ui/app/elements/gr-app-element.ts33
-rw-r--r--polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts24
-rw-r--r--polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts1
-rw-r--r--polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts136
-rw-r--r--polygerrit-ui/app/package.json1
-rw-r--r--polygerrit-ui/app/services/shortcuts/shortcuts-config.ts241
-rw-r--r--polygerrit-ui/app/services/shortcuts/shortcuts-service.ts210
-rw-r--r--polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts102
-rw-r--r--polygerrit-ui/app/types/events.ts27
-rw-r--r--polygerrit-ui/app/utils/dom-util.ts31
-rw-r--r--polygerrit-ui/app/yarn.lock2
22 files changed, 771 insertions, 1290 deletions
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
index a3db699c6b..a79dd8ea0d 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
@@ -27,6 +27,7 @@ import {appContext} from '../../../services/app-context';
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {
GerritNav,
@@ -46,10 +47,9 @@ import {
PreferencesInput,
} from '../../../types/common';
import {hasAttention} from '../../../utils/attention-set-util';
-import {IronKeyboardEvent} from '../../../types/events';
import {fireEvent, fireReload} from '../../../utils/event-util';
-import {isShiftPressed, modifierPressed} from '../../../utils/dom-util';
import {ScrollMode} from '../../../constants/constants';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
const NUMBER_FIXED_COLUMNS = 3;
const CLOSED_STATUS = ['MERGED', 'ABANDONED'];
@@ -135,9 +135,6 @@ export class GrChangeList extends base {
@property({type: Boolean})
showReviewedState = false;
- @property({type: Object})
- keyEventTarget: HTMLElement = document.body;
-
@property({type: Array})
changeTableColumns?: string[];
@@ -157,19 +154,19 @@ export class GrChangeList extends base {
private readonly restApiService = appContext.restApiService;
- private readonly shortcuts = appContext.shortcutsService;
-
- override keyboardShortcuts() {
- return {
- [Shortcut.CURSOR_NEXT_CHANGE]: '_nextChange',
- [Shortcut.CURSOR_PREV_CHANGE]: '_prevChange',
- [Shortcut.NEXT_PAGE]: '_nextPage',
- [Shortcut.PREV_PAGE]: '_prevPage',
- [Shortcut.OPEN_CHANGE]: '_openChange',
- [Shortcut.TOGGLE_CHANGE_REVIEWED]: '_toggleChangeReviewed',
- [Shortcut.TOGGLE_CHANGE_STAR]: '_toggleChangeStar',
- [Shortcut.REFRESH_CHANGE_LIST]: '_refreshChangeList',
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.CURSOR_NEXT_CHANGE, _ => this._nextChange()),
+ listen(Shortcut.CURSOR_PREV_CHANGE, _ => this._prevChange()),
+ listen(Shortcut.NEXT_PAGE, _ => this._nextPage()),
+ listen(Shortcut.PREV_PAGE, _ => this._prevPage()),
+ listen(Shortcut.OPEN_CHANGE, _ => this.openChange()),
+ listen(Shortcut.TOGGLE_CHANGE_REVIEWED, _ =>
+ this._toggleChangeReviewed()
+ ),
+ listen(Shortcut.TOGGLE_CHANGE_STAR, _ => this._toggleChangeStar()),
+ listen(Shortcut.REFRESH_CHANGE_LIST, _ => this._refreshChangeList()),
+ ];
}
private cursor = new GrCursorManager();
@@ -204,7 +201,7 @@ export class GrChangeList extends base {
}
/**
- * Iron-a11y-keys-behavior catches keyboard events globally. Some keyboard
+ * shortcut-service catches keyboard events globally. Some keyboard
* events must be scoped to a component level (e.g. `enter`) in order to not
* override native browser functionality.
*
@@ -213,7 +210,7 @@ export class GrChangeList extends base {
_scopedKeydownHandler(e: KeyboardEvent) {
if (e.keyCode === 13) {
// Enter.
- this.openChange(e);
+ this.openChange();
}
}
@@ -406,63 +403,30 @@ export class GrChangeList extends base {
);
}
- _nextChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _nextChange() {
this.isCursorMoving = true;
this.cursor.next();
this.isCursorMoving = false;
this.selectedIndex = this.cursor.index;
}
- _prevChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _prevChange() {
this.isCursorMoving = true;
this.cursor.previous();
this.isCursorMoving = false;
this.selectedIndex = this.cursor.index;
}
- _openChange(e: IronKeyboardEvent) {
- if (this.shortcuts.modifierPressed(e)) return;
- this.openChange(e.detail.keyboardEvent);
- }
-
- openChange(e: KeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || modifierPressed(e)) return;
- e.preventDefault();
+ openChange() {
const change = this._changeForIndex(this.selectedIndex);
if (change) GerritNav.navigateToChange(change);
}
- _nextPage(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !isShiftPressed(e))
- ) {
- return;
- }
-
- e.preventDefault();
+ _nextPage() {
fireEvent(this, 'next-page');
}
- _prevPage(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !isShiftPressed(e))
- ) {
- return;
- }
-
- e.preventDefault();
+ _prevPage() {
this.dispatchEvent(
new CustomEvent('previous-page', {
composed: true,
@@ -471,12 +435,7 @@ export class GrChangeList extends base {
);
}
- _toggleChangeReviewed(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _toggleChangeReviewed() {
this._toggleReviewedForIndex(this.selectedIndex);
}
@@ -490,21 +449,11 @@ export class GrChangeList extends base {
changeEl.toggleReviewed();
}
- _refreshChangeList(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
-
- e.preventDefault();
+ _refreshChangeList() {
fireReload(this);
}
- _toggleChangeStar(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _toggleChangeStar() {
this._toggleStarForIndex(this.selectedIndex);
}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js
index 49563808eb..de1a0a245b 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.js
@@ -147,38 +147,37 @@ suite('gr-change-list basic tests', () => {
await flush();
const promise = mockPromise();
afterNextRender(element, () => {
- const elementItems = element.root.querySelectorAll(
- 'gr-change-list-item');
- assert.equal(elementItems.length, 3);
-
- assert.isTrue(elementItems[0].hasAttribute('selected'));
- MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
- assert.equal(element.selectedIndex, 1);
- assert.isTrue(elementItems[1].hasAttribute('selected'));
- MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
- assert.equal(element.selectedIndex, 2);
- assert.isTrue(elementItems[2].hasAttribute('selected'));
-
- const navStub = sinon.stub(GerritNav, 'navigateToChange');
- assert.equal(element.selectedIndex, 2);
- MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
- assert.deepEqual(navStub.lastCall.args[0], {_number: 2},
- 'Should navigate to /c/2/');
-
- MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
- assert.equal(element.selectedIndex, 1);
- MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
- assert.deepEqual(navStub.lastCall.args[0], {_number: 1},
- 'Should navigate to /c/1/');
-
- MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
- MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
- MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
- assert.equal(element.selectedIndex, 0);
-
promise.resolve();
});
await promise;
+ const elementItems = element.root.querySelectorAll(
+ 'gr-change-list-item');
+ assert.equal(elementItems.length, 3);
+
+ assert.isTrue(elementItems[0].hasAttribute('selected'));
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ assert.equal(element.selectedIndex, 1);
+ assert.isTrue(elementItems[1].hasAttribute('selected'));
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ assert.equal(element.selectedIndex, 2);
+ assert.isTrue(elementItems[2].hasAttribute('selected'));
+
+ const navStub = sinon.stub(GerritNav, 'navigateToChange');
+ assert.equal(element.selectedIndex, 2);
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 2},
+ 'Should navigate to /c/2/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ assert.equal(element.selectedIndex, 1);
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 1},
+ 'Should navigate to /c/1/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ assert.equal(element.selectedIndex, 0);
});
test('no changes', () => {
@@ -449,45 +448,45 @@ suite('gr-change-list basic tests', () => {
await flush();
const promise = mockPromise();
afterNextRender(element, () => {
- const elementItems = element.root.querySelectorAll(
- 'gr-change-list-item');
- assert.equal(elementItems.length, 9);
-
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
- assert.equal(element.selectedIndex, 1);
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
-
- const navStub = sinon.stub(GerritNav, 'navigateToChange');
- assert.equal(element.selectedIndex, 2);
-
- MockInteractions.pressAndReleaseKeyOn(element, 13); // 'enter'
- assert.deepEqual(navStub.lastCall.args[0], {_number: 2},
- 'Should navigate to /c/2/');
-
- MockInteractions.pressAndReleaseKeyOn(element, 75); // 'k'
- assert.equal(element.selectedIndex, 1);
- MockInteractions.pressAndReleaseKeyOn(element, 13); // 'enter'
- assert.deepEqual(navStub.lastCall.args[0], {_number: 1},
- 'Should navigate to /c/1/');
-
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
- assert.equal(element.selectedIndex, 4);
- MockInteractions.pressAndReleaseKeyOn(element, 13); // 'enter'
- assert.deepEqual(navStub.lastCall.args[0], {_number: 4},
- 'Should navigate to /c/4/');
-
- MockInteractions.keyUpOn(element, 82); // 'r'
- const change = element._changeForIndex(element.selectedIndex);
- assert.equal(change.reviewed, true,
- 'Should mark change as reviewed');
- MockInteractions.keyUpOn(element, 82); // 'r'
- assert.equal(change.reviewed, false,
- 'Should mark change as unreviewed');
promise.resolve();
});
await promise;
+ const elementItems = element.root.querySelectorAll(
+ 'gr-change-list-item');
+ assert.equal(elementItems.length, 9);
+
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ assert.equal(element.selectedIndex, 1);
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+
+ const navStub = sinon.stub(GerritNav, 'navigateToChange');
+ assert.equal(element.selectedIndex, 2);
+
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'Enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 2},
+ 'Should navigate to /c/2/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
+ assert.equal(element.selectedIndex, 1);
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'Enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 1},
+ 'Should navigate to /c/1/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
+ assert.equal(element.selectedIndex, 4);
+ MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'Enter');
+ assert.deepEqual(navStub.lastCall.args[0], {_number: 4},
+ 'Should navigate to /c/4/');
+
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
+ const change = element._changeForIndex(element.selectedIndex);
+ assert.equal(change.reviewed, true,
+ 'Should mark change as reviewed');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
+ assert.equal(change.reviewed, false,
+ 'Should mark change as unreviewed');
});
test('_computeItemHighlight gives false for null account', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index c19be58ec3..b635d3f6da 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -48,11 +48,12 @@ import {htmlTemplate} from './gr-change-view_html';
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
ShortcutSection,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {GrEditConstants} from '../../edit/gr-edit-constants';
import {pluralize} from '../../../utils/string-util';
-import {windowLocationReload, querySelectorAll} from '../../../utils/dom-util';
+import {querySelectorAll, windowLocationReload} from '../../../utils/dom-util';
import {
GeneratedWebLink,
GerritNav,
@@ -62,8 +63,8 @@ import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader
import {RevisionInfo as RevisionInfoClass} from '../../shared/revision-info/revision-info';
import {DiffViewMode} from '../../../api/diff';
import {
- DefaultBase,
ChangeStatus,
+ DefaultBase,
PrimaryTab,
SecondaryTab,
} from '../../../constants/constants';
@@ -83,9 +84,9 @@ import {
changeIsOpen,
changeStatuses,
isCc,
+ isInvolved,
isOwner,
isReviewer,
- isInvolved,
} from '../../../utils/change-util';
import {EventType as PluginEventType} from '../../../api/plugin';
import {customElement, observe, property} from '@polymer/decorators';
@@ -158,9 +159,7 @@ import {
ParsedChangeInfo,
} from '../../../types/types';
import {
- IronKeyboardEventListener,
CloseFixPreviewEvent,
- IronKeyboardEvent,
EditableContentSaveEvent,
EventType,
OpenFixPreviewEvent,
@@ -192,10 +191,11 @@ import {
drafts$,
} from '../../../services/comments/comments-model';
import {
- hasAttention,
getAddedByReason,
getRemovedByReason,
+ hasAttention,
} from '../../../utils/attention-set-util';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
const MIN_LINES_FOR_COMMIT_COLLAPSE = 18;
@@ -295,9 +295,6 @@ export class GrChangeView extends base {
@property({type: Boolean})
hasParent?: boolean;
- @property({type: Object})
- keyEventTarget = document.body;
-
@property({type: Boolean})
disableEdit = false;
@@ -529,7 +526,7 @@ export class GrChangeView extends base {
@property({type: Boolean})
_showRobotCommentsButton = false;
- _throttledToggleChangeStar?: IronKeyboardEventListener;
+ _throttledToggleChangeStar?: (e: KeyboardEvent) => void;
@property({type: Boolean})
_showChecksTab = false;
@@ -560,28 +557,50 @@ export class GrChangeView extends base {
private replyDialogResizeObserver?: ResizeObserver;
- override keyboardShortcuts() {
- return {
- [Shortcut.SEND_REPLY]: null, // DOC_ONLY binding
- [Shortcut.EMOJI_DROPDOWN]: null, // DOC_ONLY binding
- [Shortcut.REFRESH_CHANGE]: '_handleRefreshChange',
- [Shortcut.OPEN_REPLY_DIALOG]: '_handleOpenReplyDialog',
- [Shortcut.OPEN_DOWNLOAD_DIALOG]: '_handleOpenDownloadDialogShortcut',
- [Shortcut.TOGGLE_DIFF_MODE]: '_handleToggleDiffMode',
- [Shortcut.TOGGLE_CHANGE_STAR]: '_throttledToggleChangeStar',
- [Shortcut.UP_TO_DASHBOARD]: '_handleUpToDashboard',
- [Shortcut.EXPAND_ALL_MESSAGES]: '_handleExpandAllMessages',
- [Shortcut.COLLAPSE_ALL_MESSAGES]: '_handleCollapseAllMessages',
- [Shortcut.OPEN_DIFF_PREFS]: '_handleOpenDiffPrefsShortcut',
- [Shortcut.EDIT_TOPIC]: '_handleEditTopic',
- [Shortcut.DIFF_AGAINST_BASE]: '_handleDiffAgainstBase',
- [Shortcut.DIFF_AGAINST_LATEST]: '_handleDiffAgainstLatest',
- [Shortcut.DIFF_BASE_AGAINST_LEFT]: '_handleDiffBaseAgainstLeft',
- [Shortcut.DIFF_RIGHT_AGAINST_LATEST]: '_handleDiffRightAgainstLatest',
- [Shortcut.DIFF_BASE_AGAINST_LATEST]: '_handleDiffBaseAgainstLatest',
- [Shortcut.OPEN_SUBMIT_DIALOG]: '_handleOpenSubmitDialog',
- [Shortcut.TOGGLE_ATTENTION_SET]: '_handleToggleAttentionSet',
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.SEND_REPLY, _ => {}), // docOnly
+ listen(Shortcut.EMOJI_DROPDOWN, _ => {}), // docOnly
+ listen(Shortcut.REFRESH_CHANGE, _ => fireReload(this, true)),
+ listen(Shortcut.OPEN_REPLY_DIALOG, _ => this._handleOpenReplyDialog()),
+ listen(Shortcut.OPEN_DOWNLOAD_DIALOG, _ =>
+ this._handleOpenDownloadDialog()
+ ),
+ listen(Shortcut.TOGGLE_DIFF_MODE, _ => this._handleToggleDiffMode()),
+ listen(Shortcut.TOGGLE_CHANGE_STAR, e => {
+ if (this._throttledToggleChangeStar) {
+ this._throttledToggleChangeStar(e);
+ }
+ }),
+ listen(Shortcut.UP_TO_DASHBOARD, _ => this._determinePageBack()),
+ listen(Shortcut.EXPAND_ALL_MESSAGES, _ =>
+ this._handleExpandAllMessages()
+ ),
+ listen(Shortcut.COLLAPSE_ALL_MESSAGES, _ =>
+ this._handleCollapseAllMessages()
+ ),
+ listen(Shortcut.OPEN_DIFF_PREFS, _ =>
+ this._handleOpenDiffPrefsShortcut()
+ ),
+ listen(Shortcut.EDIT_TOPIC, _ => this.$.metadata.editTopic()),
+ listen(Shortcut.DIFF_AGAINST_BASE, _ => this._handleDiffAgainstBase()),
+ listen(Shortcut.DIFF_AGAINST_LATEST, _ =>
+ this._handleDiffAgainstLatest()
+ ),
+ listen(Shortcut.DIFF_BASE_AGAINST_LEFT, _ =>
+ this._handleDiffBaseAgainstLeft()
+ ),
+ listen(Shortcut.DIFF_RIGHT_AGAINST_LATEST, _ =>
+ this._handleDiffRightAgainstLatest()
+ ),
+ listen(Shortcut.DIFF_BASE_AGAINST_LATEST, _ =>
+ this._handleDiffBaseAgainstLatest()
+ ),
+ listen(Shortcut.OPEN_SUBMIT_DIALOG, _ => this._handleOpenSubmitDialog()),
+ listen(Shortcut.TOGGLE_ATTENTION_SET, _ =>
+ this._handleToggleAttentionSet()
+ ),
+ ];
}
disconnected$ = new Subject();
@@ -632,8 +651,8 @@ export class GrChangeView extends base {
override connectedCallback() {
super.connectedCallback();
- this._throttledToggleChangeStar = throttleWrap<IronKeyboardEvent>(e =>
- this._handleToggleChangeStar(e)
+ this._throttledToggleChangeStar = throttleWrap<KeyboardEvent>(_ =>
+ this._handleToggleChangeStar()
);
this._getServerConfig().then(config => {
this._serverConfig = config;
@@ -742,12 +761,7 @@ export class GrChangeView extends base {
if (e.detail.fixApplied) fireReload(this);
}
- _handleToggleDiffMode(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleToggleDiffMode() {
if (this.viewState.diffMode === DiffViewMode.SIDE_BY_SIDE) {
this.$.fileListHeader.setDiffViewMode(DiffViewMode.UNIFIED);
} else {
@@ -1486,52 +1500,22 @@ export class GrChangeView extends base {
return label;
}
- _handleOpenReplyDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
+ _handleOpenReplyDialog() {
this._getLoggedIn().then(isLoggedIn => {
if (!isLoggedIn) {
fireEvent(this, 'show-auth-required');
return;
}
-
- e.preventDefault();
this._openReplyDialog(this.$.replyDialog.FocusTarget.ANY);
});
}
- _handleOpenDownloadDialogShortcut(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
- this._handleOpenDownloadDialog();
- }
-
- _handleEditTopic(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
- this.$.metadata.editTopic();
- }
-
- _handleOpenSubmitDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || !this._submitEnabled) {
- return;
- }
-
- e.preventDefault();
+ _handleOpenSubmitDialog() {
+ if (!this._submitEnabled) return;
this.$.actions.showSubmitDialog();
}
- _handleToggleAttentionSet(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleToggleAttentionSet() {
if (!this._change || !this._account?._account_id) return;
if (!this._loggedIn || !isInvolved(this._change, this._account)) return;
if (!this._change.attention_set) this._change.attention_set = {};
@@ -1570,10 +1554,7 @@ export class GrChangeView extends base {
this._change = {...this._change};
}
- _handleDiffAgainstBase(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffAgainstBase() {
assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1584,10 +1565,7 @@ export class GrChangeView extends base {
GerritNav.navigateToChange(this._change, this._patchRange.patchNum);
}
- _handleDiffBaseAgainstLeft(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffBaseAgainstLeft() {
assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1598,10 +1576,7 @@ export class GrChangeView extends base {
GerritNav.navigateToChange(this._change, this._patchRange.basePatchNum);
}
- _handleDiffAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffAgainstLatest() {
assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1617,10 +1592,7 @@ export class GrChangeView extends base {
);
}
- _handleDiffRightAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffRightAgainstLatest() {
assertIsDefined(this._change, '_change');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
if (!this._patchRange)
@@ -1636,10 +1608,7 @@ export class GrChangeView extends base {
);
}
- _handleDiffBaseAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ _handleDiffBaseAgainstLatest() {
assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1654,59 +1623,24 @@ export class GrChangeView extends base {
GerritNav.navigateToChange(this._change, latestPatchNum);
}
- _handleRefreshChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
- e.preventDefault();
- fireReload(this, true);
- }
-
- _handleToggleChangeStar(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
- e.preventDefault();
+ _handleToggleChangeStar() {
this.$.changeStar.toggleStar();
}
- _handleUpToDashboard(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
- this._determinePageBack();
- }
-
- _handleExpandAllMessages(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleExpandAllMessages() {
if (this.messagesList) {
this.messagesList.handleExpandCollapse(true);
}
}
- _handleCollapseAllMessages(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleCollapseAllMessages() {
if (this.messagesList) {
this.messagesList.handleExpandCollapse(false);
}
}
- _handleOpenDiffPrefsShortcut(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
+ _handleOpenDiffPrefsShortcut() {
if (!this._loggedIn) return;
- e.preventDefault();
this.$.fileList.openDiffPrefs();
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index e508c635f6..6cbe59c838 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -82,17 +82,12 @@ import {
} from '../../../types/common';
import {
pressAndReleaseKeyOn,
- keyUpOn,
tap,
} from '@polymer/iron-test-helpers/mock-interactions';
import {GrEditControls} from '../../edit/gr-edit-controls/gr-edit-controls';
import {AppElementChangeViewParams} from '../../gr-app-types';
import {SinonFakeTimers, SinonStubbedMember} from 'sinon';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
-import {
- IronKeyboardEvent,
- IronKeyboardEventDetail,
-} from '../../../types/events';
import {CommentThread, UIRobot} from '../../../utils/comment-util';
import {GerritView} from '../../../services/router/router-model';
import {ParsedChangeInfo} from '../../../types/types';
@@ -403,7 +398,7 @@ suite('gr-change-view tests', () => {
patchNum: 3 as RevisionPatchSetNum,
basePatchNum: 1 as BasePatchSetNum,
};
- element._handleDiffAgainstBase(new CustomEvent('') as IronKeyboardEvent);
+ element._handleDiffAgainstBase();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[0], element._change);
@@ -419,7 +414,7 @@ suite('gr-change-view tests', () => {
basePatchNum: 1 as BasePatchSetNum,
patchNum: 3 as RevisionPatchSetNum,
};
- element._handleDiffAgainstLatest(new CustomEvent('') as IronKeyboardEvent);
+ element._handleDiffAgainstLatest();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[0], element._change);
@@ -436,9 +431,7 @@ suite('gr-change-view tests', () => {
patchNum: 3 as RevisionPatchSetNum,
basePatchNum: 1 as BasePatchSetNum,
};
- element._handleDiffBaseAgainstLeft(
- new CustomEvent('') as IronKeyboardEvent
- );
+ element._handleDiffBaseAgainstLeft();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[0], element._change);
@@ -454,9 +447,7 @@ suite('gr-change-view tests', () => {
basePatchNum: 1 as BasePatchSetNum,
patchNum: 3 as RevisionPatchSetNum,
};
- element._handleDiffRightAgainstLatest(
- new CustomEvent('') as IronKeyboardEvent
- );
+ element._handleDiffRightAgainstLatest();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[1], 10 as PatchSetNum);
@@ -472,9 +463,7 @@ suite('gr-change-view tests', () => {
basePatchNum: 1 as BasePatchSetNum,
patchNum: 3 as RevisionPatchSetNum,
};
- element._handleDiffBaseAgainstLatest(
- new CustomEvent('') as IronKeyboardEvent
- );
+ element._handleDiffBaseAgainstLatest();
assert(navigateToChangeStub.called);
const args = navigateToChangeStub.getCall(0).args;
assert.equal(args[1], 10 as PatchSetNum);
@@ -501,11 +490,11 @@ suite('gr-change-view tests', () => {
assert.isNotOk(element._change.attention_set);
await element._getLoggedIn();
await element.restApiService.getAccount();
- element._handleToggleAttentionSet(new CustomEvent('') as IronKeyboardEvent);
+ element._handleToggleAttentionSet();
assert.isTrue(addToAttentionSetStub.called);
assert.isFalse(removeFromAttentionSetStub.called);
- element._handleToggleAttentionSet(new CustomEvent('') as IronKeyboardEvent);
+ element._handleToggleAttentionSet();
assert.isTrue(removeFromAttentionSetStub.called);
});
@@ -650,7 +639,7 @@ suite('gr-change-view tests', () => {
sinon.stub(element, '_getLoggedIn').returns(Promise.resolve(false));
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- keyUpOn(element, 65, null, 'a');
+ pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isFalse(element.$.replyOverlay.opened);
assert.isTrue(loggedInErrorSpy.called);
@@ -683,7 +672,7 @@ suite('gr-change-view tests', () => {
const openSpy = sinon.spy(element, '_openReplyDialog');
- keyUpOn(element, 65, null, 'a');
+ pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isTrue(element.$.replyOverlay.opened);
element.$.replyOverlay.close();
@@ -796,7 +785,7 @@ suite('gr-change-view tests', () => {
const stub = sinon
.stub(element.$.downloadOverlay, 'open')
.returns(Promise.resolve());
- keyUpOn(element, 68, null, 'd');
+ pressAndReleaseKeyOn(element, 68, null, 'd');
assert.isTrue(stub.called);
});
@@ -819,17 +808,14 @@ suite('gr-change-view tests', () => {
element.$.fileListHeader,
'setDiffViewMode'
);
- const e = new CustomEvent<IronKeyboardEventDetail>('keydown', {
- detail: {keyboardEvent: new KeyboardEvent('keydown'), key: 'x'},
- });
flush();
element.viewState.diffMode = DiffViewMode.SIDE_BY_SIDE;
- element._handleToggleDiffMode(e);
+ element._handleToggleDiffMode();
assert.isTrue(setModeStub.calledWith(DiffViewMode.UNIFIED));
element.viewState.diffMode = DiffViewMode.UNIFIED;
- element._handleToggleDiffMode(e);
+ element._handleToggleDiffMode();
assert.isTrue(setModeStub.calledWith(DiffViewMode.SIDE_BY_SIDE));
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index 4fd5ff32b7..cc76145026 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -36,6 +36,7 @@ import {asyncForeach, debounce, DelayedTask} from '../../../utils/async-util';
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {FilesExpandedState} from '../gr-file-list-constants';
import {pluralize} from '../../../utils/string-util';
@@ -50,10 +51,9 @@ import {
} from '../../../constants/constants';
import {
addGlobalShortcut,
+ addShortcut,
descendedFromClass,
- isShiftPressed,
Key,
- modifierPressed,
toggleClass,
} from '../../../utils/dom-util';
import {
@@ -80,7 +80,6 @@ import {GrDiffCursor} from '../../diff/gr-diff-cursor/gr-diff-cursor';
import {GrCursorManager} from '../../shared/gr-cursor-manager/gr-cursor-manager';
import {PolymerSpliceChange} from '@polymer/polymer/interfaces';
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
-import {IronKeyboardEvent} from '../../../types/events';
import {ParsedChangeInfo, PatchSetFile} from '../../../types/types';
import {Timing} from '../../../constants/reporting';
import {RevisionInfo} from '../../shared/revision-info/revision-info';
@@ -88,6 +87,7 @@ import {preferences$} from '../../../services/user/user-model';
import {changeComments$} from '../../../services/comments/comments-model';
import {Subject} from 'rxjs';
import {takeUntil} from 'rxjs/operators';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
export const DEFAULT_NUM_FILES_SHOWN = 200;
@@ -200,9 +200,6 @@ export class GrFileList extends base {
selectedIndex = -1;
@property({type: Object})
- keyEventTarget = document.body;
-
- @property({type: Object})
change?: ParsedChangeInfo;
@property({type: String, notify: true, observer: '_updateDiffPreferences'})
@@ -324,45 +321,53 @@ export class GrFileList extends base {
/** Called in disconnectedCallback. */
private cleanups: (() => void)[] = [];
- override keyboardShortcuts() {
- return {
- [Shortcut.LEFT_PANE]: '_handleLeftPane',
- [Shortcut.RIGHT_PANE]: '_handleRightPane',
- [Shortcut.TOGGLE_INLINE_DIFF]: '_handleToggleInlineDiff',
- [Shortcut.TOGGLE_ALL_INLINE_DIFFS]: '_handleToggleAllInlineDiffs',
- [Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS]:
- '_handleToggleHideAllCommentThreads',
- [Shortcut.CURSOR_NEXT_FILE]: '_handleCursorNext',
- [Shortcut.CURSOR_PREV_FILE]: '_handleCursorPrev',
- [Shortcut.NEXT_LINE]: '_handleCursorNext',
- [Shortcut.PREV_LINE]: '_handleCursorPrev',
- [Shortcut.NEW_COMMENT]: '_handleNewComment',
- [Shortcut.OPEN_LAST_FILE]: '_handleOpenLastFile',
- [Shortcut.OPEN_FIRST_FILE]: '_handleOpenFirstFile',
- [Shortcut.OPEN_FILE]: '_handleOpenFile',
- [Shortcut.NEXT_CHUNK]: '_handleNextChunk',
- [Shortcut.PREV_CHUNK]: '_handlePrevChunk',
- [Shortcut.TOGGLE_FILE_REVIEWED]: '_handleToggleFileReviewed',
- [Shortcut.TOGGLE_LEFT_PANE]: '_handleToggleLeftPane',
-
- // Final two are actually handled by gr-comment-thread.
- [Shortcut.EXPAND_ALL_COMMENT_THREADS]: null,
- [Shortcut.COLLAPSE_ALL_COMMENT_THREADS]: null,
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.LEFT_PANE, _ => this._handleLeftPane()),
+ listen(Shortcut.RIGHT_PANE, _ => this._handleRightPane()),
+ listen(Shortcut.TOGGLE_INLINE_DIFF, _ => this._handleToggleInlineDiff()),
+ listen(Shortcut.TOGGLE_ALL_INLINE_DIFFS, _ => this._toggleInlineDiffs()),
+ listen(Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS, _ =>
+ toggleClass(this, 'hideComments')
+ ),
+ listen(Shortcut.CURSOR_NEXT_FILE, e => this._handleCursorNext(e)),
+ listen(Shortcut.CURSOR_PREV_FILE, e => this._handleCursorPrev(e)),
+ // This is already been taken care of by CURSOR_NEXT_FILE above. The two
+ // shortcuts share the same bindings. It depends on whether all files
+ // are expanded whether the cursor moves to the next file or line.
+ listen(Shortcut.NEXT_LINE, _ => {}), // docOnly
+ // This is already been taken care of by CURSOR_PREV_FILE above. The two
+ // shortcuts share the same bindings. It depends on whether all files
+ // are expanded whether the cursor moves to the previous file or line.
+ listen(Shortcut.PREV_LINE, _ => {}), // docOnly
+ listen(Shortcut.NEW_COMMENT, _ => this._handleNewComment()),
+ listen(Shortcut.OPEN_LAST_FILE, _ =>
+ this._openSelectedFile(this._files.length - 1)
+ ),
+ listen(Shortcut.OPEN_FIRST_FILE, _ => this._openSelectedFile(0)),
+ listen(Shortcut.OPEN_FILE, _ => this.handleOpenFile()),
+ listen(Shortcut.NEXT_CHUNK, _ => this._handleNextChunk()),
+ listen(Shortcut.PREV_CHUNK, _ => this._handlePrevChunk()),
+ listen(Shortcut.NEXT_COMMENT_THREAD, _ => this._handleNextComment()),
+ listen(Shortcut.PREV_COMMENT_THREAD, _ => this._handlePrevComment()),
+ listen(Shortcut.TOGGLE_FILE_REVIEWED, _ =>
+ this._handleToggleFileReviewed()
+ ),
+ listen(Shortcut.TOGGLE_LEFT_PANE, _ => this._handleToggleLeftPane()),
+ listen(Shortcut.EXPAND_ALL_COMMENT_THREADS, _ => {}), // docOnly
+ listen(Shortcut.COLLAPSE_ALL_COMMENT_THREADS, _ => {}), // docOnly
+ ];
}
private fileCursor = new GrCursorManager();
private diffCursor = new GrDiffCursor();
- private readonly shortcuts = appContext.shortcutsService;
-
constructor() {
super();
this.fileCursor.scrollMode = ScrollMode.KEEP_VISIBLE;
this.fileCursor.cursorTargetClass = 'selected';
this.fileCursor.focusOnMove = true;
- this.addEventListener('keydown', e => this._scopedKeydownHandler(e));
}
override connectedCallback() {
@@ -415,7 +420,8 @@ export class GrFileList extends base {
}
});
this.cleanups.push(
- addGlobalShortcut({key: Key.ESC}, e => this._handleEscKey(e))
+ addGlobalShortcut({key: Key.ESC}, _ => this._handleEscKey()),
+ addShortcut(this, {key: Key.ENTER}, _ => this.handleOpenFile())
);
}
@@ -430,17 +436,6 @@ export class GrFileList extends base {
super.disconnectedCallback();
}
- /**
- * Iron-a11y-keys-behavior catches keyboard events globally. Some keyboard
- * events must be scoped to a component level (e.g. `enter`) in order to not
- * override native browser functionality.
- *
- * Context: Issue 7277
- */
- _scopedKeydownHandler(e: KeyboardEvent) {
- if (e.keyCode === 13) this.handleOpenFile(e);
- }
-
reload() {
if (!this.changeNum || !this.patchRange?.patchNum) {
return Promise.resolve();
@@ -885,196 +880,84 @@ export class GrFileList extends base {
return fileData;
}
- _handleLeftPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this._noDiffsExpanded()) {
- return;
- }
-
- e.preventDefault();
+ _handleLeftPane() {
+ if (this._noDiffsExpanded()) return;
this.diffCursor.moveLeft();
}
- _handleRightPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this._noDiffsExpanded()) {
- return;
- }
-
- e.preventDefault();
+ _handleRightPane() {
+ if (this._noDiffsExpanded()) return;
this.diffCursor.moveRight();
}
- _handleToggleInlineDiff(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- this.shortcuts.modifierPressed(e) ||
- e.detail?.keyboardEvent?.repeat ||
- this.fileCursor.index === -1
- ) {
- return;
- }
-
- e.preventDefault();
+ _handleToggleInlineDiff() {
+ if (this.fileCursor.index === -1) return;
this._toggleFileExpandedByIndex(this.fileCursor.index);
}
- _handleToggleAllInlineDiffs(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || e.detail?.keyboardEvent?.repeat) {
- return;
- }
-
- e.preventDefault();
- this._toggleInlineDiffs();
- }
-
- _handleToggleHideAllCommentThreads(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
- toggleClass(this, 'hideComments');
- }
-
- _handleCursorNext(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
+ _handleCursorNext(e: KeyboardEvent) {
if (this.filesExpanded === FilesExpandedState.ALL) {
- e.preventDefault();
this.diffCursor.moveDown();
this._displayLine = true;
} else {
- // Down key
- if (e.detail.keyboardEvent.keyCode === 40) {
- return;
- }
- e.preventDefault();
+ if (e.key === Key.DOWN) return;
this.fileCursor.next({circular: true});
this.selectedIndex = this.fileCursor.index;
}
}
- _handleCursorPrev(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
-
+ _handleCursorPrev(e: KeyboardEvent) {
if (this.filesExpanded === FilesExpandedState.ALL) {
- e.preventDefault();
this.diffCursor.moveUp();
this._displayLine = true;
} else {
- // Up key
- if (e.detail.keyboardEvent.keyCode === 38) {
- return;
- }
- e.preventDefault();
+ if (e.key === Key.UP) return;
this.fileCursor.previous({circular: true});
this.selectedIndex = this.fileCursor.index;
}
}
- _handleNewComment(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
- e.preventDefault();
+ _handleNewComment() {
this.classList.remove('hideComments');
this.diffCursor.createCommentInPlace();
}
- _handleOpenLastFile(e: IronKeyboardEvent) {
- // Check for meta key to avoid overriding native chrome shortcut.
- if (this.shortcuts.shouldSuppress(e) || e.detail.keyboardEvent.metaKey) {
- return;
- }
-
- e.preventDefault();
- this._openSelectedFile(this._files.length - 1);
- }
-
- _handleOpenFirstFile(e: IronKeyboardEvent) {
- // Check for meta key to avoid overriding native chrome shortcut.
- if (this.shortcuts.shouldSuppress(e) || e.detail.keyboardEvent.metaKey) {
- return;
- }
-
- e.preventDefault();
- this._openSelectedFile(0);
- }
-
- _handleOpenFile(e: IronKeyboardEvent) {
- if (this.shortcuts.modifierPressed(e)) return;
- this.handleOpenFile(e.detail.keyboardEvent);
- }
-
- handleOpenFile(e: KeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || modifierPressed(e)) {
- return;
- }
- e.preventDefault();
-
+ handleOpenFile() {
if (this.filesExpanded === FilesExpandedState.ALL) {
this._openCursorFile();
return;
}
-
this._openSelectedFile();
}
- _handleNextChunk(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !isShiftPressed(e)) ||
- this._noDiffsExpanded()
- ) {
- return;
- }
-
- e.preventDefault();
- if (isShiftPressed(e)) {
- this.diffCursor.moveToNextCommentThread();
- } else {
- this.diffCursor.moveToNextChunk();
- }
+ _handleNextChunk() {
+ if (this._noDiffsExpanded()) return;
+ this.diffCursor.moveToNextChunk();
}
- _handlePrevChunk(e: IronKeyboardEvent) {
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !isShiftPressed(e)) ||
- this._noDiffsExpanded()
- ) {
- return;
- }
+ _handleNextComment() {
+ if (this._noDiffsExpanded()) return;
+ this.diffCursor.moveToNextCommentThread();
+ }
- e.preventDefault();
- if (isShiftPressed(e)) {
- this.diffCursor.moveToPreviousCommentThread();
- } else {
- this.diffCursor.moveToPreviousChunk();
- }
+ _handlePrevChunk() {
+ if (this._noDiffsExpanded()) return;
+ this.diffCursor.moveToPreviousChunk();
}
- _handleToggleFileReviewed(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
- return;
- }
+ _handlePrevComment() {
+ if (this._noDiffsExpanded()) return;
+ this.diffCursor.moveToPreviousCommentThread();
+ }
- e.preventDefault();
+ _handleToggleFileReviewed() {
if (!this._files[this.fileCursor.index]) {
return;
}
this._reviewFile(this._files[this.fileCursor.index].__path);
}
- _handleToggleLeftPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleToggleLeftPane() {
this._forEachDiff(diff => {
diff.toggleLeftDiff();
});
@@ -1546,9 +1429,7 @@ export class GrFileList extends base {
return undefined;
}
- _handleEscKey(e: KeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- e.preventDefault();
+ _handleEscKey() {
this._displayLine = false;
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
index 062d6a2754..7409be756f 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
@@ -27,11 +27,11 @@ import {GerritNav} from '../../core/gr-navigation/gr-navigation.js';
import {runA11yAudit} from '../../../test/a11y-test-utils.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
import {
- stubRestApi,
- spyRestApi,
listenOnce,
mockPromise,
query,
+ spyRestApi,
+ stubRestApi,
} from '../../../test/test-utils.js';
import {EditPatchSetNum} from '../../../types/common.js';
import {createCommentThreads} from '../../../utils/comment-util.js';
@@ -470,7 +470,7 @@ suite('gr-file-list tests', () => {
// https://github.com/sinonjs/sinon/issues/781
const diffsStub = sinon.stub(element, 'diffs')
.get(() => [{toggleLeftDiff: toggleLeftDiffStub}]);
- MockInteractions.pressAndReleaseKeyOn(element, 65, 'shift', 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'A');
assert.isTrue(toggleLeftDiffStub.calledOnce);
diffsStub.restore();
});
@@ -486,7 +486,7 @@ suite('gr-file-list tests', () => {
assert.isFalse(items[1].classList.contains('selected'));
assert.isFalse(items[2].classList.contains('selected'));
// j with a modifier should not move the cursor.
- MockInteractions.pressAndReleaseKeyOn(element, 74, 'shift', 'j');
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'J');
assert.equal(element.fileCursor.index, 0);
// down should not move the cursor.
MockInteractions.pressAndReleaseKeyOn(element, 40, null, 'down');
@@ -502,7 +502,7 @@ suite('gr-file-list tests', () => {
assert.equal(element.selectedIndex, 2);
// k with a modifier should not move the cursor.
- MockInteractions.pressAndReleaseKeyOn(element, 75, 'shift', 'k');
+ MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'K');
assert.equal(element.fileCursor.index, 2);
// up should not move the cursor.
@@ -560,7 +560,7 @@ suite('gr-file-list tests', () => {
assert.equal(element._expandedFiles.length, 1);
assert.equal(element._expandedFiles[0].path, paths[1]);
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i');
+ MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'I');
flush();
assert.equal(element.diffs.length, paths.length);
assert.equal(element._expandedFiles.length, paths.length);
@@ -569,7 +569,7 @@ suite('gr-file-list tests', () => {
}
// since _expandedFilesChanged is stubbed
element.filesExpanded = FilesExpandedState.ALL;
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i');
+ MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'I');
flush();
assert.equal(element.diffs.length, 0);
assert.equal(element._expandedFiles.length, 0);
@@ -584,16 +584,16 @@ suite('gr-file-list tests', () => {
assert.equal(getNumReviewed(), 0);
// Press the review key to toggle it (set the flag).
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
flush();
assert.equal(getNumReviewed(), 1);
// Press the review key to toggle it (clear the flag).
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.equal(getNumReviewed(), 0);
});
- suite('_handleOpenFile', () => {
+ suite('handleOpenFile', () => {
let interact;
setup(() => {
@@ -605,14 +605,7 @@ suite('gr-file-list tests', () => {
openCursorStub.reset();
openSelectedStub.reset();
expandStub.reset();
-
- const keyboardEvent = new KeyboardEvent('keydown');
- const e = new CustomEvent('keydown', {
- detail: {keyboardEvent, key: 'x'},
- });
- sinon.stub(keyboardEvent, 'preventDefault');
- element._handleOpenFile(e);
- assert.isTrue(keyboardEvent.preventDefault.called);
+ element.handleOpenFile();
const result = {};
if (openCursorStub.called) {
result.opened_cursor = true;
@@ -653,16 +646,20 @@ suite('gr-file-list tests', () => {
sinon.stub(element, '_noDiffsExpanded')
.callsFake(() => noDiffsExpanded);
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'left');
+ MockInteractions.pressAndReleaseKeyOn(
+ element, 73, 'shift', 'ArrowLeft');
assert.isFalse(moveLeftStub.called);
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'right');
+ MockInteractions.pressAndReleaseKeyOn(
+ element, 73, 'shift', 'ArrowRight');
assert.isFalse(moveRightStub.called);
noDiffsExpanded = false;
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'left');
+ MockInteractions.pressAndReleaseKeyOn(
+ element, 73, 'shift', 'ArrowLeft');
assert.isTrue(moveLeftStub.called);
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'right');
+ MockInteractions.pressAndReleaseKeyOn(
+ element, 73, 'shift', 'ArrowRight');
assert.isTrue(moveRightStub.called);
});
});
@@ -1590,7 +1587,7 @@ suite('gr-file-list tests', () => {
});
test('cursor with toggle all files', async () => {
- MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i');
+ MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'I');
await flush();
const diffs = await renderAndGetNewDiffs(0);
@@ -1620,14 +1617,12 @@ suite('gr-file-list tests', () => {
});
suite('n key presses', () => {
- let nKeySpy;
let nextCommentStub;
let nextChunkStub;
let fileRows;
setup(() => {
sinon.stub(element, '_renderInOrder').returns(Promise.resolve());
- nKeySpy = sinon.spy(element, '_handleNextChunk');
nextCommentStub = sinon.stub(element.diffCursor,
'moveToNextCommentThread');
nextChunkStub = sinon.stub(element.diffCursor,
@@ -1636,58 +1631,40 @@ suite('gr-file-list tests', () => {
element.root.querySelectorAll('.row:not(.header-row)');
});
- test('n key with some files expanded and no shift key', async () => {
+ test('n key with some files expanded', async () => {
MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'i');
await flush();
+ assert.equal(element.filesExpanded, FilesExpandedState.SOME);
- // Handle N key should return before calling diff cursor functions.
MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'n');
- assert.isTrue(nKeySpy.called);
- assert.isFalse(nextCommentStub.called);
-
- // This is also called in diffCursor.moveToFirstChunk.
- assert.equal(nextChunkStub.callCount, 1);
- assert.equal(element.filesExpanded, 'some');
+ assert.isTrue(nextChunkStub.calledOnce);
});
- test('n key with some files expanded and shift key', async () => {
+ test('N key with some files expanded', async () => {
MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'i');
await flush();
- assert.equal(nextChunkStub.callCount, 0);
-
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
- assert.isTrue(nKeySpy.called);
- assert.isTrue(nextCommentStub.called);
+ assert.equal(element.filesExpanded, FilesExpandedState.SOME);
- // This is also called in diffCursor.moveToFirstChunk.
- assert.equal(nextChunkStub.callCount, 0);
- assert.equal(element.filesExpanded, 'some');
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
+ assert.isTrue(nextCommentStub.calledOnce);
});
- test('n key without all files expanded and shift key', async () => {
- MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, 'shift', 'i');
+ test('n key with all files expanded', async () => {
+ MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'I');
await flush();
+ assert.equal(element.filesExpanded, FilesExpandedState.ALL);
MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'n');
- assert.isTrue(nKeySpy.called);
- assert.isFalse(nextCommentStub.called);
-
- // This is also called in diffCursor.moveToFirstChunk.
- assert.equal(nextChunkStub.callCount, 1);
- assert.equal(element.filesExpanded, FilesExpandedState.ALL);
+ assert.isTrue(nextChunkStub.calledOnce);
});
- test('n key without all files expanded and no shift key', async () => {
- MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, 'shift', 'i');
+ test('N key with all files expanded', async () => {
+ MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'I');
await flush();
+ assert.equal(element.filesExpanded, FilesExpandedState.ALL);
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
- assert.isTrue(nKeySpy.called);
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
assert.isTrue(nextCommentStub.called);
-
- // This is also called in diffCursor.moveToFirstChunk.
- assert.equal(nextChunkStub.callCount, 0);
- assert.equal(element.filesExpanded, FilesExpandedState.ALL);
});
});
@@ -1708,26 +1685,17 @@ suite('gr-file-list tests', () => {
test('_displayLine', () => {
element.filesExpanded = FilesExpandedState.ALL;
- const mockEvent = {
- preventDefault() {},
- composedPath() { return []; },
- detail: {
- keyboardEvent: {
- composedPath() { return []; },
- },
- },
- };
element._displayLine = false;
- element._handleCursorNext(mockEvent);
+ element._handleCursorNext(new KeyboardEvent('keydown'));
assert.isTrue(element._displayLine);
element._displayLine = false;
- element._handleCursorPrev(mockEvent);
+ element._handleCursorPrev(new KeyboardEvent('keydown'));
assert.isTrue(element._displayLine);
element._displayLine = true;
- element._handleEscKey(mockEvent);
+ element._handleEscKey();
assert.isFalse(element._displayLine);
});
@@ -1737,13 +1705,13 @@ suite('gr-file-list tests', () => {
const saveReviewStub = sinon.stub(element, '_saveReviewedState');
element.editMode = false;
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(saveReviewStub.calledOnce);
element.editMode = true;
await flush();
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(saveReviewStub.calledOnce);
});
diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts
index c91ae5a0a5..8610999895 100644
--- a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts
+++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.ts
@@ -22,11 +22,11 @@ import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-keyboard-shortcuts-dialog_html';
import {
ShortcutSection,
- ShortcutListener,
SectionView,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {property, customElement} from '@polymer/decorators';
import {appContext} from '../../../services/app-context';
+import {ShortcutViewListener} from '../../../services/shortcuts/shortcuts-service';
declare global {
interface HTMLElementTagNameMap {
@@ -57,7 +57,7 @@ export class GrKeyboardShortcutsDialog extends PolymerElement {
@property({type: Array})
_right?: SectionShortcut[];
- private readonly shortcutListener: ShortcutListener;
+ private readonly shortcutListener: ShortcutViewListener;
private readonly shortcuts = appContext.shortcutsService;
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
index dd1ebd19c4..2901b8a43b 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
@@ -22,6 +22,7 @@ import {htmlTemplate} from './gr-search-bar_html';
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {customElement, property} from '@polymer/decorators';
import {ServerInfo} from '../../../types/common';
@@ -31,9 +32,9 @@ import {
GrAutocomplete,
} from '../../shared/gr-autocomplete/gr-autocomplete';
import {getDocsBaseUrl} from '../../../utils/url-util';
-import {IronKeyboardEvent} from '../../../types/events';
import {MergeabilityComputationBehavior} from '../../../constants/constants';
import {appContext} from '../../../services/app-context';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
// Possible static search options for auto complete, without negations.
const SEARCH_OPERATORS: ReadonlyArray<string> = [
@@ -169,9 +170,6 @@ export class GrSearchBar extends base {
value = '';
@property({type: Object})
- keyEventTarget: unknown = document.body;
-
- @property({type: Object})
query: AutocompleteQuery;
@property({type: Object})
@@ -197,8 +195,6 @@ export class GrSearchBar extends base {
private readonly restApiService = appContext.restApiService;
- private readonly shortcuts = appContext.shortcutsService;
-
constructor() {
super();
this.query = (input: string) => this._getSearchSuggestions(input);
@@ -245,10 +241,8 @@ export class GrSearchBar extends base {
}
}
- override keyboardShortcuts() {
- return {
- [Shortcut.SEARCH]: '_handleSearch',
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [listen(Shortcut.SEARCH, _ => this._handleSearch())];
}
_valueChanged(value: string) {
@@ -396,16 +390,7 @@ export class GrSearchBar extends base {
});
}
- _handleSearch(e: IronKeyboardEvent) {
- const keyboardEvent = e.detail.keyboardEvent;
- if (
- this.shortcuts.shouldSuppress(e) ||
- (this.shortcuts.modifierPressed(e) && !keyboardEvent.shiftKey)
- ) {
- return;
- }
-
- e.preventDefault();
+ _handleSearch() {
this.$.searchInput.focus();
this.$.searchInput.selectAll();
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 236f00f0fb..08139006a6 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -37,6 +37,7 @@ import {htmlTemplate} from './gr-diff-view_html';
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
ShortcutSection,
} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {
@@ -94,16 +95,11 @@ import {LineOfInterest} from '../gr-diff/gr-diff';
import {RevisionInfo as RevisionInfoObj} from '../../shared/revision-info/revision-info';
import {
CommentMap,
- isInBaseOfPatchRange,
getPatchRangeForCommentUrl,
+ isInBaseOfPatchRange,
} from '../../../utils/comment-util';
import {AppElementParams} from '../../gr-app-types';
-import {
- IronKeyboardEventListener,
- IronKeyboardEvent,
- EventType,
- OpenFixPreviewEvent,
-} from '../../../types/events';
+import {EventType, OpenFixPreviewEvent} from '../../../types/events';
import {fireAlert, fireEvent, fireTitleChange} from '../../../utils/event-util';
import {GerritView} from '../../../services/router/router-model';
import {assertIsDefined} from '../../../utils/common-util';
@@ -114,6 +110,7 @@ import {changeComments$} from '../../../services/comments/comments-model';
import {takeUntil} from 'rxjs/operators';
import {Subject} from 'rxjs';
import {preferences$} from '../../../services/user/user-model';
+import {listen} from '../../../services/shortcuts/shortcuts-service';
const ERR_REVIEW_STATUS = 'Couldn’t change file review status.';
const LOADING_BLAME = 'Loading blame...';
@@ -168,9 +165,6 @@ export class GrDiffView extends base {
@property({type: Object, observer: '_paramsChanged'})
params?: AppElementParams;
- @property({type: Object})
- keyEventTarget: HTMLElement = document.body;
-
@property({type: Object, notify: true, observer: '_changeViewStateChanged'})
changeViewState: Partial<ChangeViewState> = {};
@@ -284,46 +278,71 @@ export class GrDiffView extends base {
/** Called in disconnectedCallback. */
private cleanups: (() => void)[] = [];
- override keyboardShortcuts() {
- return {
- [Shortcut.LEFT_PANE]: '_handleLeftPane',
- [Shortcut.RIGHT_PANE]: '_handleRightPane',
- [Shortcut.NEXT_LINE]: '_handleNextLineOrFileWithComments',
- [Shortcut.PREV_LINE]: '_handlePrevLineOrFileWithComments',
- [Shortcut.VISIBLE_LINE]: '_handleVisibleLine',
- [Shortcut.NEXT_FILE_WITH_COMMENTS]: '_handleNextLineOrFileWithComments',
- [Shortcut.PREV_FILE_WITH_COMMENTS]: '_handlePrevLineOrFileWithComments',
- [Shortcut.NEW_COMMENT]: '_handleNewComment',
- [Shortcut.SAVE_COMMENT]: null, // DOC_ONLY binding
- [Shortcut.NEXT_FILE]: '_handleNextFile',
- [Shortcut.PREV_FILE]: '_handlePrevFile',
- [Shortcut.NEXT_CHUNK]: '_handleNextChunkOrCommentThread',
- [Shortcut.NEXT_COMMENT_THREAD]: '_handleNextChunkOrCommentThread',
- [Shortcut.PREV_CHUNK]: '_handlePrevChunkOrCommentThread',
- [Shortcut.PREV_COMMENT_THREAD]: '_handlePrevChunkOrCommentThread',
- [Shortcut.OPEN_REPLY_DIALOG]: '_handleOpenReplyDialog',
- [Shortcut.TOGGLE_LEFT_PANE]: '_handleToggleLeftPane',
- [Shortcut.OPEN_DOWNLOAD_DIALOG]: '_handleOpenDownloadDialog',
- [Shortcut.UP_TO_CHANGE]: '_handleUpToChange',
- [Shortcut.OPEN_DIFF_PREFS]: '_handleCommaKey',
- [Shortcut.TOGGLE_DIFF_MODE]: '_handleToggleDiffMode',
- [Shortcut.TOGGLE_FILE_REVIEWED]: '_throttledToggleFileReviewed',
- [Shortcut.TOGGLE_ALL_DIFF_CONTEXT]: '_handleToggleAllDiffContext',
- [Shortcut.NEXT_UNREVIEWED_FILE]: '_handleNextUnreviewedFile',
- [Shortcut.TOGGLE_BLAME]: '_handleToggleBlame',
- [Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS]:
- '_handleToggleHideAllCommentThreads',
- [Shortcut.OPEN_FILE_LIST]: '_handleOpenFileList',
- [Shortcut.DIFF_AGAINST_BASE]: '_handleDiffAgainstBase',
- [Shortcut.DIFF_AGAINST_LATEST]: '_handleDiffAgainstLatest',
- [Shortcut.DIFF_BASE_AGAINST_LEFT]: '_handleDiffBaseAgainstLeft',
- [Shortcut.DIFF_RIGHT_AGAINST_LATEST]: '_handleDiffRightAgainstLatest',
- [Shortcut.DIFF_BASE_AGAINST_LATEST]: '_handleDiffBaseAgainstLatest',
-
- // Final two are actually handled by gr-comment-thread.
- [Shortcut.EXPAND_ALL_COMMENT_THREADS]: null,
- [Shortcut.COLLAPSE_ALL_COMMENT_THREADS]: null,
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.LEFT_PANE, _ => this.cursor.moveLeft()),
+ listen(Shortcut.RIGHT_PANE, _ => this.cursor.moveRight()),
+ listen(Shortcut.NEXT_LINE, _ => this._handleNextLine()),
+ listen(Shortcut.PREV_LINE, _ => this._handlePrevLine()),
+ listen(Shortcut.VISIBLE_LINE, _ => this.cursor.moveToVisibleArea()),
+ listen(Shortcut.NEXT_FILE_WITH_COMMENTS, _ =>
+ this._moveToNextFileWithComment()
+ ),
+ listen(Shortcut.PREV_FILE_WITH_COMMENTS, _ =>
+ this._moveToPreviousFileWithComment()
+ ),
+ listen(Shortcut.NEW_COMMENT, _ => this._handleNewComment()),
+ listen(Shortcut.SAVE_COMMENT, _ => {}),
+ listen(Shortcut.NEXT_FILE, _ => this._handleNextFile()),
+ listen(Shortcut.PREV_FILE, _ => this._handlePrevFile()),
+ listen(Shortcut.NEXT_CHUNK, _ => this._handleNextChunk()),
+ listen(Shortcut.PREV_CHUNK, _ => this._handlePrevChunk()),
+ listen(Shortcut.NEXT_COMMENT_THREAD, _ =>
+ this._handleNextCommentThread()
+ ),
+ listen(Shortcut.PREV_COMMENT_THREAD, _ =>
+ this._handlePrevCommentThread()
+ ),
+ listen(Shortcut.OPEN_REPLY_DIALOG, _ => this._handleOpenReplyDialog()),
+ listen(Shortcut.TOGGLE_LEFT_PANE, _ => this._handleToggleLeftPane()),
+ listen(Shortcut.OPEN_DOWNLOAD_DIALOG, _ =>
+ this._handleOpenDownloadDialog()
+ ),
+ listen(Shortcut.UP_TO_CHANGE, _ => this._handleUpToChange()),
+ listen(Shortcut.OPEN_DIFF_PREFS, _ => this._handleCommaKey()),
+ listen(Shortcut.TOGGLE_DIFF_MODE, _ => this._handleToggleDiffMode()),
+ listen(Shortcut.TOGGLE_FILE_REVIEWED, e => {
+ if (this._throttledToggleFileReviewed) {
+ this._throttledToggleFileReviewed(e);
+ }
+ }),
+ listen(Shortcut.TOGGLE_ALL_DIFF_CONTEXT, _ =>
+ this._handleToggleAllDiffContext()
+ ),
+ listen(Shortcut.NEXT_UNREVIEWED_FILE, _ =>
+ this._handleNextUnreviewedFile()
+ ),
+ listen(Shortcut.TOGGLE_BLAME, _ => this._handleToggleBlame()),
+ listen(Shortcut.TOGGLE_HIDE_ALL_COMMENT_THREADS, _ =>
+ this._handleToggleHideAllCommentThreads()
+ ),
+ listen(Shortcut.OPEN_FILE_LIST, _ => this._handleOpenFileList()),
+ listen(Shortcut.DIFF_AGAINST_BASE, _ => this._handleDiffAgainstBase()),
+ listen(Shortcut.DIFF_AGAINST_LATEST, _ =>
+ this._handleDiffAgainstLatest()
+ ),
+ listen(Shortcut.DIFF_BASE_AGAINST_LEFT, _ =>
+ this._handleDiffBaseAgainstLeft()
+ ),
+ listen(Shortcut.DIFF_RIGHT_AGAINST_LATEST, _ =>
+ this._handleDiffRightAgainstLatest()
+ ),
+ listen(Shortcut.DIFF_BASE_AGAINST_LATEST, _ =>
+ this._handleDiffBaseAgainstLatest()
+ ),
+ listen(Shortcut.EXPAND_ALL_COMMENT_THREADS, _ => {}), // docOnly
+ listen(Shortcut.COLLAPSE_ALL_COMMENT_THREADS, _ => {}), // docOnly
+ ];
}
private readonly reporting = appContext.reportingService;
@@ -334,7 +353,7 @@ export class GrDiffView extends base {
private readonly shortcuts = appContext.shortcutsService;
- _throttledToggleFileReviewed?: IronKeyboardEventListener;
+ _throttledToggleFileReviewed?: (e: KeyboardEvent) => void;
_onRenderHandler?: EventListener;
@@ -344,8 +363,8 @@ export class GrDiffView extends base {
override connectedCallback() {
super.connectedCallback();
- this._throttledToggleFileReviewed = throttleWrap(e =>
- this._handleToggleFileReviewed(e)
+ this._throttledToggleFileReviewed = throttleWrap(_ =>
+ this._handleToggleFileReviewed()
);
this._getLoggedIn().then(loggedIn => {
this._loggedIn = loggedIn;
@@ -371,7 +390,10 @@ export class GrDiffView extends base {
};
this.$.diffHost.addEventListener('render', this._onRenderHandler);
this.cleanups.push(
- addGlobalShortcut({key: Key.ESC}, e => this._handleEscKey(e))
+ addGlobalShortcut(
+ {key: Key.ESC},
+ _ => (this.$.diffHost.displayLine = false)
+ )
);
}
@@ -525,81 +547,20 @@ export class GrDiffView extends base {
);
}
- _handleToggleFileReviewed(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
- e.preventDefault();
+ _handleToggleFileReviewed() {
this._setReviewed(!this.$.reviewed.checked);
}
- _handleEscKey(e: KeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- e.preventDefault();
- this.$.diffHost.displayLine = false;
- }
-
- _handleLeftPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- e.preventDefault();
- this.cursor.moveLeft();
- }
-
- _handleRightPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- e.preventDefault();
- this.cursor.moveRight();
- }
-
- _handlePrevLineOrFileWithComments(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- if (
- e.detail.keyboardEvent?.shiftKey &&
- e.detail.keyboardEvent?.keyCode === 75
- ) {
- // 'K'
- this._moveToPreviousFileWithComment();
- return;
- }
- if (this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handlePrevLine() {
this.$.diffHost.displayLine = true;
this.cursor.moveUp();
}
- _handleVisibleLine(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- e.preventDefault();
- this.cursor.moveToVisibleArea();
- }
-
_onOpenFixPreview(e: OpenFixPreviewEvent) {
this.$.applyFixDialog.open(e);
}
- _handleNextLineOrFileWithComments(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- if (
- e.detail.keyboardEvent?.shiftKey &&
- e.detail.keyboardEvent?.keyCode === 74
- ) {
- // 'J'
- this._moveToNextFileWithComment();
- return;
- }
- if (this.shortcuts.modifierPressed(e)) {
- return;
- }
-
- e.preventDefault();
+ _handleNextLine() {
this.$.diffHost.displayLine = true;
this.cursor.moveDown();
}
@@ -643,59 +604,34 @@ export class GrDiffView extends base {
);
}
- _handleNewComment(ike: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(ike)) return;
- if (this.shortcuts.modifierPressed(ike)) return;
-
- ike.preventDefault();
+ _handleNewComment() {
this.classList.remove('hideComments');
this.cursor.createCommentInPlace();
}
- _handlePrevFile(ike: IronKeyboardEvent) {
- const ke = ike.detail.keyboardEvent;
- if (this.shortcuts.shouldSuppress(ike)) return;
- // Check for meta key to avoid overriding native chrome shortcut.
- if (ke.metaKey) return;
+ _handlePrevFile() {
if (!this._path) return;
if (!this._fileList) return;
-
- ike.preventDefault();
this._navToFile(this._path, this._fileList, -1);
}
- _handleNextFile(ike: IronKeyboardEvent) {
- const ke = ike.detail.keyboardEvent;
- if (this.shortcuts.shouldSuppress(ike)) return;
- // Check for meta key to avoid overriding native chrome shortcut.
- if (ke.metaKey) return;
+ _handleNextFile() {
if (!this._path) return;
if (!this._fileList) return;
-
- ike.preventDefault();
this._navToFile(this._path, this._fileList, 1);
}
- _handleNextChunkOrCommentThread(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleNextChunk() {
+ const result = this.cursor.moveToNextChunk();
+ if (result === CursorMoveResult.CLIPPED && this.cursor.isAtEnd()) {
+ this.showToastAndNavigateFile('next', 'n');
+ }
+ }
- e.preventDefault();
- if (e.detail.keyboardEvent?.shiftKey) {
- const result = this.cursor.moveToNextCommentThread();
- if (result === CursorMoveResult.CLIPPED) {
- this._navigateToNextFileWithCommentThread();
- }
- } else {
- if (this.shortcuts.modifierPressed(e)) return;
- const result = this.cursor.moveToNextChunk();
- // navigate to next file if key is not being held down
- if (
- !e.detail.keyboardEvent?.repeat &&
- result === CursorMoveResult.CLIPPED &&
- this.cursor.isAtEnd()
- ) {
- this.showToastAndNavigateFile('next', 'n');
- }
+ _handleNextCommentThread() {
+ const result = this.cursor.moveToNextCommentThread();
+ if (result === CursorMoveResult.CLIPPED) {
+ this._navigateToNextFileWithCommentThread();
}
}
@@ -737,25 +673,19 @@ export class GrDiffView extends base {
this._navToFile(this._path, unreviewedFiles, direction === 'next' ? 1 : -1);
}
- _handlePrevChunkOrCommentThread(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
- e.preventDefault();
- if (e.detail.keyboardEvent?.shiftKey) {
- this.cursor.moveToPreviousCommentThread();
- } else {
- if (this.shortcuts.modifierPressed(e)) return;
- this.cursor.moveToPreviousChunk();
- if (!e.detail.keyboardEvent?.repeat && this.cursor.isAtStart()) {
- this.showToastAndNavigateFile('previous', 'p');
- }
+ _handlePrevChunk() {
+ this.cursor.moveToPreviousChunk();
+ if (this.cursor.isAtStart()) {
+ this.showToastAndNavigateFile('previous', 'p');
}
}
+ _handlePrevCommentThread() {
+ this.cursor.moveToPreviousCommentThread();
+ }
+
// Similar to gr-change-view._handleOpenReplyDialog
- _handleOpenReplyDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
+ _handleOpenReplyDialog() {
this._getLoggedIn().then(isLoggedIn => {
if (!isLoggedIn) {
fireEvent(this, 'show-auth-required');
@@ -763,50 +693,29 @@ export class GrDiffView extends base {
}
this.set('changeViewState.showReplyDialog', true);
- e.preventDefault();
this._navToChangeView();
});
}
- _handleToggleLeftPane(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (!e.detail.keyboardEvent?.shiftKey) return;
-
- e.preventDefault();
+ _handleToggleLeftPane() {
this.$.diffHost.toggleLeftDiff();
}
- _handleOpenDownloadDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
+ _handleOpenDownloadDialog() {
this.set('changeViewState.showDownloadDialog', true);
- e.preventDefault();
this._navToChangeView();
}
- _handleUpToChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
- e.preventDefault();
+ _handleUpToChange() {
this._navToChangeView();
}
- _handleCommaKey(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
+ _handleCommaKey() {
if (!this._loggedIn) return;
-
- e.preventDefault();
this.$.diffPreferencesDialog.open();
}
- _handleToggleDiffMode(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
- e.preventDefault();
+ _handleToggleDiffMode() {
if (this._getDiffViewMode() === DiffViewMode.SIDE_BY_SIDE) {
this.$.modeSelect.setMode(DiffViewMode.UNIFIED);
} else {
@@ -1689,28 +1598,19 @@ export class GrDiffView extends base {
this._loadBlame();
}
- _handleToggleBlame(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
+ _handleToggleBlame() {
this._toggleBlame();
}
- _handleToggleHideAllCommentThreads(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
-
+ _handleToggleHideAllCommentThreads() {
toggleClass(this, 'hideComments');
}
- _handleOpenFileList(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- if (this.shortcuts.modifierPressed(e)) return;
+ _handleOpenFileList() {
this.$.dropdown.open();
}
- _handleDiffAgainstBase(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffAgainstBase() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1726,8 +1626,7 @@ export class GrDiffView extends base {
);
}
- _handleDiffBaseAgainstLeft(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffBaseAgainstLeft() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1747,8 +1646,7 @@ export class GrDiffView extends base {
);
}
- _handleDiffAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffAgainstLatest() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1767,8 +1665,7 @@ export class GrDiffView extends base {
);
}
- _handleDiffRightAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffRightAgainstLatest() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1786,8 +1683,7 @@ export class GrDiffView extends base {
);
}
- _handleDiffBaseAgainstLatest(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleDiffBaseAgainstLatest() {
if (!this._change) return;
if (!this._path) return;
if (!this._patchRange) return;
@@ -1824,14 +1720,11 @@ export class GrDiffView extends base {
return '';
}
- _handleToggleAllDiffContext(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
-
+ _handleToggleAllDiffContext() {
this.$.diffHost.toggleAllContext();
}
- _handleNextUnreviewedFile(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _handleNextUnreviewedFile() {
this._setReviewed(true);
this.navigateToUnreviewedFile('next');
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index d45ca0e177..e4a8aa4875 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -426,7 +426,7 @@ suite('gr-diff-view tests', () => {
test('toggle left diff with a hotkey', () => {
const toggleLeftDiffStub = sinon.stub(
element.$.diffHost, 'toggleLeftDiff');
- MockInteractions.pressAndReleaseKeyOn(element, 65, 'shift', 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'A');
assert.isTrue(toggleLeftDiffStub.calledOnce);
});
@@ -498,12 +498,12 @@ suite('gr-diff-view tests', () => {
assert(scrollStub.calledOnce);
scrollStub = sinon.stub(element.cursor, 'moveToNextCommentThread');
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
assert(scrollStub.calledOnce);
scrollStub = sinon.stub(element.cursor,
'moveToPreviousCommentThread');
- MockInteractions.pressAndReleaseKeyOn(element, 80, 'shift', 'p');
+ MockInteractions.pressAndReleaseKeyOn(element, 80, null, 'P');
assert(scrollStub.calledOnce);
const computeContainerClassStub = sinon.stub(element.$.diffHost.$.diff,
@@ -516,22 +516,29 @@ suite('gr-diff-view tests', () => {
assert(computeContainerClassStub.lastCall.calledWithExactly(
false, 'SIDE_BY_SIDE', false));
+ // Note that stubbing _setReviewed means that the value of the
+ // `element.$.reviewed` checkbox is not flipped.
sinon.stub(element, '_setReviewed');
sinon.spy(element, '_handleToggleFileReviewed');
element.$.reviewed.checked = false;
- MockInteractions.keyUpOn(element, 82, 'shift', 'r');
+ assert.isFalse(element._handleToggleFileReviewed.called);
assert.isFalse(element._setReviewed.called);
+
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(element._handleToggleFileReviewed.calledOnce);
+ assert.isTrue(element._setReviewed.calledOnce);
+ assert.equal(element._setReviewed.lastCall.args[0], true);
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ // Handler is throttled, so another key press within 500 ms is ignored.
+ clock.tick(100);
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(element._handleToggleFileReviewed.calledOnce);
+ assert.isTrue(element._setReviewed.calledOnce);
clock.tick(1000);
-
- MockInteractions.keyUpOn(element, 82, null, 'r');
+ MockInteractions.pressAndReleaseKeyOn(element, 82, null, 'r');
assert.isTrue(element._handleToggleFileReviewed.calledTwice);
- assert.isTrue(element._setReviewed.called);
- assert.equal(element._setReviewed.lastCall.args[0], true);
+ assert.isTrue(element._setReviewed.calledTwice);
});
test('moveToNextCommentThread navigates to next file', () => {
@@ -563,14 +570,14 @@ suite('gr-diff-view tests', () => {
element.changeViewState.selectedFileIndex = 1;
element._loggedIn = true;
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
flush();
assert.isTrue(diffNavStub.calledWithExactly(
element._change, 'wheatley.md', 10, PARENT, 21));
element._path = 'wheatley.md'; // navigated to next file
- MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'N');
flush();
assert.isTrue(diffChangeStub.called);
@@ -578,7 +585,7 @@ suite('gr-diff-view tests', () => {
test('shift+x shortcut toggles all diff context', () => {
const toggleStub = sinon.stub(element.$.diffHost, 'toggleAllContext');
- MockInteractions.pressAndReleaseKeyOn(element, 88, 'shift', 'x');
+ MockInteractions.pressAndReleaseKeyOn(element, 88, null, 'X');
flush();
assert.isTrue(toggleStub.called);
});
@@ -691,7 +698,7 @@ suite('gr-diff-view tests', () => {
sinon.stub(element, '_getLoggedIn').returns(Promise.resolve(false));
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- MockInteractions.keyUpOn(element, 65, null, 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isTrue(changeNavStub.notCalled, 'The `a` keyboard shortcut ' +
'should only work when the user is logged in.');
@@ -717,7 +724,7 @@ suite('gr-diff-view tests', () => {
sinon.stub(element, '_getLoggedIn').returns(Promise.resolve(true));
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- MockInteractions.keyUpOn(element, 65, null, 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isTrue(element.changeViewState.showReplyDialog);
assert(changeNavStub.lastCall.calledWithExactly(element._change, 10,
@@ -743,7 +750,7 @@ suite('gr-diff-view tests', () => {
sinon.stub(element, '_getLoggedIn').returns(Promise.resolve(true));
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- MockInteractions.keyUpOn(element, 65, null, 'a');
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
await flush();
assert.isTrue(element.changeViewState.showReplyDialog);
assert(changeNavStub.lastCall.calledWithExactly(element._change, 1,
@@ -807,7 +814,7 @@ suite('gr-diff-view tests', () => {
'Should navigate to /c/42/5..10');
assert.isUndefined(element.changeViewState.showDownloadDialog);
- MockInteractions.keyUpOn(element, 68, null, 'd');
+ MockInteractions.pressAndReleaseKeyOn(element, 68, null, 'd');
assert.isTrue(element.changeViewState.showDownloadDialog);
});
@@ -1730,7 +1737,7 @@ suite('gr-diff-view tests', () => {
test('toggle blame with shortcut', () => {
const toggleBlame = sinon.stub(
element.$.diffHost, 'loadBlame').callsFake(() => Promise.resolve());
- MockInteractions.keyUpOn(element, 66, null, 'b');
+ MockInteractions.pressAndReleaseKeyOn(element, 66, null, 'b');
assert.isTrue(toggleBlame.calledOnce);
});
});
@@ -1890,7 +1897,7 @@ suite('gr-diff-view tests', () => {
element._path = 'file1';
const reviewedStub = sinon.stub(element, '_setReviewed');
const navStub = sinon.stub(element, '_navToFile');
- MockInteractions.pressAndReleaseKeyOn(element, 77, 'shift', 'm');
+ MockInteractions.pressAndReleaseKeyOn(element, 77, null, 'M');
flush();
assert.isTrue(reviewedStub.lastCall.args[0]);
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
index 8821725f1f..551889f660 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
@@ -47,9 +47,6 @@ export class GrSelectionActionBox extends PolymerElement {
* @event create-comment-requested
*/
- @property({type: Object})
- keyEventTarget = document.body;
-
@property({type: Boolean})
positionBelow = false;
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index 38f55bdd48..d88eeaf49f 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -43,6 +43,7 @@ import {getBaseUrl} from '../utils/url-util';
import {
KeyboardShortcutMixin,
Shortcut,
+ ShortcutListener,
} from '../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {GerritNav} from './core/gr-navigation/gr-navigation';
import {appContext} from '../services/app-context';
@@ -68,7 +69,6 @@ import {
import {GrMainHeader} from './core/gr-main-header/gr-main-header';
import {GrSettingsView} from './settings/gr-settings-view/gr-settings-view';
import {
- IronKeyboardEvent,
DialogChangeEventDetail,
EventType,
LocationChangeEvent,
@@ -81,6 +81,7 @@ import {GerritView} from '../services/router/router-model';
import {LifeCycle} from '../constants/reporting';
import {fireIronAnnounce} from '../utils/event-util';
import {assertIsDefined} from '../utils/common-util';
+import {listen} from '../services/shortcuts/shortcuts-service';
interface ErrorInfo {
text: string;
@@ -120,9 +121,6 @@ export class GrAppElement extends base {
@property({type: Object})
params?: AppElementParams;
- @property({type: Object})
- keyEventTarget = document.body;
-
@property({type: Object, observer: '_accountChanged'})
_account?: AccountDetailInfo;
@@ -214,17 +212,19 @@ export class GrAppElement extends base {
private readonly restApiService = appContext.restApiService;
- private readonly shortcuts = appContext.shortcutsService;
-
- override keyboardShortcuts() {
- return {
- [Shortcut.OPEN_SHORTCUT_HELP_DIALOG]: '_showKeyboardShortcuts',
- [Shortcut.GO_TO_USER_DASHBOARD]: '_goToUserDashboard',
- [Shortcut.GO_TO_OPENED_CHANGES]: '_goToOpenedChanges',
- [Shortcut.GO_TO_MERGED_CHANGES]: '_goToMergedChanges',
- [Shortcut.GO_TO_ABANDONED_CHANGES]: '_goToAbandonedChanges',
- [Shortcut.GO_TO_WATCHED_CHANGES]: '_goToWatchedChanges',
- };
+ override keyboardShortcuts(): ShortcutListener[] {
+ return [
+ listen(Shortcut.OPEN_SHORTCUT_HELP_DIALOG, _ =>
+ this._showKeyboardShortcuts()
+ ),
+ listen(Shortcut.GO_TO_USER_DASHBOARD, _ => this._goToUserDashboard()),
+ listen(Shortcut.GO_TO_OPENED_CHANGES, _ => this._goToOpenedChanges()),
+ listen(Shortcut.GO_TO_MERGED_CHANGES, _ => this._goToMergedChanges()),
+ listen(Shortcut.GO_TO_ABANDONED_CHANGES, _ =>
+ this._goToAbandonedChanges()
+ ),
+ listen(Shortcut.GO_TO_WATCHED_CHANGES, _ => this._goToWatchedChanges()),
+ ];
}
constructor() {
@@ -502,8 +502,7 @@ export class GrAppElement extends base {
(this.shadowRoot!.querySelector('#keyboardShortcuts') as GrOverlay).open();
}
- _showKeyboardShortcuts(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
+ _showKeyboardShortcuts() {
// same shortcut should close the dialog if pressed again
// when dialog is open
this.loadKeyboardShortcutsDialog = true;
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
index afb695f8ef..e0d1d15929 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
+++ b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.ts
@@ -23,13 +23,13 @@ import {PolymerElement} from '@polymer/polymer/polymer-element';
import {customElement, property} from '@polymer/decorators';
import {htmlTemplate} from './gr-editable-label_html';
import {IronDropdownElement} from '@polymer/iron-dropdown/iron-dropdown';
-import {dom, EventApi} from '@polymer/polymer/lib/legacy/polymer.dom';
import {PaperInputElementExt} from '../../../types/types';
import {
AutocompleteQuery,
GrAutocomplete,
} from '../gr-autocomplete/gr-autocomplete';
import {addShortcut, Key} from '../../../utils/dom-util';
+import {queryAndAssert} from '../../../utils/common-util';
const AWAIT_MAX_ITERS = 10;
const AWAIT_STEP = 5;
@@ -212,18 +212,24 @@ export class GrEditableLabel extends PolymerElement {
this.getGrAutocomplete()) as HTMLInputElement;
}
- _handleEnter(e: KeyboardEvent) {
- const target = (dom(e) as EventApi).rootTarget;
- if (target === this._nativeInput) {
- e.preventDefault();
+ _handleEnter(event: KeyboardEvent) {
+ const inputContainer = queryAndAssert(this, '.inputContainer');
+ const isEventFromInput = event
+ .composedPath()
+ .some(element => element === inputContainer);
+ if (isEventFromInput) {
+ event.preventDefault();
this._save();
}
}
- _handleEsc(e: KeyboardEvent) {
- const target = (dom(e) as EventApi).rootTarget;
- if (target === this._nativeInput) {
- e.preventDefault();
+ _handleEsc(event: KeyboardEvent) {
+ const inputContainer = queryAndAssert(this, '.inputContainer');
+ const isEventFromInput = event
+ .composedPath()
+ .some(element => element === inputContainer);
+ if (isEventFromInput) {
+ event.preventDefault();
this._cancel();
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
index 337d59515c..9e6b42a7d9 100644
--- a/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
+++ b/polygerrit-ui/app/elements/shared/gr-textarea/gr-textarea.ts
@@ -17,7 +17,6 @@
import '../gr-autocomplete-dropdown/gr-autocomplete-dropdown';
import '../gr-cursor-manager/gr-cursor-manager';
import '../gr-overlay/gr-overlay';
-import '@polymer/iron-a11y-keys-behavior/iron-a11y-keys-behavior';
import '@polymer/iron-autogrow-textarea/iron-autogrow-textarea';
import '../../../styles/shared-styles';
import {flush} from '@polymer/polymer/lib/legacy/polymer.dom';
diff --git a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
index 02cfa9f1d5..f133c116df 100644
--- a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
+++ b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.ts
@@ -14,12 +14,9 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import {IronA11yKeysBehavior} from '@polymer/iron-a11y-keys-behavior/iron-a11y-keys-behavior';
-import {mixinBehaviors} from '@polymer/polymer/lib/legacy/class';
import {property} from '@polymer/decorators';
import {PolymerElement} from '@polymer/polymer';
import {check, Constructor} from '../../utils/common-util';
-import {IronKeyboardEvent} from '../../types/events';
import {appContext} from '../../services/app-context';
import {
Shortcut,
@@ -27,7 +24,6 @@ import {
SPECIAL_SHORTCUT,
} from '../../services/shortcuts/shortcuts-config';
import {
- ComboKey,
SectionView,
ShortcutListener,
} from '../../services/shortcuts/shortcuts-service';
@@ -40,18 +36,7 @@ export {
SectionView,
};
-interface IronA11yKeysMixinConstructor {
- // Note: this is needed to have same interface as other mixins
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- new (...args: any[]): IronA11yKeysBehavior;
-}
-/**
- * @polymer
- * @mixinFunction
- */
-const InternalKeyboardShortcutMixin = <
- T extends Constructor<PolymerElement> & IronA11yKeysMixinConstructor
->(
+export const KeyboardShortcutMixin = <T extends Constructor<PolymerElement>>(
superClass: T
) => {
/**
@@ -59,14 +44,10 @@ const InternalKeyboardShortcutMixin = <
* @mixinClass
*/
class Mixin extends superClass {
- @property({type: Object})
- _shortcut_go_table: Map<string, string> = new Map<string, string>();
-
- @property({type: Object})
- _shortcut_v_table: Map<string, string> = new Map<string, string>();
-
+ // This enables `Shortcut` to be used in the html template.
Shortcut = Shortcut;
+ // This enables `ShortcutSection` to be used in the html template.
ShortcutSection = ShortcutSection;
private readonly shortcuts = appContext.shortcutsService;
@@ -88,30 +69,6 @@ const InternalKeyboardShortcutMixin = <
/** Are shortcuts currently enabled? True only when element is visible. */
private bindingsEnabled = false;
- _addOwnKeyBindings(shortcut: Shortcut, handler: string) {
- const bindings = this.shortcuts.getBindingsForShortcut(shortcut);
- if (!bindings) {
- return;
- }
- if (bindings[0] === SPECIAL_SHORTCUT.DOC_ONLY) {
- return;
- }
- if (bindings[0] === SPECIAL_SHORTCUT.GO_KEY) {
- bindings
- .slice(1)
- .forEach(binding => this._shortcut_go_table.set(binding, handler));
- } else if (bindings[0] === SPECIAL_SHORTCUT.V_KEY) {
- // for each binding added with the go/v key, we set the handler to be
- // handleVKeyAction. handleVKeyAction then looks up in th
- // shortcut_table to see what the relevant handler should be
- bindings
- .slice(1)
- .forEach(binding => this._shortcut_v_table.set(binding, handler));
- } else {
- this.addOwnKeyBinding(bindings.join(' '), handler);
- }
- }
-
override connectedCallback() {
super.connectedCallback();
this.createVisibilityObserver();
@@ -157,28 +114,7 @@ const InternalKeyboardShortcutMixin = <
if (this.bindingsEnabled) return;
this.bindingsEnabled = true;
- const shortcuts = new Map<string, string>(
- Object.entries(this.keyboardShortcuts())
- );
- this.shortcuts.attachHost(this, shortcuts);
-
- for (const [key, value] of shortcuts.entries()) {
- this._addOwnKeyBindings(key as Shortcut, value);
- }
-
- // If any of the shortcuts utilized GO_KEY, then they are handled
- // directly by this behavior.
- if (this._shortcut_go_table.size > 0) {
- this._shortcut_go_table.forEach((_, key) => {
- this.addOwnKeyBinding(key, '_handleGoAction');
- });
- }
-
- if (this._shortcut_v_table.size > 0) {
- this._shortcut_v_table.forEach((_, key) => {
- this.addOwnKeyBinding(key, '_handleVAction');
- });
- }
+ this.shortcuts.attachHost(this, this.keyboardShortcuts());
}
/**
@@ -189,76 +125,22 @@ const InternalKeyboardShortcutMixin = <
private disableBindings() {
if (!this.bindingsEnabled) return;
this.bindingsEnabled = false;
- if (this.shortcuts.detachHost(this)) {
- this.removeOwnKeyBindings();
- }
+ this.shortcuts.detachHost(this);
}
private hasKeyboardShortcuts() {
- return Object.entries(this.keyboardShortcuts()).length > 0;
- }
-
- keyboardShortcuts() {
- return {};
+ return this.keyboardShortcuts().length > 0;
}
- _handleVAction(e: IronKeyboardEvent) {
- if (
- !this.shortcuts.isInSpecificComboKeyMode(ComboKey.V) ||
- !this._shortcut_v_table.has(e.detail.key) ||
- this.shortcuts.shouldSuppress(e)
- ) {
- return;
- }
- e.preventDefault();
- const handler = this._shortcut_v_table.get(e.detail.key);
- if (handler) {
- // TODO(TS): should fix this
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- (this as any)[handler](e);
- }
- }
-
- _handleGoAction(e: IronKeyboardEvent) {
- if (
- !this.shortcuts.isInSpecificComboKeyMode(ComboKey.G) ||
- !this._shortcut_go_table.has(e.detail.key) ||
- this.shortcuts.shouldSuppress(e)
- ) {
- return;
- }
- e.preventDefault();
- const handler = this._shortcut_go_table.get(e.detail.key);
- if (handler) {
- // TODO(TS): should fix this
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- (this as any)[handler](e);
- }
+ keyboardShortcuts(): ShortcutListener[] {
+ return [];
}
}
return Mixin as T & Constructor<KeyboardShortcutMixinInterface>;
};
-// The following doesn't work (IronA11yKeysBehavior crashes):
-// const KeyboardShortcutMixin = superClass => {
-// class Mixin extends mixinBehaviors([IronA11yKeysBehavior], superClass) {
-// ...
-// }
-// return Mixin;
-// }
-// This is a workaround
-export const KeyboardShortcutMixin = <T extends Constructor<PolymerElement>>(
- superClass: T
-): T & Constructor<KeyboardShortcutMixinInterface> =>
- InternalKeyboardShortcutMixin(
- // TODO(TS): mixinBehaviors in some lib is returning: `new () => T` instead
- // which will fail the type check due to missing IronA11yKeysBehavior interface
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- mixinBehaviors([IronA11yKeysBehavior], superClass) as any
- );
-
/** The interface corresponding to KeyboardShortcutMixin */
export interface KeyboardShortcutMixinInterface {
- keyboardShortcuts(): {[key: string]: string | null};
+ keyboardShortcuts(): ShortcutListener[];
}
diff --git a/polygerrit-ui/app/package.json b/polygerrit-ui/app/package.json
index 2ad4e79365..281a1eb652 100644
--- a/polygerrit-ui/app/package.json
+++ b/polygerrit-ui/app/package.json
@@ -6,7 +6,6 @@
"@polymer/decorators": "^3.0.0",
"@polymer/font-roboto-local": "^3.0.2",
"@polymer/iron-a11y-announcer": "^3.1.0",
- "@polymer/iron-a11y-keys-behavior": "^3.0.1",
"@polymer/iron-autogrow-textarea": "^3.0.3",
"@polymer/iron-dropdown": "^3.0.1",
"@polymer/iron-fit-behavior": "^3.1.0",
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-config.ts
index bd004d7cd3..3c9e0587d0 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 enum Shortcut {
export interface ShortcutHelpItem {
shortcut: Shortcut;
text: string;
- bindings: string[];
+ bindings: Binding[];
}
export const config = new Map<ShortcutSection, ShortcutHelpItem[]>();
@@ -124,8 +126,8 @@ function describe(
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 @@ function describe(
}
}
-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'
-);
-describe(
- Shortcut.EDIT_TOPIC,
- ShortcutSection.ACTIONS,
- 'Add a change topic',
- 't'
+ {key: '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',
- '.'
-);
-describe(
- Shortcut.NEXT_CHUNK,
- ShortcutSection.DIFFS,
- 'Go to next diff chunk',
- 'n'
+ {key: '.'}
);
+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'
-);
-describe(
- Shortcut.LEFT_PANE,
- ShortcutSection.DIFFS,
- 'Select left pane',
- 'shift+left'
-);
-describe(
- Shortcut.RIGHT_PANE,
- ShortcutSection.DIFFS,
- 'Select right pane',
- 'shift+right'
+ {key: 'h'}
);
+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'
-);
-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',
- ']'
-);
+ {key: 'M'}
+);
+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'
-);
-describe(
- Shortcut.UP_TO_CHANGE,
- ShortcutSection.NAVIGATION,
- 'Up to change',
- 'u'
+ {key: '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 edc31a4f07..19f12c5475 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -19,27 +19,39 @@ import {
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 @@ export class ShortcutsService {
* 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 @@ export class ShortcutsService {
}
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 @@ export class ShortcutsService {
);
}
- 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 @@ export class ShortcutsService {
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 @@ export class ShortcutsService {
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 @@ export class ShortcutsService {
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 @@ export class ShortcutsService {
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;
- }
+ notifyViewListeners() {
+ const view = this.directoryView();
+ this.listeners.forEach(listener => listener(view));
}
+}
- describeBinding(binding: string) {
- // single key bindings
- if (binding.length === 1) {
- return [binding];
- }
- return binding
- .split(':')[0]
- .split('+')
- .map(part => this._describeKey(part));
+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;
}
+}
- notifyListeners() {
- const view = this.directoryView();
- this.listeners.forEach(listener => listener(view));
+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 d8aa11e8ea..05c4f538df 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 @@ suite('shortcuts-service tests', () => {
});
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 @@ suite('shortcuts-service tests', () => {
}
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 @@ suite('shortcuts-service tests', () => {
);
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 @@ suite('shortcuts-service tests', () => {
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 @@ suite('shortcuts-service tests', () => {
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'},
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index c78f61a8aa..b6376c4645 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -241,30 +241,3 @@ export interface TitleChangeEventDetail {
title: string;
}
export type TitleChangeEvent = CustomEvent<TitleChangeEventDetail>;
-
-/**
- * Keyboard events emitted from elements using IronA11yKeysBehavior: That means
- * that the element returns a list of handlers from either `keyBindings()` or
- * from `keyboardShortcuts()`. This event should not be used in Lit elements
- * and will be obsolete once the Lit migration is completed.
- */
-export interface IronKeyboardEvent extends CustomEvent {
- detail: IronKeyboardEventDetail;
-}
-
-export interface IronKeyboardEventDetail {
- keyboardEvent: KeyboardEvent;
- key: string;
- combo?: string;
-}
-
-export function isIronKeyboardEvent(
- e: IronKeyboardEvent | Event | CustomEvent
-): e is IronKeyboardEvent {
- const ike = e as IronKeyboardEvent;
- return !!ike?.detail?.keyboardEvent;
-}
-
-export interface IronKeyboardEventListener {
- (evt: IronKeyboardEvent): void;
-}
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts
index a7ad6e1119..b6dece03ff 100644
--- a/polygerrit-ui/app/utils/dom-util.ts
+++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -16,7 +16,6 @@
*/
import {EventApi} from '@polymer/polymer/lib/legacy/polymer.dom';
import {check} from './common-util';
-import {IronKeyboardEvent} from '../types/events';
/**
* Event emitted from polymer elements.
@@ -326,8 +325,18 @@ export enum Modifier {
SHIFT_KEY,
}
-export interface Shortcut {
+export enum ComboKey {
+ G = 'g',
+ V = 'v',
+}
+
+export interface Binding {
key: string | Key;
+ /** Defaults to false. */
+ docOnly?: boolean;
+ /** Defaults to not being a combo shortcut. */
+ combo?: ComboKey;
+ /** Defaults to no modifiers. */
modifiers?: Modifier[];
}
@@ -358,7 +367,7 @@ function altMustMatch(key: string | Key) {
export function eventMatchesShortcut(
e: KeyboardEvent,
- shortcut: Shortcut
+ shortcut: Binding
): boolean {
if (e.key !== shortcut.key) return false;
const modifiers = shortcut.modifiers ?? [];
@@ -380,7 +389,7 @@ export function eventMatchesShortcut(
}
export function addGlobalShortcut(
- shortcut: Shortcut,
+ shortcut: Binding,
listener: (e: KeyboardEvent) => void
) {
return addShortcut(document.body, shortcut, listener);
@@ -388,12 +397,14 @@ export function addGlobalShortcut(
export function addShortcut(
element: HTMLElement,
- shortcut: Shortcut,
+ shortcut: Binding,
listener: (e: KeyboardEvent) => void
) {
const wrappedListener = (e: KeyboardEvent) => {
if (e.repeat) return;
- if (eventMatchesShortcut(e, shortcut)) listener(e);
+ if (eventMatchesShortcut(e, shortcut)) {
+ listener(e);
+ }
};
element.addEventListener('keydown', wrappedListener);
return () => element.removeEventListener('keydown', wrappedListener);
@@ -406,11 +417,3 @@ export function modifierPressed(e: KeyboardEvent) {
export function shiftPressed(e: KeyboardEvent) {
return e.shiftKey;
}
-
-export function isModifierPressed(e: IronKeyboardEvent) {
- return modifierPressed(e.detail.keyboardEvent);
-}
-
-export function isShiftPressed(e: IronKeyboardEvent) {
- return shiftPressed(e.detail.keyboardEvent);
-}
diff --git a/polygerrit-ui/app/yarn.lock b/polygerrit-ui/app/yarn.lock
index bfca566952..d3b22ebe52 100644
--- a/polygerrit-ui/app/yarn.lock
+++ b/polygerrit-ui/app/yarn.lock
@@ -46,7 +46,7 @@
dependencies:
"@polymer/polymer" "^3.0.0"
-"@polymer/iron-a11y-keys-behavior@^3.0.0-pre.26", "@polymer/iron-a11y-keys-behavior@^3.0.1":
+"@polymer/iron-a11y-keys-behavior@^3.0.0-pre.26":
version "3.0.1"
resolved "https://registry.yarnpkg.com/@polymer/iron-a11y-keys-behavior/-/iron-a11y-keys-behavior-3.0.1.tgz#2868ea72912d2007ffab4734684a91f5aac49b84"
integrity sha512-lnrjKq3ysbBPT/74l0Fj0U9H9C35Tpw2C/tpJ8a+5g8Y3YJs1WSZYnEl1yOkw6sEyaxOq/1DkzH0+60gGu5/PQ==