Cleanup: use iron-a11y-keys-behavior for keyboard shortcuts
+ This does not cover on-keydown handlers within elements.
A follow-up change will account for those.
+ Keyboard shortcuts are disabled within gr-overlay, input,
and textarea elements.
+ Added tests for new behavior (plus some missing ones covering
broken behavior).
+ Removed blur hacks used on elements to placate the kb
shortcuts due to restrictions that have been removed.
Bug: Issue 4198
Change-Id: Ide8009a3bfc340a35a8ec8b9189a85b49c8a95aa
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
index 9dbb25b..b6471e7 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
@@ -217,8 +217,7 @@
id="modeSelect"
is="gr-select"
bind-value="{{changeViewState.diffMode}}"
- hidden$="[[_computeModeSelectHidden(_isImageDiff)]]"
- on-change="_handleDropdownChange">
+ hidden$="[[_computeModeSelectHidden(_isImageDiff)]]">
<option value="SIDE_BY_SIDE">Side By Side</option>
<option value="UNIFIED_DIFF">Unified</option>
</select>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index 44204fd..1d63b47 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -100,6 +100,22 @@
'_getFiles(_changeNum, _patchRange.*)',
],
+ keyBindings: {
+ 'esc': '_handleEscKey',
+ 'shift+left': '_handleShiftLeftKey',
+ 'shift+right': '_handleShiftRightKey',
+ 'up k': '_handleUpKey',
+ 'down j': '_handleDownKey',
+ 'c': '_handleCKey',
+ '[': '_handleLeftBracketKey',
+ ']': '_handleRightBracketKey',
+ 'n shift+n': '_handleNKey',
+ 'p shift+p': '_handlePKey',
+ 'a shift+a': '_handleAKey',
+ 'u': '_handleUKey',
+ ',': '_handleCommaKey',
+ },
+
attached: function() {
this._getLoggedIn().then(function(loggedIn) {
this._loggedIn = loggedIn;
@@ -185,103 +201,129 @@
this._patchRange.patchNum, this._path, reviewed);
},
- _checkForModifiers: function(e) {
- return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey || false;
- },
-
- _handleKey: function(e) {
+ _handleEscKey: function(e) {
if (this.shouldSuppressKeyboardShortcut(e)) { return; }
- switch (e.keyCode) {
- case 27: // escape
- e.preventDefault();
- this.$.diff.displayLine = false;
- break;
- case 37: // left
- if (e.shiftKey) {
- e.preventDefault();
- this.$.cursor.moveLeft();
- }
- break;
- case 39: // right
- if (e.shiftKey) {
- e.preventDefault();
- this.$.cursor.moveRight();
- }
- break;
- case 40: // down
- case 74: // 'j'
- e.preventDefault();
- this.$.diff.displayLine = true;
- this.$.cursor.moveDown();
- break;
- case 38: // up
- case 75: // 'k'
- e.preventDefault();
- this.$.diff.displayLine = true;
- this.$.cursor.moveUp();
- break;
- case 67: // 'c'
- if (this._checkForModifiers(e)) { return; }
- if (!this.$.diff.isRangeSelected()) {
- e.preventDefault();
- var line = this.$.cursor.getTargetLineElement();
- if (line) {
- this.$.diff.addDraftAtLine(line);
- }
- }
- break;
- case 219: // '['
- e.preventDefault();
- this._navToFile(this._path, this._fileList, -1);
- break;
- case 221: // ']'
- e.preventDefault();
- this._navToFile(this._path, this._fileList, 1);
- break;
- case 78: // 'n'
- e.preventDefault();
- if (e.shiftKey) {
- this.$.cursor.moveToNextCommentThread();
- } else {
- this.$.cursor.moveToNextChunk();
- }
- break;
- case 80: // 'p'
- e.preventDefault();
- if (e.shiftKey) {
- this.$.cursor.moveToPreviousCommentThread();
- } else {
- this.$.cursor.moveToPreviousChunk();
- }
- break;
- case 65: // 'a'
- if (e.shiftKey) { // Hide left diff.
- e.preventDefault();
- this.$.diff.toggleLeftDiff();
- break;
- }
+ e.preventDefault();
+ this.$.diff.displayLine = false;
+ },
- if (!this._loggedIn) { break; }
+ _handleShiftLeftKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
- this.set('changeViewState.showReplyDialog', true);
- /* falls through */ // required by JSHint
- case 85: // 'u'
- if (this._changeNum && this._patchRange.patchNum) {
- e.preventDefault();
- page.show(this._getChangePath(
- this._changeNum,
- this._patchRange,
- this._change && this._change.revisions));
- }
- break;
- case 188: // ','
- e.preventDefault();
- this._openPrefs();
- break;
+ e.preventDefault();
+ this.$.cursor.moveLeft();
+ },
+
+ _handleShiftRightKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ this.$.cursor.moveRight();
+ },
+
+ _handleUpKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ this.$.diff.displayLine = true;
+ this.$.cursor.moveUp();
+ },
+
+ _handleDownKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ this.$.diff.displayLine = true;
+ this.$.cursor.moveDown();
+ },
+
+ _handleCKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+ if (this.$.diff.isRangeSelected()) { return; }
+
+ e.preventDefault();
+ var line = this.$.cursor.getTargetLineElement();
+ if (line) {
+ this.$.diff.addDraftAtLine(line);
}
},
+ _handleLeftBracketKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ this._navToFile(this._path, this._fileList, -1);
+ },
+
+ _handleRightBracketKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ this._navToFile(this._path, this._fileList, 1);
+ },
+
+ _handleNKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ if (e.detail.keyboardEvent.shiftKey) {
+ this.$.cursor.moveToNextCommentThread();
+ } else {
+ this.$.cursor.moveToNextChunk();
+ }
+ },
+
+ _handlePKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ if (e.detail.keyboardEvent.shiftKey) {
+ this.$.cursor.moveToPreviousCommentThread();
+ } else {
+ this.$.cursor.moveToPreviousChunk();
+ }
+ },
+
+ _handleAKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ if (e.detail.keyboardEvent.shiftKey) { // Hide left diff.
+ e.preventDefault();
+ this.$.diff.toggleLeftDiff();
+ return;
+ }
+
+ if (!this._loggedIn) { return; }
+
+ this.set('changeViewState.showReplyDialog', true);
+ e.preventDefault();
+ this._navToChangeView();
+ },
+
+ _handleUKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ this._navToChangeView();
+ },
+
+ _handleCommaKey: function(e) {
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+
+ e.preventDefault();
+ this._openPrefs();
+ },
+
+ _navToChangeView: function() {
+ if (!this._changeNum || !this._patchRange.patchNum) { return; }
+
+ page.show(this._getChangePath(
+ this._changeNum,
+ this._patchRange,
+ this._change && this._change.revisions));
+ },
+
_navToFile: function(path, fileList, direction) {
var url = this._computeNavLinkURL(path, fileList, direction);
if (!url) { return; }
@@ -556,10 +598,6 @@
history.replaceState(null, null, '#' + this.$.cursor.getAddress());
},
- _handleDropdownChange: function(e) {
- e.target.blur();
- },
-
_computeDownloadLink: function(changeNum, patchRange, path) {
var url = this.changeBaseURL(changeNum, patchRange.patchNum);
url += '/patch?zip&path=' + encodeURIComponent(path);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index e423e45..c6ccb1b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -62,7 +62,7 @@
test('toggle left diff with a hotkey', function() {
var toggleLeftDiffStub = sandbox.stub(element.$.diff, 'toggleLeftDiff');
- MockInteractions.pressAndReleaseKeyOn(element, 65, 'shift'); // 'a'
+ MockInteractions.pressAndReleaseKeyOn(element, 65, 'shift', 'a');
assert.isTrue(toggleLeftDiffStub.calledOnce);
});
@@ -82,29 +82,29 @@
element.changeViewState.selectedFileIndex = 1;
var showStub = sandbox.stub(page, 'show');
- MockInteractions.pressAndReleaseKeyOn(element, 85); // 'u'
+ MockInteractions.pressAndReleaseKeyOn(element, 85, null, 'u');
assert(showStub.lastCall.calledWithExactly('/c/42/'),
'Should navigate to /c/42/');
- MockInteractions.pressAndReleaseKeyOn(element, 221); // ']'
+ MockInteractions.pressAndReleaseKeyOn(element, 221, null, ']');
assert(showStub.lastCall.calledWithExactly('/c/42/10/wheatley.md'),
'Should navigate to /c/42/10/wheatley.md');
element._path = 'wheatley.md';
assert.equal(element.changeViewState.selectedFileIndex, 2);
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/10/glados.txt'),
'Should navigate to /c/42/10/glados.txt');
element._path = 'glados.txt';
assert.equal(element.changeViewState.selectedFileIndex, 1);
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/10/chell.go'),
'Should navigate to /c/42/10/chell.go');
element._path = 'chell.go';
assert.equal(element.changeViewState.selectedFileIndex, 0);
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/'),
'Should navigate to /c/42/');
assert.equal(element.changeViewState.selectedFileIndex, 0);
@@ -112,33 +112,33 @@
var showPrefsStub = sandbox.stub(element.$.prefsOverlay, 'open',
function() { return Promise.resolve({}); });
- MockInteractions.pressAndReleaseKeyOn(element, 188); // ','
+ MockInteractions.pressAndReleaseKeyOn(element, 188, null, ',');
assert(showPrefsStub.calledOnce);
var scrollStub = sandbox.stub(element.$.cursor, 'moveToNextChunk');
- MockInteractions.pressAndReleaseKeyOn(element, 78); // 'n'
+ MockInteractions.pressAndReleaseKeyOn(element, 78, null, 'n');
assert(scrollStub.calledOnce);
scrollStub = sandbox.stub(element.$.cursor, 'moveToPreviousChunk');
- MockInteractions.pressAndReleaseKeyOn(element, 80); // 'p'
+ MockInteractions.pressAndReleaseKeyOn(element, 80, null, 'p');
assert(scrollStub.calledOnce);
scrollStub = sandbox.stub(element.$.cursor, 'moveToNextCommentThread');
- MockInteractions.pressAndReleaseKeyOn(element, 78, ['shift']); // 'N'
+ MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
assert(scrollStub.calledOnce);
scrollStub = sandbox.stub(element.$.cursor,
'moveToPreviousCommentThread');
- MockInteractions.pressAndReleaseKeyOn(element, 80, ['shift']); // 'P'
+ MockInteractions.pressAndReleaseKeyOn(element, 80, 'shift', 'p');
assert(scrollStub.calledOnce);
var computeContainerClassStub = sandbox.stub(element.$.diff,
'_computeContainerClass');
- MockInteractions.pressAndReleaseKeyOn(element, 74); // 'j'
+ MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
assert(computeContainerClassStub.lastCall.calledWithExactly(
false, 'SIDE_BY_SIDE', true));
- MockInteractions.pressAndReleaseKeyOn(element, 27); // 'escape'
+ MockInteractions.pressAndReleaseKeyOn(element, 27, null, 'esc');
assert(computeContainerClassStub.lastCall.calledWithExactly(
false, 'SIDE_BY_SIDE', false));
});
@@ -175,39 +175,39 @@
var showStub = sandbox.stub(page, 'show');
- MockInteractions.pressAndReleaseKeyOn(element, 65); // 'a'
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
assert.isTrue(showStub.notCalled, 'The `a` keyboard shortcut should ' +
'only work when the user is logged in.');
assert.isNull(window.sessionStorage.getItem(
'changeView.showReplyDialog'));
element._loggedIn = true;
- MockInteractions.pressAndReleaseKeyOn(element, 65); // 'a'
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
assert.isTrue(element.changeViewState.showReplyDialog);
assert(showStub.lastCall.calledWithExactly('/c/42/5..10'),
'Should navigate to /c/42/5..10');
- MockInteractions.pressAndReleaseKeyOn(element, 85); // 'u'
+ MockInteractions.pressAndReleaseKeyOn(element, 85, null, 'u');
assert(showStub.lastCall.calledWithExactly('/c/42/5..10'),
'Should navigate to /c/42/5..10');
- MockInteractions.pressAndReleaseKeyOn(element, 221); // ']'
+ MockInteractions.pressAndReleaseKeyOn(element, 221, null, ']');
assert(showStub.lastCall.calledWithExactly('/c/42/5..10/wheatley.md'),
'Should navigate to /c/42/5..10/wheatley.md');
element._path = 'wheatley.md';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/5..10/glados.txt'),
'Should navigate to /c/42/5..10/glados.txt');
element._path = 'glados.txt';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/5..10/chell.go'),
'Should navigate to /c/42/5..10/chell.go');
element._path = 'chell.go';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/5..10'),
'Should navigate to /c/42/5..10');
});
@@ -229,39 +229,39 @@
var showStub = sandbox.stub(page, 'show');
- MockInteractions.pressAndReleaseKeyOn(element, 65); // 'a'
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
assert.isTrue(showStub.notCalled, 'The `a` keyboard shortcut should ' +
'only work when the user is logged in.');
assert.isNull(window.sessionStorage.getItem(
'changeView.showReplyDialog'));
element._loggedIn = true;
- MockInteractions.pressAndReleaseKeyOn(element, 65); // 'a'
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
assert.isTrue(element.changeViewState.showReplyDialog);
assert(showStub.lastCall.calledWithExactly('/c/42/1'),
'Should navigate to /c/42/1');
- MockInteractions.pressAndReleaseKeyOn(element, 85); // 'u'
+ MockInteractions.pressAndReleaseKeyOn(element, 85, null, 'u');
assert(showStub.lastCall.calledWithExactly('/c/42/1'),
'Should navigate to /c/42/1');
- MockInteractions.pressAndReleaseKeyOn(element, 221); // ']'
+ MockInteractions.pressAndReleaseKeyOn(element, 221, null, ']');
assert(showStub.lastCall.calledWithExactly('/c/42/1/wheatley.md'),
'Should navigate to /c/42/1/wheatley.md');
element._path = 'wheatley.md';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/1/glados.txt'),
'Should navigate to /c/42/1/glados.txt');
element._path = 'glados.txt';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/1/chell.go'),
'Should navigate to /c/42/1/chell.go');
element._path = 'chell.go';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/1'),
'Should navigate to /c/42/1');
});
@@ -278,39 +278,39 @@
var showStub = sandbox.stub(page, 'show');
- MockInteractions.pressAndReleaseKeyOn(element, 65); // 'a'
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
assert.isTrue(showStub.notCalled, 'The `a` keyboard shortcut should ' +
'only work when the user is logged in.');
assert.isNull(window.sessionStorage.getItem(
'changeView.showReplyDialog'));
element._loggedIn = true;
- MockInteractions.pressAndReleaseKeyOn(element, 65); // 'a'
+ MockInteractions.pressAndReleaseKeyOn(element, 65, null, 'a');
assert.isTrue(element.changeViewState.showReplyDialog);
assert(showStub.lastCall.calledWithExactly('/c/42/1'),
'Should navigate to /c/42/1');
- MockInteractions.pressAndReleaseKeyOn(element, 85); // 'u'
+ MockInteractions.pressAndReleaseKeyOn(element, 85, null, 'u');
assert(showStub.lastCall.calledWithExactly('/c/42/1'),
'Should navigate to /c/42/1');
- MockInteractions.pressAndReleaseKeyOn(element, 221); // ']'
+ MockInteractions.pressAndReleaseKeyOn(element, 221, null, ']');
assert(showStub.lastCall.calledWithExactly('/c/42/1/wheatley.md'),
'Should navigate to /c/42/1/wheatley.md');
element._path = 'wheatley.md';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/1/glados.txt'),
'Should navigate to /c/42/1/glados.txt');
element._path = 'glados.txt';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/1/chell.go'),
'Should navigate to /c/42/1/chell.go');
element._path = 'chell.go';
- MockInteractions.pressAndReleaseKeyOn(element, 219); // '['
+ MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(showStub.lastCall.calledWithExactly('/c/42/1'),
'Should navigate to /c/42/1');
});
@@ -457,7 +457,6 @@
test('diff mode selector correctly toggles the diff', function() {
var select = element.$.modeSelect;
var diffDisplay = element.$.diff;
- var blurSpy = sandbox.spy(select, 'blur');
element._userPrefs = {diff_view: 'SIDE_BY_SIDE'};
// The mode selected in the view state reflects the selected option.
@@ -477,7 +476,6 @@
assert.equal(element._getDiffViewMode(), newMode);
assert.equal(element._getDiffViewMode(), select.value);
assert.equal(element._getDiffViewMode(), diffDisplay.viewMode);
- assert(blurSpy.called, 'select should be blurred after selection');
});
test('diff mode selector initializes from preferences', function() {
@@ -557,14 +555,6 @@
assert.equal(element.$.cursor.side, 'left');
});
- test('_checkForModifiers', function() {
- assert.isTrue(element._checkForModifiers({altKey: true}));
- assert.isTrue(element._checkForModifiers({ctrlKey: true}));
- assert.isTrue(element._checkForModifiers({metaKey: true}));
- assert.isTrue(element._checkForModifiers({shiftKey: true}));
- assert.isFalse(element._checkForModifiers({}));
- });
-
test('_shortenPath with long path should add ellipsis', function() {
var path =
'level1/level2/level3/level4/file.js';
