From 9c853fc1fabc1eb621142567e6329e2cb623aaeb Mon Sep 17 00:00:00 2001 From: Dhruv Srivastava Date: Tue, 5 May 2020 13:48:25 +0200 Subject: [PATCH] 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 --- polygerrit-ui/app/constants/constants.js | 14 ++++++++++- .../gr-change-list/gr-change-list_html.js | 2 +- .../change/gr-file-list/gr-file-list.js | 10 +++++++- .../change/gr-file-list/gr-file-list_html.js | 2 +- .../diff/gr-diff-cursor/gr-diff-cursor.js | 25 ++++++++++--------- .../gr-diff-cursor/gr-diff-cursor_html.js | 2 +- .../gr-diff-cursor/gr-diff-cursor_test.html | 14 +++++------ .../gr-autocomplete-dropdown_html.js | 2 +- .../gr-cursor-manager/gr-cursor-manager.js | 20 ++++++--------- .../gr-cursor-manager_test.html | 4 +-- .../shared/gr-dropdown/gr-dropdown_html.js | 2 +- 11 files changed, 57 insertions(+), 40 deletions(-) diff --git a/polygerrit-ui/app/constants/constants.js b/polygerrit-ui/app/constants/constants.js index 084bb0f4b4..5001dd2e71 100644 --- a/polygerrit-ui/app/constants/constants.js +++ b/polygerrit-ui/app/constants/constants.js @@ -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', -}; \ 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 17150df0f8..e5193f8b07 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 @@ export const htmlTemplate = html` 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 24daeabc6f..9f93e21a7d 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 @@ 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(); })); } 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 f1010bff47..42e07f5038 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 @@ export const htmlTemplate = html` 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 444fad42e8..b12bd9ba97 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 {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(); 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 1ac47f6980..e400792f26 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 @@ import {html} from '@polymer/polymer/lib/utils/html-tag.js'; export const htmlTemplate = html` { }); 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); 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 b31af73188..fecaa73419 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 @@ export const htmlTemplate = html` id="cursor" index="{{index}}" cursor-target-class="selected" - scroll-behavior="never" + scroll-mode="never" focus-on-move="" stops="[[_suggestionEls]]" > 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 2901ea6cda..7fa1f19362 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 {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; } 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 98a7d24928..6059e76e11 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 @@ 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]); 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 d0b0d099f3..6617a0eb5a 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 @@ export const htmlTemplate = html`