Fix jumping scroll position when diff view loads Previously, there was an issue where if you started scrolling on a diff view page before everything was loaded, the page would jump back to the top after loading finished. This change temporarily adjusts scroll behavior when scrolling is detected mid-diff load. The scroll behavior is then restored after loading completes. Bug: Issue 4411 Change-Id: I8175d433632fd8710f1353f7ba5635b826151ce0
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html index 40a90b7..e77ef21 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -62,6 +62,12 @@ is: 'gr-diff-builder', /** + * Fired when the diff begins rendering. + * + * @event render-start + */ + + /** * Fired when the diff is rendered. * * @event render @@ -121,6 +127,7 @@ reporting.time(TimingLabel.TOTAL); reporting.time(TimingLabel.CONTENT); + this.fire('render-start'); return this.$.processor.process(this.diff.content).then(function() { if (this.isImageDiff) { this._builder.renderDiffImages();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html index af44629..905c65b 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -613,6 +613,16 @@ assert.strictEqual(sections[0], section[0]); assert.strictEqual(sections[1], section[1]); }); + + test('render-start and render are fired', function() { + var fireStub = sinon.stub(element, 'fire'); + element.render({left: [], right: []}, {}); + flush(function() { + assert.isTrue(fireStub.calledWithExactly('render-start')); + assert.isTrue(fireStub.calledWithExactly('render')); + done(); + }); + }); }); suite('mock-diff', function() {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.html b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.html index 491eded..c8eea3c 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.html
@@ -21,7 +21,7 @@ <template> <gr-cursor-manager id="cursorManager" - scroll="keep-visible" + scroll="[[_scrollBehavior]]" cursor-target-class="target-row" target="{{diffRow}}"></gr-cursor-manager> </template>
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 dd11f2c..e783658 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
@@ -24,6 +24,11 @@ UNIFIED: 'UNIFIED_DIFF', }; + var ScrollBehavior = { + KEEP_VISIBLE: 'keep-visible', + NEVER: 'never', + }; + var LEFT_SIDE_CLASS = 'target-side-left'; var RIGHT_SIDE_CLASS = 'target-side-right'; @@ -63,6 +68,18 @@ type: Number, value: null, }, + + /** + * The scroll behavior for the cursor. Values are 'never' and + * 'keep-visible'. 'keep-visible' will only scroll if the cursor is beyond + * the viewport. + */ + _scrollBehavior: { + type: String, + value: ScrollBehavior.KEEP_VISIBLE, + }, + + _listeningForScroll: Boolean, }, observers: [ @@ -70,6 +87,15 @@ '_diffsChanged(diffs.splices)', ], + attached: function() { + // Catch when users are scrolling as the view loads. + this.listen(window, 'scroll', '_handleWindowScroll'); + }, + + detached: function() { + this.unlisten(window, 'scroll', '_handleWindowScroll'); + }, + moveLeft: function() { this.side = DiffSides.LEFT; if (this._isTargetBlank()) { @@ -169,12 +195,25 @@ } }, + _handleWindowScroll: function() { + if (this._listeningForScroll) { + this._scrollBehavior = ScrollBehavior.NEVER; + this._listeningForScroll = false; + } + }, + handleDiffUpdate: function() { this._updateStops(); if (!this.diffRow) { this.reInitCursor(); } + this._scrollBehavior = ScrollBehavior.KEEP_VISIBLE; + this._listeningForScroll = false; + }, + + _handleDiffRenderStart: function() { + this._listeningForScroll = true; }, /** @@ -320,12 +359,15 @@ for (i = splice.index; i < splice.index + splice.addedCount; i++) { + this.listen(this.diffs[i], 'render-start', '_handleDiffRenderStart'); this.listen(this.diffs[i], 'render', 'handleDiffUpdate'); } for (i = 0; i < splice.removed && splice.removed.length; i++) { + this.unlisten(splice.removed[i], + 'render-start', '_handleDiffRenderStart'); this.unlisten(splice.removed[i], 'render', 'handleDiffUpdate'); } }
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 5bdd138..a7d98e6 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
@@ -98,6 +98,17 @@ assert.equal(cursorElement.diffRow, firstDeltaRow); }); + test('cursor scroll behavior', function() { + cursorElement._handleDiffRenderStart(); + assert.equal(cursorElement._scrollBehavior, 'keep-visible'); + + cursorElement._handleWindowScroll(); + assert.equal(cursorElement._scrollBehavior, 'never'); + + cursorElement.handleDiffUpdate(); + assert.equal(cursorElement._scrollBehavior, 'keep-visible'); + }); + suite('unified diff', function() { setup(function(done) {