Prevent cursor from scrolling if multiple files are being rendered
After clicking Expand All, Gerrit begins to render the diffs and if
the user starts scrolling, the 'never' scroll behaviour is supposed to
prevent the scrolling of the user to the top.
We prevent any auto scrolling happening now after Expand All is
clicked.
This however introduces a regression in which the first chunk is not
focused on a small screen but we are fine in not supporting this use
case as of now.
Rename ScrollBehaviour to ScrollModes as Behaviours in Gerrit are
similar to mixins.
Move ScrollModes into constants.js
Change-Id: Ic904c01c5bf93934c8c9b215e35e4b1d95f4498d
diff --git a/polygerrit-ui/app/constants/constants.js b/polygerrit-ui/app/constants/constants.js
index 084bb0f..5001dd2 100644
--- a/polygerrit-ui/app/constants/constants.js
+++ b/polygerrit-ui/app/constants/constants.js
@@ -42,4 +42,16 @@
TAG_NEW_PATCHSET: 'autogenerated:gerrit:newPatchSet',
TAG_NEW_WIP_PATCHSET: 'autogenerated:gerrit:newWipPatchSet',
TAG_REVIEWER_UPDATE: 'autogenerated:gerrit:reviewerUpdate',
-};
\ No newline at end of file
+};
+
+/**
+ * @enum
+ * @desc Modes for gr-diff-cursor
+ * The scroll behavior for the cursor. Values are 'never' and
+ * 'keep-visible'. 'keep-visible' will only scroll if the cursor is beyond
+ * the viewport.
+ */
+export const ScrollModes = {
+ KEEP_VISIBLE: 'keep-visible',
+ NEVER: 'never',
+};
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js
index 17150df..e5193f8 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js
@@ -138,7 +138,7 @@
<gr-cursor-manager
id="cursor"
index="{{selectedIndex}}"
- scroll-behavior="keep-visible"
+ scroll-mode="keep-visible"
focus-on-move=""
></gr-cursor-manager>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
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 24daeab..9f93e21 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
@@ -1195,7 +1195,15 @@
console.log('Finished expanding', initialCount, 'diff(s)');
this.reporting.timeEndWithAverage(EXPAND_ALL_TIMING_LABEL,
EXPAND_ALL_AVG_TIMING_LABEL, initialCount);
- this.$.diffCursor.handleDiffUpdate();
+ /* Block diff cursor from auto scrolling after files are done rendering.
+ * This prevents the bug where the screen jumps to the first diff chunk
+ * after files are done being rendered after the user has already begun
+ * scrolling.
+ * This also however results in the fact that the cursor does not auto
+ * focus on the first diff chunk on a small screen. This is however, a use
+ * case we are willing to not support for now.
+ */
+ this.$.diffCursor.reInitAndUpdateStops();
}));
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
index f1010bf..42e07f5 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
@@ -584,7 +584,7 @@
<gr-diff-cursor id="diffCursor"></gr-diff-cursor>
<gr-cursor-manager
id="fileCursor"
- scroll-behavior="keep-visible"
+ scroll-mode="keep-visible"
focus-on-move=""
cursor-target-class="selected"
></gr-cursor-manager>
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 444fad4..b12bd9b 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
@@ -22,6 +22,7 @@
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin.js';
import {PolymerElement} from '@polymer/polymer/polymer-element.js';
import {htmlTemplate} from './gr-diff-cursor_html.js';
+import {ScrollModes} from '../../../constants/constants.js';
const DiffSides = {
LEFT: 'left',
@@ -33,11 +34,6 @@
UNIFIED: 'UNIFIED_DIFF',
};
-const ScrollBehavior = {
- KEEP_VISIBLE: 'keep-visible',
- NEVER: 'never',
-};
-
const LEFT_SIDE_CLASS = 'target-side-left';
const RIGHT_SIDE_CLASS = 'target-side-right';
@@ -92,9 +88,9 @@
* 'keep-visible'. 'keep-visible' will only scroll if the cursor is beyond
* the viewport.
*/
- _scrollBehavior: {
+ _scrollMode: {
type: String,
- value: ScrollBehavior.KEEP_VISIBLE,
+ value: ScrollModes.KEEP_VISIBLE,
},
_focusOnMove: {
@@ -294,9 +290,9 @@
if (!this.diffRow) {
// does not scroll during init unless requested
const scrollingBehaviorForInit = this.initialLineNumber ?
- ScrollBehavior.KEEP_VISIBLE :
- ScrollBehavior.NEVER;
- this._scrollBehavior = scrollingBehaviorForInit;
+ ScrollModes.KEEP_VISIBLE :
+ ScrollModes.NEVER;
+ this._scrollMode = scrollingBehaviorForInit;
if (this.initialLineNumber) {
this.moveToLineNumber(this.initialLineNumber, this.side);
this.initialLineNumber = null;
@@ -308,17 +304,22 @@
}
reInit() {
- this._scrollBehavior = ScrollBehavior.KEEP_VISIBLE;
+ this._scrollMode = ScrollModes.KEEP_VISIBLE;
}
_handleWindowScroll() {
if (this._preventAutoScrollOnManualScroll) {
- this._scrollBehavior = ScrollBehavior.NEVER;
+ this._scrollMode = ScrollModes.NEVER;
this._focusOnMove = false;
this._preventAutoScrollOnManualScroll = false;
}
}
+ reInitAndUpdateStops() {
+ this.reInit();
+ this._updateStops();
+ }
+
handleDiffUpdate() {
this._updateStops();
this.reInitCursor();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_html.js b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_html.js
index 1ac47f6..e400792 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_html.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_html.js
@@ -19,7 +19,7 @@
export const htmlTemplate = html`
<gr-cursor-manager
id="cursorManager"
- scroll-behavior="[[_scrollBehavior]]"
+ scroll-mode="[[_scrollMode]]"
cursor-target-class="target-row"
focus-on-move="[[_focusOnMove]]"
target="{{diffRow}}"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
index 68a2a77..773ec0b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
@@ -117,20 +117,20 @@
});
test('cursor scroll behavior', () => {
- assert.equal(cursorElement._scrollBehavior, 'keep-visible');
+ assert.equal(cursorElement._scrollMode, 'keep-visible');
cursorElement._handleDiffRenderStart();
assert.isTrue(cursorElement._focusOnMove);
cursorElement._handleWindowScroll();
- assert.equal(cursorElement._scrollBehavior, 'never');
+ assert.equal(cursorElement._scrollMode, 'never');
assert.isFalse(cursorElement._focusOnMove);
cursorElement._handleDiffRenderContent();
assert.isTrue(cursorElement._focusOnMove);
cursorElement.reInitCursor();
- assert.equal(cursorElement._scrollBehavior, 'keep-visible');
+ assert.equal(cursorElement._scrollMode, 'keep-visible');
});
test('moves to selected line', () => {
@@ -248,7 +248,7 @@
let scrollBehaviorDuringMove;
const moveToNumStub = sandbox.stub(cursorElement, 'moveToLineNumber');
const moveToChunkStub = sandbox.stub(cursorElement, 'moveToFirstChunk',
- () => { scrollBehaviorDuringMove = cursorElement._scrollBehavior; });
+ () => { scrollBehaviorDuringMove = cursorElement._scrollMode; });
function renderHandler() {
diffElement.removeEventListener('render', renderHandler);
@@ -256,7 +256,7 @@
assert.isFalse(moveToNumStub.called);
assert.isTrue(moveToChunkStub.called);
assert.equal(scrollBehaviorDuringMove, 'never');
- assert.equal(cursorElement._scrollBehavior, 'keep-visible');
+ assert.equal(cursorElement._scrollMode, 'keep-visible');
done();
}
diffElement.addEventListener('render', renderHandler);
@@ -266,7 +266,7 @@
test('initialLineNumber provided', done => {
let scrollBehaviorDuringMove;
const moveToNumStub = sandbox.stub(cursorElement, 'moveToLineNumber',
- () => { scrollBehaviorDuringMove = cursorElement._scrollBehavior; });
+ () => { scrollBehaviorDuringMove = cursorElement._scrollMode; });
const moveToChunkStub = sandbox.stub(cursorElement, 'moveToFirstChunk');
function renderHandler() {
diffElement.removeEventListener('render', renderHandler);
@@ -276,7 +276,7 @@
assert.equal(moveToNumStub.lastCall.args[0], 10);
assert.equal(moveToNumStub.lastCall.args[1], 'right');
assert.equal(scrollBehaviorDuringMove, 'keep-visible');
- assert.equal(cursorElement._scrollBehavior, 'keep-visible');
+ assert.equal(cursorElement._scrollMode, 'keep-visible');
done();
}
diffElement.addEventListener('render', renderHandler);
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_html.js b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_html.js
index b31af73..fecaa73 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_html.js
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_html.js
@@ -95,7 +95,7 @@
id="cursor"
index="{{index}}"
cursor-target-class="selected"
- scroll-behavior="never"
+ scroll-mode="never"
focus-on-move=""
stops="[[_suggestionEls]]"
></gr-cursor-manager>
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 2901ea6..7fa1f19 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
@@ -18,11 +18,7 @@
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin.js';
import {PolymerElement} from '@polymer/polymer/polymer-element.js';
import {htmlTemplate} from './gr-cursor-manager_html.js';
-
-const ScrollBehavior = {
- NEVER: 'never',
- KEEP_VISIBLE: 'keep-visible',
-};
+import {ScrollModes} from '../../../constants/constants.js';
/** @extends PolymerElement */
class GrCursorManager extends GestureEventListeners(
@@ -81,9 +77,9 @@
*
* @type {string|undefined}
*/
- scrollBehavior: {
+ scrollMode: {
type: String,
- value: ScrollBehavior.NEVER,
+ value: ScrollModes.NEVER,
},
/**
@@ -213,8 +209,8 @@
setCursor(element, opt_noScroll) {
let behavior;
if (opt_noScroll) {
- behavior = this.scrollBehavior;
- this.scrollBehavior = ScrollBehavior.NEVER;
+ behavior = this.scrollMode;
+ this.scrollMode = ScrollModes.NEVER;
}
this.unsetCursor();
@@ -222,7 +218,7 @@
this._updateIndex();
this._decorateTarget();
- if (opt_noScroll) { this.scrollBehavior = behavior; }
+ if (opt_noScroll) { this.scrollMode = behavior; }
}
unsetCursor() {
@@ -390,7 +386,7 @@
*/
_targetIsVisible(top) {
const dims = this._getWindowDims();
- return this.scrollBehavior === ScrollBehavior.KEEP_VISIBLE &&
+ return this.scrollMode === ScrollModes.KEEP_VISIBLE &&
top > (dims.pageYOffset + this.scrollTopMargin) &&
top < dims.pageYOffset + dims.innerHeight;
}
@@ -402,7 +398,7 @@
}
_scrollToTarget() {
- if (!this.target || this.scrollBehavior === ScrollBehavior.NEVER) {
+ if (!this.target || this.scrollMode === ScrollModes.NEVER) {
return;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.html b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.html
index 98a7d24..6059e76 100644
--- a/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager_test.html
@@ -178,7 +178,7 @@
sandbox.stub(element, '_targetIsVisible', () => false);
const scrollStub = sandbox.stub(window, 'scrollTo');
element.stops = list.querySelectorAll('li');
- element.scrollBehavior = 'keep-visible';
+ element.scrollMode = 'keep-visible';
element.setCursorAtIndex(1, true);
assert.isFalse(scrollStub.called);
@@ -236,7 +236,7 @@
let scrollStub;
setup(() => {
element.stops = list.querySelectorAll('li');
- element.scrollBehavior = 'keep-visible';
+ element.scrollMode = 'keep-visible';
// There is a target which has a targetNext
element.setCursor(list.children[0]);
diff --git a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_html.js b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_html.js
index d0b0d09..6617a0e 100644
--- a/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_html.js
+++ b/polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_html.js
@@ -161,7 +161,7 @@
<gr-cursor-manager
id="cursor"
cursor-target-class="selected"
- scroll-behavior="never"
+ scroll-mode="never"
focus-on-move=""
stops="[[_listElements]]"
></gr-cursor-manager>
