opendev/gerrit
Code Issues Proposed changes

Prevent cursor from scrolling if multiple files are being rendered

Browse Source 
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
This commit is contained in:
Dhruv Srivastava
2020-05-05 13:48:25 +02:00
parent 697a5ec4af
commit 9c853fc1fa
11 changed files with 57 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
polygerrit-ui/app/constants/constants.js
View File

@@ -42,4 +42,16 @@ export const MessageTags = {
TAG_NEW_PATCHSET: 'autogenerated:gerrit:newPatchSet',
TAG_NEW_WIP_PATCHSET: 'autogenerated:gerrit:newWipPatchSet',
TAG_REVIEWER_UPDATE: 'autogenerated:gerrit:reviewerUpdate',
};
};
/**
* @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',
};

2
polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.js
View File

@@ -138,7 +138,7 @@ export const htmlTemplate = html`
<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>

10
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
View File

@@ -1195,7 +1195,15 @@ class GrFileList extends mixinBehaviors( [
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();
}));
}

2
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.js
View File

@@ -584,7 +584,7 @@ export const htmlTemplate = html`
<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>

25
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
View File

@@ -22,6 +22,7 @@ import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-l
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 @@ const DiffViewMode = {
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 @@ class GrDiffCursor extends GestureEventListeners(
* '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 @@ class GrDiffCursor extends GestureEventListeners(
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 @@ class GrDiffCursor extends GestureEventListeners(
}
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();

2
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_html.js
View File

@@ -19,7 +19,7 @@ import {html} from '@polymer/polymer/lib/utils/html-tag.js';
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}}"

14
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
View File

@@ -117,20 +117,20 @@ suite('gr-diff-cursor tests', () => {
});
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 @@ suite('gr-diff-cursor tests', () => {
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 @@ suite('gr-diff-cursor tests', () => {
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 @@ suite('gr-diff-cursor tests', () => {
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 @@ suite('gr-diff-cursor tests', () => {
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);

2
polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_html.js
View File

@@ -95,7 +95,7 @@ export const htmlTemplate = html`
id="cursor"
index="{{index}}"
cursor-target-class="selected"
scroll-behavior="never"
scroll-mode="never"
focus-on-move=""
stops="[[_suggestionEls]]"
></gr-cursor-manager>

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

@@ -18,11 +18,7 @@ import {GestureEventListeners} from '@polymer/polymer/lib/mixins/gesture-event-l
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 @@ class GrCursorManager extends GestureEventListeners(
*
* @type {string|undefined}
*/
scrollBehavior: {
scrollMode: {
type: String,
value: ScrollBehavior.NEVER,
value: ScrollModes.NEVER,
},
/**
@@ -213,8 +209,8 @@ class GrCursorManager extends GestureEventListeners(
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 @@ class GrCursorManager extends GestureEventListeners(
this._updateIndex();
this._decorateTarget();
if (opt_noScroll) { this.scrollBehavior = behavior; }
if (opt_noScroll) { this.scrollMode = behavior; }
}
unsetCursor() {
@@ -390,7 +386,7 @@ class GrCursorManager extends GestureEventListeners(
*/
_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 @@ class GrCursorManager extends GestureEventListeners(
}
_scrollToTarget() {
if (!this.target || this.scrollBehavior === ScrollBehavior.NEVER) {
if (!this.target || this.scrollMode === ScrollModes.NEVER) {
return;
}

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

@@ -178,7 +178,7 @@ suite('gr-cursor-manager tests', () => {
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 @@ suite('gr-cursor-manager tests', () => {
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]);

2
polygerrit-ui/app/elements/shared/gr-dropdown/gr-dropdown_html.js
View File

@@ -161,7 +161,7 @@ export const htmlTemplate = html`
<gr-cursor-manager
id="cursor"
cursor-target-class="selected"
scroll-behavior="never"
scroll-mode="never"
focus-on-move=""
stops="[[_listElements]]"
></gr-cursor-manager>