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
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 63a1a7d..9bfdcfb 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
@@ -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);
},
});
})();
