Scroll to target if the bottom is not visible and meets condition

Previously, we did not scroll to target in the cursor manager if the top
was visible. However, there were times where the bottom content was
not visible, and it could have moved up into view.

This change passes an optional function for calcuating target height to
the cursor manager. If it is passed, the function is used to calculate
height instead of targetHeight. Height is ultimately used to determine
if the bottom is visible.

If the top is visible, but the bottom is not, scroll to the target if
the scroll position is farther down than the current position. Don't
scroll if the condition is not met because more content related to the
target is actually visible without scrolling, and do not want to reduce
it.

Bug: Issue 5498
Change-Id: I1708c921093b6e8b1916ae68fb468816f7c67633

polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js

@@ -127,7 +127,10 @@
     },
 
     moveToNextChunk: function() {
-      this.$.cursorManager.next(this._isFirstRowOfChunk.bind(this));
+      this.$.cursorManager.next(this._isFirstRowOfChunk.bind(this),
+          function(target) {
+            return target.parentNode.scrollHeight;
+          });
       this._fixSide();
     },
 

polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js

@@ -35,6 +35,10 @@
         notify: true,
         observer: '_scrollToTarget',
       },
+      /**
+       * The height of content intended to be included with the target.
+       */
+      _targetHeight: Number,
 
       /**
        * The index of the current target (if any). -1 otherwise.
@@ -76,8 +80,8 @@
       this.unsetCursor();
     },
 
-    next: function(opt_condition) {
-      this._moveCursor(1, opt_condition);
+    next: function(opt_condition, opt_getTargetHeight) {
+      this._moveCursor(1, opt_condition, opt_getTargetHeight);
     },
 
     previous: function(opt_condition) {
@@ -109,6 +113,7 @@
       this._unDecorateTarget();
       this.index = -1;
       this.target = null;
+      this._targetHeight = null;
     },
 
     isAtStart: function() {
@@ -136,9 +141,12 @@
      * @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.
      * @private
      */
-    _moveCursor: function(delta, opt_condition) {
+    _moveCursor: function(delta, opt_condition, opt_getTargetHeight) {
       if (!this.stops.length) {
         this.unsetCursor();
         return;
@@ -153,6 +161,12 @@
         newTarget = this.stops[newIndex];
       }
 
+      if (opt_getTargetHeight) {
+        this._targetHeight = opt_getTargetHeight(newTarget);
+      } else {
+        this._targetHeight = newTarget.scrollHeight;
+      }
+
       this.index = newIndex;
       this.target = newTarget;
 
@@ -245,22 +259,35 @@
           top < window.pageYOffset + window.innerHeight;
     },
 
+    _calculateScrollToValue: function(top, target) {
+      return top - (window.innerHeight / 3) + (target.offsetHeight / 2);
+    },
+
     _scrollToTarget: function() {
       if (!this.target || this.scrollBehavior === ScrollBehavior.NEVER) {
         return;
       }
 
       var top = this._getTop(this.target);
+      var bottomIsVisible = this._targetHeight ?
+          this._targetIsVisible(top + this._targetHeight) : true;
+      var scrollToValue = this._calculateScrollToValue(top, this.target);
+
       if (this._targetIsVisible(top)) {
-        return;
+        // Don't scroll if either the bottom is visible or if the position that
+        // would get scrolled to is higher up than the current position. this
+        // woulld cause less of the target content to be displayed than is
+        // already.
+        if (bottomIsVisible || scrollToValue < window.scrollY) {
+          return;
+        }
       }
 
       // Scroll the element to the middle of the window. Dividing by a third
       // instead of half the inner height feels a bit better otherwise the
       // element appears to be below the center of the window even when it
       // isn't.
-      window.scrollTo(0, top - (window.innerHeight / 3) +
-          (this.target.offsetHeight / 2));
+      window.scrollTo(0, scrollToValue);
     },
   });
 })();

polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.html

@@ -125,6 +125,23 @@ limitations under the License.
       assert.equal(element.index, -1);
     });
 
+
+    test('_moveCursor', function() {
+      // Initialize the cursor with its stops.
+      element.stops = list.querySelectorAll('li');
+      // Select the first stop.
+      element.setCursor(list.children[0]);
+      var getTargetHeight = sinon.stub();
+
+      // Move the cursor without an optional get target height function.
+      element._moveCursor(1);
+      assert.isFalse(getTargetHeight.called);
+
+      // Move the cursor with an optional get target height function.
+      element._moveCursor(1, null, getTargetHeight);
+      assert.isTrue(getTargetHeight.called);
+    });
+
     test('opt_noScroll', function() {
       sandbox.stub(element, '_targetIsVisible', function() { return false; });
       var scrollStub = sandbox.stub(window, 'scrollTo');
@@ -182,5 +199,63 @@ limitations under the License.
       element.next();
       assert.isTrue(element.target.focus.called);
     });
+
+    suite('_scrollToTarget', function() {
+      var scrollStub;
+      setup(function() {
+        element.stops = list.querySelectorAll('li');
+        element.scrollBehavior = 'keep-visible';
+
+        // There is a target which has a targetNext
+        element.setCursor(list.children[0]);
+        element._moveCursor(1);
+        scrollStub = sandbox.stub(window, 'scrollTo');
+        window.innerHeight = 60;
+      });
+
+      test('Called when top and bottom not visible', function() {
+        sandbox.stub(element, '_targetIsVisible', function() {
+          return false;
+        });
+        element._scrollToTarget();
+        assert.isTrue(scrollStub.called);
+      });
+
+      test('Not called when top and bottom visible', function() {
+        sandbox.stub(element, '_targetIsVisible', function() {
+          return true;
+        });
+        element._scrollToTarget();
+        assert.isFalse(scrollStub.called);
+      });
+
+      test('Called when top is visible, bottom is not, and scroll is lower',
+          function() {
+        var visibleStub = sandbox.stub(element, '_targetIsVisible', function() {
+          return visibleStub.callCount == 2;
+        });
+        window.scrollY = 15;
+        sandbox.stub(element, '_calculateScrollToValue', function() {
+          return 20;
+        });
+        element._scrollToTarget();
+        assert.isTrue(scrollStub.called);
+        assert.equal(visibleStub.callCount, 2);
+      });
+
+      test('Called when top is visible, bottom is not, and scroll is higher',
+          function() {
+        var visibleStub = sandbox.stub(element, '_targetIsVisible', function() {
+          return visibleStub.callCount == 2;
+        });
+        window.scrollY = 25;
+        sandbox.stub(element, '_calculateScrollToValue', function() {
+          return 20;
+        });
+        element._scrollToTarget();
+        assert.isFalse(scrollStub.called);
+        assert.equal(visibleStub.callCount, 2);
+      });
+    });
   });
 </script>