Merge changes I1fba88e7,I5202c808,I96fb1e3a
* changes:
Add addShortcut() utility as a replacement for keyBindings()
Move modifierPressed() method from mixin to service
Move combo key handling from mixin into service
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 ac44908..a3db699 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
@@ -407,7 +407,7 @@
}
_nextChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -419,7 +419,7 @@
}
_prevChange(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -431,7 +431,7 @@
}
_openChange(e: IronKeyboardEvent) {
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
this.openChange(e.detail.keyboardEvent);
}
@@ -445,7 +445,7 @@
_nextPage(e: IronKeyboardEvent) {
if (
this.shortcuts.shouldSuppress(e) ||
- (this.modifierPressed(e) && !isShiftPressed(e))
+ (this.shortcuts.modifierPressed(e) && !isShiftPressed(e))
) {
return;
}
@@ -457,7 +457,7 @@
_prevPage(e: IronKeyboardEvent) {
if (
this.shortcuts.shouldSuppress(e) ||
- (this.modifierPressed(e) && !isShiftPressed(e))
+ (this.shortcuts.modifierPressed(e) && !isShiftPressed(e))
) {
return;
}
@@ -472,7 +472,7 @@
}
_toggleChangeReviewed(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -500,7 +500,7 @@
}
_toggleChangeStar(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
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 fc2cbe5..f8bdbef 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
@@ -752,7 +752,7 @@
}
_handleToggleDiffMode(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -1496,7 +1496,7 @@
}
_handleOpenReplyDialog(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
this._getLoggedIn().then(isLoggedIn => {
@@ -1511,7 +1511,7 @@
}
_handleOpenDownloadDialogShortcut(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -1520,7 +1520,7 @@
}
_handleEditTopic(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -1672,7 +1672,7 @@
}
_handleToggleChangeStar(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
e.preventDefault();
@@ -1680,7 +1680,7 @@
}
_handleUpToDashboard(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -1689,7 +1689,7 @@
}
_handleExpandAllMessages(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -1700,7 +1700,7 @@
}
_handleCollapseAllMessages(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -1711,7 +1711,7 @@
}
_handleOpenDiffPrefsShortcut(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
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 b78c78f..74c079b 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
@@ -902,7 +902,7 @@
_handleToggleInlineDiff(e: IronKeyboardEvent) {
if (
this.shortcuts.shouldSuppress(e) ||
- this.modifierPressed(e) ||
+ this.shortcuts.modifierPressed(e) ||
e.detail?.keyboardEvent?.repeat ||
this.fileCursor.index === -1
) {
@@ -923,7 +923,7 @@
}
_handleToggleHideAllCommentThreads(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -932,7 +932,7 @@
}
_handleCursorNext(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -952,7 +952,7 @@
}
_handleCursorPrev(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -972,7 +972,7 @@
}
_handleNewComment(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
e.preventDefault();
@@ -1001,7 +1001,7 @@
}
_handleOpenFile(e: IronKeyboardEvent) {
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
this.handleOpenFile(e.detail.keyboardEvent);
}
@@ -1022,7 +1022,7 @@
_handleNextChunk(e: IronKeyboardEvent) {
if (
this.shortcuts.shouldSuppress(e) ||
- (this.modifierPressed(e) && !isShiftPressed(e)) ||
+ (this.shortcuts.modifierPressed(e) && !isShiftPressed(e)) ||
this._noDiffsExpanded()
) {
return;
@@ -1039,7 +1039,7 @@
_handlePrevChunk(e: IronKeyboardEvent) {
if (
this.shortcuts.shouldSuppress(e) ||
- (this.modifierPressed(e) && !isShiftPressed(e)) ||
+ (this.shortcuts.modifierPressed(e) && !isShiftPressed(e)) ||
this._noDiffsExpanded()
) {
return;
@@ -1054,7 +1054,7 @@
}
_handleToggleFileReviewed(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
@@ -1543,7 +1543,7 @@
}
_handleEscKey(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e) || this.modifierPressed(e)) {
+ if (this.shortcuts.shouldSuppress(e) || this.shortcuts.modifierPressed(e)) {
return;
}
e.preventDefault();
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 f4064da..062d6a2 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
@@ -597,7 +597,6 @@
let interact;
setup(() => {
- sinon.stub(element, 'modifierPressed').returns(false);
const openCursorStub = sinon.stub(element, '_openCursorFile');
const openSelectedStub = sinon.stub(element, '_openSelectedFile');
const expandStub = sinon.stub(element, '_toggleFileExpanded');
@@ -1708,12 +1707,15 @@
});
test('_displayLine', () => {
- sinon.stub(element, 'modifierPressed')
- .callsFake(() => false);
element.filesExpanded = FilesExpandedState.ALL;
const mockEvent = {
preventDefault() {},
composedPath() { return []; },
+ detail: {
+ keyboardEvent: {
+ composedPath() { return []; },
+ },
+ },
};
element._displayLine = false;
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 aaeb924..dd1ebd1 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
@@ -400,7 +400,7 @@
const keyboardEvent = e.detail.keyboardEvent;
if (
this.shortcuts.shouldSuppress(e) ||
- (this.modifierPressed(e) && !keyboardEvent.shiftKey)
+ (this.shortcuts.modifierPressed(e) && !keyboardEvent.shiftKey)
) {
return;
}
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 c7a462e..295e41f 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
@@ -534,7 +534,7 @@
_handleToggleFileReviewed(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
e.preventDefault();
this._setReviewed(!this.$.reviewed.checked);
@@ -542,7 +542,7 @@
_handleEscKey(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
e.preventDefault();
this.$.diffHost.displayLine = false;
@@ -573,7 +573,7 @@
this._moveToPreviousFileWithComment();
return;
}
- if (this.modifierPressed(e)) {
+ if (this.shortcuts.modifierPressed(e)) {
return;
}
@@ -604,7 +604,7 @@
this._moveToNextFileWithComment();
return;
}
- if (this.modifierPressed(e)) {
+ if (this.shortcuts.modifierPressed(e)) {
return;
}
@@ -654,7 +654,7 @@
_handleNewComment(ike: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(ike)) return;
- if (this.modifierPressed(ike)) return;
+ if (this.shortcuts.modifierPressed(ike)) return;
ike.preventDefault();
this.classList.remove('hideComments');
@@ -695,7 +695,7 @@
this._navigateToNextFileWithCommentThread();
}
} else {
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
const result = this.cursor.moveToNextChunk();
// navigate to next file if key is not being held down
if (
@@ -753,7 +753,7 @@
if (e.detail.keyboardEvent?.shiftKey) {
this.cursor.moveToPreviousCommentThread();
} else {
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
this.cursor.moveToPreviousChunk();
if (!e.detail.keyboardEvent?.repeat && this.cursor.isAtStart()) {
this.showToastAndNavigateFile('previous', 'p');
@@ -764,7 +764,7 @@
// Similar to gr-change-view._handleOpenReplyDialog
_handleOpenReplyDialog(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
this._getLoggedIn().then(isLoggedIn => {
if (!isLoggedIn) {
fireEvent(this, 'show-auth-required');
@@ -787,7 +787,7 @@
_handleOpenDownloadDialog(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
this.set('changeViewState.showDownloadDialog', true);
e.preventDefault();
@@ -796,7 +796,7 @@
_handleUpToChange(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
e.preventDefault();
this._navToChangeView();
@@ -804,7 +804,7 @@
_handleCommaKey(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
if (this._diffPrefsDisabled) return;
e.preventDefault();
@@ -813,7 +813,7 @@
_handleToggleDiffMode(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
e.preventDefault();
if (this._getDiffViewMode() === DiffViewMode.SIDE_BY_SIDE) {
@@ -1703,21 +1703,21 @@
_handleToggleBlame(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
this._toggleBlame();
}
_handleToggleHideAllCommentThreads(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
toggleClass(this, 'hideComments');
}
_handleOpenFileList(e: IronKeyboardEvent) {
if (this.shortcuts.shouldSuppress(e)) return;
- if (this.modifierPressed(e)) return;
+ if (this.shortcuts.modifierPressed(e)) return;
this.$.dropdown.open();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index f1b74ac..e7417f4 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -21,17 +21,16 @@
import '../gr-copy-clipboard/gr-copy-clipboard';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-comment-thread_html';
-import {KeyboardShortcutMixin} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin';
import {
computeDiffFromContext,
computeId,
+ DraftInfo,
isDraft,
isRobot,
sortComments,
UIComment,
UIDraft,
UIRobot,
- DraftInfo,
} from '../../../utils/comment-util';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
import {appContext} from '../../../services/app-context';
@@ -54,15 +53,14 @@
} from '../../../types/common';
import {GrComment} from '../gr-comment/gr-comment';
import {PolymerDeepPropertyChange} from '@polymer/polymer/interfaces';
-import {IronKeyboardEvent} from '../../../types/events';
-import {LineNumber, FILE} from '../../diff/gr-diff/gr-diff-line';
+import {FILE, LineNumber} from '../../diff/gr-diff/gr-diff-line';
import {GrButton} from '../gr-button/gr-button';
import {KnownExperimentId} from '../../../services/flags/flags';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
import {DiffLayer, RenderPreferences} from '../../../api/diff';
import {
- check,
assertIsDefined,
+ check,
queryAndAssert,
} from '../../../utils/common-util';
import {fireAlert, waitForEventOnce} from '../../../utils/event-util';
@@ -72,6 +70,7 @@
import {anyLineTooLong} from '../../diff/gr-diff/gr-diff-utils';
import {getUserName} from '../../../utils/display-name-util';
import {generateAbsoluteUrl} from '../../../utils/url-util';
+import {addShortcut} from '../../../utils/dom-util';
const UNRESOLVED_EXPAND_COUNT = 5;
const NEWLINE_PATTERN = /\n/g;
@@ -83,11 +82,8 @@
};
}
-// This avoids JSC_DYNAMIC_EXTENDS_WITHOUT_JSDOC closure compiler error.
-const base = KeyboardShortcutMixin(PolymerElement);
-
@customElement('gr-comment-thread')
-export class GrCommentThread extends base {
+export class GrCommentThread extends PolymerElement {
// KeyboardShortcutMixin Not used in this element rather other elements tests
static get template() {
@@ -123,9 +119,6 @@
@property({type: Object, reflectToAttribute: true})
range?: CommentRange;
- @property({type: Object})
- keyEventTarget: HTMLElement = document.body;
-
@property({type: String, reflectToAttribute: true})
diffSide?: Side;
@@ -210,11 +203,8 @@
@property({type: Array})
layers: DiffLayer[] = [];
- get keyBindings() {
- return {
- 'e shift+e': '_handleEKey',
- };
- }
+ /** Called in disconnectedCallback. */
+ private cleanups: (() => void)[] = [];
private readonly reporting = appContext.reportingService;
@@ -240,8 +230,20 @@
});
}
+ override disconnectedCallback() {
+ super.disconnectedCallback();
+ for (const cleanup of this.cleanups) cleanup();
+ this.cleanups = [];
+ }
+
override connectedCallback() {
super.connectedCallback();
+ this.cleanups.push(
+ addShortcut({key: 'e'}, e => this.handleExpandShortcut(e))
+ );
+ this.cleanups.push(
+ addShortcut({key: 'E'}, e => this.handleCollapseShortcut(e))
+ );
this._getLoggedIn().then(loggedIn => {
this._showActions = loggedIn;
});
@@ -499,21 +501,14 @@
return this._orderedComments[this._orderedComments.length - 1] || {};
}
- _handleEKey(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) {
- return;
- }
+ private handleExpandShortcut(e: KeyboardEvent) {
+ if (this.shortcuts.shouldSuppress(e)) return;
+ this._expandCollapseComments(false);
+ }
- // Don’t preventDefault in this case because it will render the event
- // useless for other handlers (other gr-comment-thread elements).
- if (e.detail.keyboardEvent?.shiftKey) {
- this._expandCollapseComments(true);
- } else {
- if (this.modifierPressed(e)) {
- return;
- }
- this._expandCollapseComments(false);
- }
+ private handleCollapseShortcut(e: KeyboardEvent) {
+ if (this.shortcuts.shouldSuppress(e)) return;
+ this._expandCollapseComments(true);
}
_expandCollapseComments(actionIsCollapse: boolean) {
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index 06d25b3..595de34 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -673,7 +673,7 @@
pressAndReleaseKeyOn(element, 69, null, 'e');
assert.isTrue(expandCollapseStub.lastCall.calledWith(false));
- pressAndReleaseKeyOn(element, 69, 'shift', 'e');
+ pressAndReleaseKeyOn(element, 69, 'shift', 'E');
assert.isTrue(expandCollapseStub.lastCall.calledWith(true));
});
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 3d1e120..02cfa9f 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
@@ -19,7 +19,6 @@
import {property} from '@polymer/decorators';
import {PolymerElement} from '@polymer/polymer';
import {check, Constructor} from '../../utils/common-util';
-import {isModifierPressed} from '../../utils/dom-util';
import {IronKeyboardEvent} from '../../types/events';
import {appContext} from '../../services/app-context';
import {
@@ -28,8 +27,9 @@
SPECIAL_SHORTCUT,
} from '../../services/shortcuts/shortcuts-config';
import {
- ShortcutListener,
+ ComboKey,
SectionView,
+ ShortcutListener,
} from '../../services/shortcuts/shortcuts-service';
export {
@@ -40,12 +40,6 @@
SectionView,
};
-// The maximum age of a keydown event to be used in a jump navigation. This
-// is only for cases when the keyup event is lost.
-const GO_KEY_TIMEOUT_MS = 1000;
-
-const V_KEY_TIMEOUT_MS = 1000;
-
interface IronA11yKeysMixinConstructor {
// Note: this is needed to have same interface as other mixins
// eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -65,12 +59,6 @@
* @mixinClass
*/
class Mixin extends superClass {
- @property({type: Number})
- _shortcut_go_key_last_pressed: number | null = null;
-
- @property({type: Number})
- _shortcut_v_key_last_pressed: number | null = null;
-
@property({type: Object})
_shortcut_go_table: Map<string, string> = new Map<string, string>();
@@ -100,16 +88,6 @@
/** Are shortcuts currently enabled? True only when element is visible. */
private bindingsEnabled = false;
- modifierPressed(e: IronKeyboardEvent) {
- /* We are checking for g/v as modifiers pressed. There are cases such as
- * pressing v and then /, where we want the handler for / to be triggered.
- * TODO(dhruvsri): find a way to support that keyboard combination
- */
- return (
- isModifierPressed(e) || !!this._inGoKeyMode() || !!this.inVKeyMode()
- );
- }
-
_addOwnKeyBindings(shortcut: Shortcut, handler: string) {
const bindings = this.shortcuts.getBindingsForShortcut(shortcut);
if (!bindings) {
@@ -188,11 +166,6 @@
this._addOwnKeyBindings(key as Shortcut, value);
}
- // each component that uses this behaviour must be aware if go key is
- // pressed or not, since it needs to check it as a modifier
- this.addOwnKeyBinding('g:keydown', '_handleGoKeyDown');
- this.addOwnKeyBinding('g:keyup', '_handleGoKeyUp');
-
// If any of the shortcuts utilized GO_KEY, then they are handled
// directly by this behavior.
if (this._shortcut_go_table.size > 0) {
@@ -201,8 +174,6 @@
});
}
- this.addOwnKeyBinding('v:keydown', '_handleVKeyDown');
- this.addOwnKeyBinding('v:keyup', '_handleVKeyUp');
if (this._shortcut_v_table.size > 0) {
this._shortcut_v_table.forEach((_, key) => {
this.addOwnKeyBinding(key, '_handleVAction');
@@ -231,27 +202,9 @@
return {};
}
- _handleVKeyDown(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- this._shortcut_v_key_last_pressed = Date.now();
- }
-
- _handleVKeyUp() {
- setTimeout(() => {
- this._shortcut_v_key_last_pressed = null;
- }, V_KEY_TIMEOUT_MS);
- }
-
- private inVKeyMode() {
- return !!(
- this._shortcut_v_key_last_pressed &&
- Date.now() - this._shortcut_v_key_last_pressed <= V_KEY_TIMEOUT_MS
- );
- }
-
_handleVAction(e: IronKeyboardEvent) {
if (
- !this.inVKeyMode() ||
+ !this.shortcuts.isInSpecificComboKeyMode(ComboKey.V) ||
!this._shortcut_v_table.has(e.detail.key) ||
this.shortcuts.shouldSuppress(e)
) {
@@ -266,29 +219,9 @@
}
}
- _handleGoKeyDown(e: IronKeyboardEvent) {
- if (this.shortcuts.shouldSuppress(e)) return;
- this._shortcut_go_key_last_pressed = Date.now();
- }
-
- _handleGoKeyUp() {
- // Set go_key_last_pressed to null `GO_KEY_TIMEOUT_MS` after keyup event
- // so that users can trigger `g + i` by pressing g and i quickly.
- setTimeout(() => {
- this._shortcut_go_key_last_pressed = null;
- }, GO_KEY_TIMEOUT_MS);
- }
-
- _inGoKeyMode() {
- return !!(
- this._shortcut_go_key_last_pressed &&
- Date.now() - this._shortcut_go_key_last_pressed <= GO_KEY_TIMEOUT_MS
- );
- }
-
_handleGoAction(e: IronKeyboardEvent) {
if (
- !this._inGoKeyMode() ||
+ !this.shortcuts.isInSpecificComboKeyMode(ComboKey.G) ||
!this._shortcut_go_table.has(e.detail.key) ||
this.shortcuts.shouldSuppress(e)
) {
@@ -304,10 +237,7 @@
}
}
- return Mixin as T &
- Constructor<
- KeyboardShortcutMixinInterface & KeyboardShortcutMixinInterfaceTesting
- >;
+ return Mixin as T & Constructor<KeyboardShortcutMixinInterface>;
};
// The following doesn't work (IronA11yKeysBehavior crashes):
@@ -320,10 +250,7 @@
// This is a workaround
export const KeyboardShortcutMixin = <T extends Constructor<PolymerElement>>(
superClass: T
-): T &
- Constructor<
- KeyboardShortcutMixinInterface & KeyboardShortcutMixinInterfaceTesting
- > =>
+): 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
@@ -334,13 +261,4 @@
/** The interface corresponding to KeyboardShortcutMixin */
export interface KeyboardShortcutMixinInterface {
keyboardShortcuts(): {[key: string]: string | null};
- modifierPressed(event: IronKeyboardEvent): boolean;
-}
-
-export interface KeyboardShortcutMixinInterfaceTesting {
- _shortcut_go_key_last_pressed: number | null;
- _shortcut_v_key_last_pressed: number | null;
- _shortcut_go_table: Map<string, string>;
- _shortcut_v_table: Map<string, string>;
- _handleGoAction: (e: IronKeyboardEvent) => void;
}
diff --git a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin_test.ts b/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin_test.ts
deleted file mode 100644
index 01ad6cc..0000000
--- a/polygerrit-ui/app/mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin_test.ts
+++ /dev/null
@@ -1,139 +0,0 @@
-/**
- * @license
- * Copyright (C) 2016 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-import '../../test/common-test-setup-karma';
-import {KeyboardShortcutMixin} from './keyboard-shortcut-mixin';
-import {PolymerElement} from '@polymer/polymer/polymer-element';
-import '../../elements/shared/gr-overlay/gr-overlay';
-import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions';
-import {IronKeyboardEvent} from '../../types/events';
-
-class GrKeyboardShortcutMixinTestElement extends KeyboardShortcutMixin(
- PolymerElement
-) {
- static get is() {
- return 'keyboard-shortcut-mixin-test-element';
- }
-
- get keyBindings() {
- return {
- k: '_handleKey',
- enter: '_handleKey',
- };
- }
-
- _handleKey(_: any) {}
-
- _handleA(_: any) {}
-}
-
-declare global {
- interface HTMLElementTagNameMap {
- 'keyboard-shortcut-mixin-test-element': GrKeyboardShortcutMixinTestElement;
- }
-}
-
-customElements.define(
- GrKeyboardShortcutMixinTestElement.is,
- GrKeyboardShortcutMixinTestElement
-);
-
-const basicFixture = fixtureFromElement('keyboard-shortcut-mixin-test-element');
-
-suite('keyboard-shortcut-mixin tests', () => {
- let element: GrKeyboardShortcutMixinTestElement;
-
- setup(async () => {
- element = basicFixture.instantiate();
- await flush();
- });
-
- test('modifierPressed returns accurate values', () => {
- const spy = sinon.spy(element, 'modifierPressed');
- element._handleKey = e => {
- element.modifierPressed(e);
- };
- MockInteractions.keyDownOn(element, 75, 'shift', 'k');
- assert.isTrue(spy.lastCall.returnValue);
- MockInteractions.keyDownOn(element, 75, null, 'k');
- assert.isFalse(spy.lastCall.returnValue);
- MockInteractions.keyDownOn(element, 75, 'ctrl', 'k');
- assert.isTrue(spy.lastCall.returnValue);
- MockInteractions.keyDownOn(element, 75, null, 'k');
- assert.isFalse(spy.lastCall.returnValue);
- MockInteractions.keyDownOn(element, 75, 'meta', 'k');
- assert.isTrue(spy.lastCall.returnValue);
- MockInteractions.keyDownOn(element, 75, null, 'k');
- assert.isFalse(spy.lastCall.returnValue);
- MockInteractions.keyDownOn(element, 75, 'alt', 'k');
- assert.isTrue(spy.lastCall.returnValue);
- });
-
- suite('GO_KEY timing', () => {
- let handlerStub: sinon.SinonStub;
-
- setup(() => {
- element._shortcut_go_table.set('a', '_handleA');
- handlerStub = element._handleA = sinon.stub();
- sinon.stub(Date, 'now').returns(10000);
- });
-
- test('success', () => {
- const e = {
- detail: {key: 'a'},
- preventDefault: () => {},
- composedPath: () => [],
- } as unknown as IronKeyboardEvent;
- element._shortcut_go_key_last_pressed = 9000;
- element._handleGoAction(e);
- assert.isTrue(handlerStub.calledOnce);
- assert.strictEqual(handlerStub.lastCall.args[0], e);
- });
-
- test('go key not pressed', () => {
- const e = {
- detail: {key: 'a'},
- preventDefault: () => {},
- composedPath: () => [],
- } as unknown as IronKeyboardEvent;
- element._shortcut_go_key_last_pressed = null;
- element._handleGoAction(e);
- assert.isFalse(handlerStub.called);
- });
-
- test('go key pressed too long ago', () => {
- const e = {
- detail: {key: 'a'},
- preventDefault: () => {},
- composedPath: () => [],
- } as unknown as IronKeyboardEvent;
- element._shortcut_go_key_last_pressed = 3000;
- element._handleGoAction(e);
- assert.isFalse(handlerStub.called);
- });
-
- test('unrecognized key', () => {
- const e = {
- detail: {key: 'f'},
- preventDefault: () => {},
- composedPath: () => [],
- } as unknown as IronKeyboardEvent;
- element._shortcut_go_key_last_pressed = 9000;
- element._handleGoAction(e);
- assert.isFalse(handlerStub.called);
- });
- });
-});
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
index d0e2d49..edc31a4 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -23,7 +23,7 @@
} from './shortcuts-config';
import {disableShortcuts$} from '../user/user-model';
import {IronKeyboardEvent, isIronKeyboardEvent} from '../../types/events';
-import {isElementTarget} from '../../utils/dom-util';
+import {isElementTarget, isModifierPressed} from '../../utils/dom-util';
import {ReportingService} from '../gr-reporting/gr-reporting';
export type SectionView = Array<{binding: string[][]; text: string}>;
@@ -35,7 +35,16 @@
viewMap?: Map<ShortcutSection, SectionView>
) => void;
-const COMBO_KEYS = ['g', 'v'];
+export enum ComboKey {
+ G = 'g',
+ V = 'v',
+}
+
+function isComboKey(key: string): key is ComboKey {
+ return Object.values(ComboKey).includes(key as ComboKey);
+}
+
+export const COMBO_TIMEOUT_MS = 1000;
/**
* Shortcuts service, holds all hosts, bindings and listeners.
@@ -54,12 +63,12 @@
private readonly listeners = new Set<ShortcutListener>();
/**
- * Maps keys (e.g. 'g') to the timestamp when they have last been pressed.
+ * Stores the timestamp of the last combo key being pressed.
* This enabled key combinations like 'g+o' where we can check whether 'g' was
* pressed recently when 'o' is processed. Keys of this map must be items of
* COMBO_KEYS. Values are Date timestamps in milliseconds.
*/
- private readonly keyLastPressed = new Map<string, number>();
+ private comboKeyLastPressed: {key?: ComboKey; timestampMs?: number} = {};
/** Keeps track of the corresponding user preference. */
private shortcutsDisabled = false;
@@ -73,9 +82,9 @@
}
disableShortcuts$.subscribe(x => (this.shortcutsDisabled = x));
document.addEventListener('keydown', (e: KeyboardEvent) => {
- if (!COMBO_KEYS.includes(e.key)) return;
+ if (!isComboKey(e.key)) return;
if (this.shouldSuppress(e)) return;
- this.keyLastPressed.set(e.key, Date.now());
+ this.comboKeyLastPressed = {key: e.key, timestampMs: Date.now()};
});
}
@@ -83,6 +92,25 @@
return this.activeHosts.size === 0 && this.listeners.size === 0;
}
+ isInComboKeyMode() {
+ return Object.values(ComboKey).some(key =>
+ this.isInSpecificComboKeyMode(key)
+ );
+ }
+
+ isInSpecificComboKeyMode(comboKey: ComboKey) {
+ const {key, timestampMs} = this.comboKeyLastPressed;
+ return (
+ key === comboKey &&
+ timestampMs &&
+ Date.now() - timestampMs < COMBO_TIMEOUT_MS
+ );
+ }
+
+ modifierPressed(e: IronKeyboardEvent) {
+ return isModifierPressed(e) || this.isInComboKeyMode();
+ }
+
shouldSuppress(event: IronKeyboardEvent | KeyboardEvent) {
if (this.shortcutsDisabled) return true;
const e = isIronKeyboardEvent(event) ? event.detail.keyboardEvent : event;
@@ -116,9 +144,8 @@
}
// eg: {key: "k:keydown", ..., from: "gr-diff-view"}
let key = `${e.key}:${e.type}`;
- // TODO(brohlfs): Re-enable reporting of g- and v-keys.
- // if (this._inGoKeyMode()) key = 'g+' + key;
- // if (this.inVKeyMode()) key = 'v+' + key;
+ if (this.isInSpecificComboKeyMode(ComboKey.G)) key = 'g+' + key;
+ if (this.isInSpecificComboKeyMode(ComboKey.V)) key = 'v+' + key;
if (e.shiftKey) key = 'shift+' + key;
if (e.ctrlKey) key = 'ctrl+' + key;
if (e.metaKey) key = 'meta+' + key;
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
index 0998a4c..d8aa11e 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
@@ -15,9 +15,13 @@
* limitations under the License.
*/
import '../../test/common-test-setup-karma';
-import {ShortcutsService} from '../../services/shortcuts/shortcuts-service';
+import {
+ ShortcutsService,
+ COMBO_TIMEOUT_MS,
+} 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';
async function keyEventOn(
el: HTMLElement,
@@ -290,4 +294,48 @@
});
});
});
+
+ suite('combo keys', () => {
+ let clock: SinonFakeTimers;
+ setup(() => {
+ clock = sinon.useFakeTimers();
+ clock.tick(1000);
+ });
+
+ teardown(() => {
+ clock.restore();
+ });
+
+ test('not in combo key mode initially', () => {
+ assert.isFalse(service.isInComboKeyMode());
+ });
+
+ test('pressing f does not switch into combo key mode', () => {
+ const event = new KeyboardEvent('keydown', {key: 'f'});
+ document.dispatchEvent(event);
+ assert.isFalse(service.isInComboKeyMode());
+ });
+
+ test('pressing g switches into combo key mode', () => {
+ const event = new KeyboardEvent('keydown', {key: 'g'});
+ document.dispatchEvent(event);
+ assert.isTrue(service.isInComboKeyMode());
+ });
+
+ test('pressing v switches into combo key mode', () => {
+ const event = new KeyboardEvent('keydown', {key: 'v'});
+ document.dispatchEvent(event);
+ assert.isTrue(service.isInComboKeyMode());
+ });
+
+ test('combo key mode timeout', () => {
+ const event = new KeyboardEvent('keydown', {key: 'g'});
+ document.dispatchEvent(event);
+ assert.isTrue(service.isInComboKeyMode());
+ clock.tick(COMBO_TIMEOUT_MS / 2);
+ assert.isTrue(service.isInComboKeyMode());
+ clock.tick(COMBO_TIMEOUT_MS);
+ assert.isFalse(service.isInComboKeyMode());
+ });
+ });
});
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts
index ead47bb..e3e342e 100644
--- a/polygerrit-ui/app/utils/dom-util.ts
+++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -304,6 +304,54 @@
}
}
+/**
+ * For matching the `key` property of KeyboardEvents. These are known to work
+ * with Firefox, Safari and Chrome.
+ */
+export enum Key {
+ ENTER = 'Enter',
+ ESC = 'Escape',
+ TAB = 'Tab',
+ SPACE = ' ',
+ LEFT = 'ArrowLeft',
+ RIGHT = 'ArrowRight',
+ UP = 'ArrowUp',
+ DOWN = 'ArrowDown',
+}
+
+export enum Modifier {
+ ALT_KEY,
+ CTRL_KEY,
+ META_KEY,
+ SHIFT_KEY,
+}
+
+export interface Shortcut {
+ key: string;
+ modifiers?: Modifier[];
+}
+
+export function addShortcut(
+ shortcut: Shortcut,
+ listener: (e: KeyboardEvent) => void
+) {
+ const wrappedListener = (e: KeyboardEvent) => {
+ if (e.key !== shortcut.key) return;
+ const modifiers = shortcut.modifiers ?? [];
+ if (e.ctrlKey !== modifiers.includes(Modifier.CTRL_KEY)) return;
+ if (e.metaKey !== modifiers.includes(Modifier.META_KEY)) return;
+ // TODO(brohlfs): Refine the matching of modifiers. For "normal" keys we
+ // don't want to check ALT and SHIFT, because we don't know what the
+ // keyboard layout looks like. The user may have to use ALT and/or SHIFT for
+ // certain keys. Comparing the `key` string is sufficient in that case.
+ // if (e.altKey !== modifiers.includes(Modifier.ALT_KEY)) return;
+ // if (e.shiftKey !== modifiers.includes(Modifier.SHIFT_KEY)) return;
+ listener(e);
+ };
+ document.addEventListener('keydown', wrappedListener);
+ return () => document.removeEventListener('keydown', wrappedListener);
+}
+
export function modifierPressed(e: KeyboardEvent) {
return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey;
}
