Merge "Fix comment navigation in file list"

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') {

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);
+    });
   });
 </script>

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();

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);
+      });
+    });
   });
 </script>