diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html index 3d99cec0a9..6003bc4259 100644 --- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html +++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html @@ -32,6 +32,13 @@ limitations under the License. return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey; }, + isModifierPressed: function(e, modifier) { + e = getKeyboardEvent(e); + // When e is a keyboardEvent, e.event is not null. + if (e.event) { e = e.event; } + return e[modifier]; + }, + shouldSuppressKeyboardShortcut: function(e) { e = getKeyboardEvent(e); if (e.path[0].tagName === 'INPUT' || e.path[0].tagName === 'TEXTAREA') { diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html index a72eb75356..840998cb0d 100644 --- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html +++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html @@ -127,5 +127,26 @@ limitations under the License. MockInteractions.keyDownOn(element, 75, 'alt', 'k'); assert.isTrue(spy.lastCall.returnValue); }); + + test('isModifierPressed returns accurate value', function() { + var spy = sandbox.spy(element, 'isModifierPressed'); + element._handleKey = function(e) { + element.isModifierPressed(e, 'shiftKey'); + }; + 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.isFalse(spy.lastCall.returnValue); + MockInteractions.keyDownOn(element, 75, null, 'k'); + assert.isFalse(spy.lastCall.returnValue); + MockInteractions.keyDownOn(element, 75, 'meta', 'k'); + assert.isFalse(spy.lastCall.returnValue); + MockInteractions.keyDownOn(element, 75, null, 'k'); + assert.isFalse(spy.lastCall.returnValue); + MockInteractions.keyDownOn(element, 75, 'alt', 'k'); + assert.isFalse(spy.lastCall.returnValue); + }); }); diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js index 19148670e3..d6eb7be546 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js @@ -524,11 +524,13 @@ _handleNKey: function(e) { if (this.shouldSuppressKeyboardShortcut(e) || - this.modifierPressed(e)) { return; } + this.modifierPressed(e) && !this.isModifierPressed(e, 'shiftKey')) { + return; + } if (!this._showInlineDiffs) { return; } e.preventDefault(); - if (e.shiftKey) { + if (this.isModifierPressed(e, 'shiftKey')) { this.$.diffCursor.moveToNextCommentThread(); } else { this.$.diffCursor.moveToNextChunk(); @@ -537,11 +539,13 @@ _handlePKey: function(e) { if (this.shouldSuppressKeyboardShortcut(e) || - this.modifierPressed(e)) { return; } + this.modifierPressed(e) && !this.isModifierPressed(e, 'shiftKey')) { + return; + } if (!this._showInlineDiffs) { return; } e.preventDefault(); - if (e.shiftKey) { + if (this.isModifierPressed(e, 'shiftKey')) { this.$.diffCursor.moveToPreviousCommentThread(); } else { this.$.diffCursor.moveToPreviousChunk(); diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html index 8775a21ec6..1b5a4e4cb5 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html @@ -941,7 +941,6 @@ limitations under the License. suite('gr-file-list inline diff tests', function() { var element; var sandbox; - var saveStub; var setupDiff = function(diff) { var mock = document.createElement('mock-diff-response'); @@ -1024,8 +1023,6 @@ limitations under the License. sandbox.stub(window, 'fetch', function() { return Promise.resolve(); }); - saveStub = sandbox.stub(element, '_saveReviewedState', - function() { return Promise.resolve(); }); flushAsynchronousOperations(); }); @@ -1063,7 +1060,7 @@ limitations under the License. MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i'); flushAsynchronousOperations(); - var diffs = renderAndGetNewDiffs(1); + diffs = renderAndGetNewDiffs(1); // Two diffs should be rendered. assert.equal(diffs.length, 2); var diffStopsFirst = diffs[0].getCursorStops(); @@ -1103,5 +1100,73 @@ limitations under the License. // The file cusor is still at 0. assert.equal(element.$.fileCursor.index, 0); }); + + suite('n key presses', function() { + var nKeySpy; + var nextCommentStub; + var nextChunkStub; + var fileRows; + setup(function() { + nKeySpy = sandbox.spy(element, '_handleNKey'); + nextCommentStub = sandbox.stub(element.$.diffCursor, + 'moveToNextCommentThread'); + nextChunkStub = sandbox.stub(element.$.diffCursor, + 'moveToNextChunk'); + fileRows = + Polymer.dom(element.root).querySelectorAll('.row:not(.header)'); + }); + test('n key with all files expanded and no shift key', function() { + MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'i'); + flushAsynchronousOperations(); + + // 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.isFalse(!!element._showInlineDiffs); + }); + + test('n key with all files expanded and shift key', function() { + MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, null, 'i'); + flushAsynchronousOperations(); + + MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n'); + assert.isTrue(nKeySpy.called); + assert.isFalse(nextCommentStub.called); + + // This is also called in diffCursor.moveToFirstChunk. + assert.equal(nextChunkStub.callCount, 1); + assert.isFalse(!!element._showInlineDiffs); + }); + + test('n key without all files expanded and shift key', function() { + MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, 'shift', 'i'); + flushAsynchronousOperations(); + + 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, 2); + assert.isTrue(element._showInlineDiffs); + }); + + test('n key without all files expanded and no shift key', function() { + MockInteractions.pressAndReleaseKeyOn(fileRows[0], 73, 'shift', 'i'); + flushAsynchronousOperations(); + + MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n'); + assert.isTrue(nKeySpy.called); + assert.isTrue(nextCommentStub.called); + + // This is also called in diffCursor.moveToFirstChunk. + assert.equal(nextChunkStub.callCount, 1); + assert.isTrue(element._showInlineDiffs); + }); + }); });