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`