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) {
