Make reinitializing cursor explicit

Initializing the cursor potentially does disruptive things like stealing
the cursor and scrolling the page - the behavior to do this on every
render should not be baked into gr-diff(-cursor). Instead, it is better
if pages that want this behavior trigger it explicitly.

The list view already does that (by calling `handleDiffUpdate()`)
when its done rendering all diffs.

This change
* makes gr-diff-view ("the file page") call reInitCursor explicitly when
  done  rendering
* removes that behavior from the reusable gr-diff-cursor

One positive side-effect is that on the list view, we have less
duplicate cursor initializations (after every diff renders, and then
again when they are all done rendering).

Change-Id: I04602bec25af753ec923914db4b5be8b33a45449
This commit is contained in:
Ole Rehmsen
2020-03-31 11:05:11 +02:00
parent 697b7900a5
commit 55dca5fe3a
3 changed files with 16 additions and 5 deletions

View File

@@ -309,7 +309,7 @@ class GrDiffCursor extends mixinBehaviors([Gerrit.FireBehavior],
}
_handleDiffRenderContent() {
this.handleDiffUpdate();
this._updateStops();
// When done rendering, turn focus on move and automatic scrolling back on
this._focusOnMove = true;
this._preventAutoScrollOnManualScroll = false;

View File

@@ -120,8 +120,9 @@ suite('gr-diff-cursor tests', () => {
});
test('cursor scroll behavior', () => {
cursorElement._handleDiffRenderStart();
assert.equal(cursorElement._scrollBehavior, 'keep-visible');
cursorElement._handleDiffRenderStart();
assert.isTrue(cursorElement._focusOnMove);
cursorElement._handleWindowScroll();
@@ -129,8 +130,10 @@ suite('gr-diff-cursor tests', () => {
assert.isFalse(cursorElement._focusOnMove);
cursorElement._handleDiffRenderContent();
assert.equal(cursorElement._scrollBehavior, 'keep-visible');
assert.isTrue(cursorElement._focusOnMove);
cursorElement.reInitCursor();
assert.equal(cursorElement._scrollBehavior, 'keep-visible');
});
suite('unified diff', () => {
@@ -238,6 +241,7 @@ suite('gr-diff-cursor tests', () => {
function renderHandler() {
diffElement.removeEventListener('render', renderHandler);
cursorElement.reInitCursor();
assert.isFalse(moveToNumStub.called);
assert.isTrue(moveToChunkStub.called);
assert.equal(scrollBehaviorDuringMove, 'never');
@@ -255,6 +259,7 @@ suite('gr-diff-cursor tests', () => {
const moveToChunkStub = sandbox.stub(cursorElement, 'moveToFirstChunk');
function renderHandler() {
diffElement.removeEventListener('render', renderHandler);
cursorElement.reInitCursor();
assert.isFalse(moveToChunkStub.called);
assert.isTrue(moveToNumStub.called);
assert.equal(moveToNumStub.lastCall.args[0], 10);
@@ -382,11 +387,11 @@ suite('gr-diff-cursor tests', () => {
});
test('expand context updates stops', done => {
sandbox.spy(cursorElement, 'handleDiffUpdate');
sandbox.spy(cursorElement, '_updateStops');
MockInteractions.tap(diffElement.shadowRoot
.querySelector('.showContext'));
flush(() => {
assert.isTrue(cursorElement.handleDiffUpdate.called);
assert.isTrue(cursorElement._updateStops.called);
done();
});
});

View File

@@ -314,6 +314,12 @@ class GrDiffView extends mixinBehaviors( [
this.addEventListener('open-fix-preview',
this._onOpenFixPreview.bind(this));
this.$.cursor.push('diffs', this.$.diffHost);
const onRender = () => {
this.$.diffHost.removeEventListener('render', onRender);
this.$.cursor.reInitCursor();
};
this.$.diffHost.addEventListener('render', onRender);
}
_getLoggedIn() {