From e3be54861c91bad17447d7e5f7d5470e798d12b9 Mon Sep 17 00:00:00 2001
From: Ole Rehmsen <oler@google.com>
Date: Mon, 6 May 2019 17:44:35 +0200
Subject: [PATCH] If file is unchanged, start scrolled to top

The previous code has a bug where when you navigate to the next file
using keyboard shortcuts, and the next file does not have changes, the
cursor checks all regions of interest (comments and such) and because
they are not diffs, moves past them to the bottom of the diff.

This change allows us to specify the intention to go back to top
whenever we get out of range to prevent this.

Change-Id: I7d00a43115ca0f926fb7a7f1b93084ff5ef0e334
---
 .../diff/gr-diff-cursor/gr-diff-cursor.js     |  6 +--
 .../gr-cursor-manager/gr-cursor-manager.js    | 37 ++++++++++++++-----
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
index 485b0bb80f..6dbc399525 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
@@ -136,11 +136,11 @@
       }
     },
 
-    moveToNextChunk() {
+    moveToNextChunk(opt_clipToTop) {
       this.$.cursorManager.next(this._isFirstRowOfChunk.bind(this),
           target => {
             return target.parentNode.scrollHeight;
-          });
+          }, opt_clipToTop);
       this._fixSide();
     },
 
@@ -200,7 +200,7 @@
 
     moveToFirstChunk() {
       this.$.cursorManager.moveToStart();
-      this.moveToNextChunk();
+      this.moveToNextChunk(true);
     },
 
     reInitCursor() {
diff --git a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js
index 36e6a7b422..2c38c2a7b5 100644
--- a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js
+++ b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js
@@ -90,8 +90,21 @@
       this.unsetCursor();
     },
 
-    next(opt_condition, opt_getTargetHeight) {
-      this._moveCursor(1, opt_condition, opt_getTargetHeight);
+    /**
+     * Move the cursor forward. Clipped to the ends of the stop list.
+     * @param {!Function=} opt_condition Optional stop condition. If a condition
+     *    is passed the cursor will continue to move in the specified direction
+     *    until the condition is met.
+     * @param {!Function=} opt_getTargetHeight Optional function to calculate the
+     *    height of the target's 'section'. The height of the target itself is
+     *    sometimes different, used by the diff cursor.
+     * @param {boolean=} opt_clipToTop When none of the next indices match, move
+     *     back to first instead of to last.
+     * @private
+     */
+
+    next(opt_condition, opt_getTargetHeight, opt_clipToTop) {
+      this._moveCursor(1, opt_condition, opt_getTargetHeight, opt_clipToTop);
     },
 
     previous(opt_condition) {
@@ -145,8 +158,8 @@
     },
 
     /**
-     * Move the cursor forward or backward by delta. Noop if moving past either
-     * end of the stop list.
+     * Move the cursor forward or backward by delta. Clipped to the beginning or
+     * end of stop list.
      * @param {number} delta either -1 or 1.
      * @param {!Function=} opt_condition Optional stop condition. If a condition
      *    is passed the cursor will continue to move in the specified direction
@@ -154,9 +167,11 @@
      * @param {!Function=} opt_getTargetHeight Optional function to calculate the
      *    height of the target's 'section'. The height of the target itself is
      *    sometimes different, used by the diff cursor.
+     * @param {boolean=} opt_clipToTop When none of the next indices match, move
+     *     back to first instead of to last.
      * @private
      */
-    _moveCursor(delta, opt_condition, opt_getTargetHeight) {
+    _moveCursor(delta, opt_condition, opt_getTargetHeight, opt_clipToTop) {
       if (!this.stops.length) {
         this.unsetCursor();
         return;
@@ -164,7 +179,7 @@
 
       this._unDecorateTarget();
 
-      const newIndex = this._getNextindex(delta, opt_condition);
+      const newIndex = this._getNextindex(delta, opt_condition, opt_clipToTop);
 
       let newTarget = null;
       if (newIndex !== -1) {
@@ -203,10 +218,12 @@
      * Get the next stop index indicated by the delta direction.
      * @param {number} delta either -1 or 1.
      * @param {!Function=} opt_condition Optional stop condition.
+     * @param {boolean=} opt_clipToTop When none of the next indices match, move
+     *     back to first instead of to last.
      * @return {number} the new index.
      * @private
      */
-    _getNextindex(delta, opt_condition) {
+    _getNextindex(delta, opt_condition, opt_clipToTop) {
       if (!this.stops.length || this.index === -1) {
         return -1;
       }
@@ -222,10 +239,10 @@
 
       // If we failed to satisfy the condition:
       if (opt_condition && !opt_condition(this.stops[newIndex])) {
-        if (delta > 0) {
-          return this.stops.length - 1;
-        } else if (delta < 0) {
+        if (delta < 0 || opt_clipToTop) {
           return 0;
+        } else if (delta > 0) {
+          return this.stops.length - 1;
         }
         return this.index;
       }