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';