opendev/gerrit
Code Issues Proposed changes

Properly bookend file-list hotkey scrolling

Browse Source 
The addition of incremental file loading in the file-list introduced an
edge case in which the user could scroll off the file-list with hotkeys.
This change implements the gr-cursor-manager to control file-list
scrolling behavior.

In addition, some interesting edge cases with the cursor-manager were
uncovered during use -- these were also fixed.

Change-Id: I936d4368058212c98684bc2617be2d98bdf6e41d
This commit is contained in:
Kasper Nilsson
2016-11-21 14:11:11 -08:00
parent c04343eaad
commit 034f79b9f4
5 changed files with 66 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
View File

@@ -21,6 +21,7 @@ limitations under the License.
<link rel="import" href="../../diff/gr-diff/gr-diff.html">
<link rel="import" href="../../diff/gr-diff-cursor/gr-diff-cursor.html">
<link rel="import" href="../../shared/gr-button/gr-button.html">
<link rel="import" href="../../shared/gr-cursor-manager/gr-cursor-manager.html">
<link rel="import" href="../../shared/gr-linked-text/gr-linked-text.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../shared/gr-select/gr-select.html">
@@ -64,7 +65,7 @@ limitations under the License.
.row:not(.header):hover {
background-color: #f5fafd;
}
.row[selected] {
.row.selected {
background-color: #ebf5fb;
}
.path {
@@ -145,7 +146,7 @@ limitations under the License.
max-width: 25em;
}
@media screen and (max-width: 50em) {
.row[selected] {
.row.selected {
background-color: transparent;
}
.stats {
@@ -207,7 +208,7 @@ limitations under the License.
items="[[_shownFiles]]"
as="file"
initial-count="[[_fileListIncrement]]">
<div class="row" selected$="[[_computeFileSelected(index, selectedIndex)]]">
<div class="file-row row" selected$="[[_computeFileSelected(index, selectedIndex)]]">
<div class="reviewed" hidden$="[[!_loggedIn]]" hidden>
<input type="checkbox" checked$="[[_computeReviewed(file, _reviewed)]]"
data-path$="[[file.__path]]" on-change="_handleReviewedChange"
@@ -302,7 +303,11 @@ limitations under the License.
</gr-button>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="storage"></gr-storage>
<gr-diff-cursor id="cursor"></gr-diff-cursor>
<gr-diff-cursor id="diffCursor"></gr-diff-cursor>
<gr-cursor-manager
id="fileCursor"
scroll-behavior="keep-visible"
cursor-target-class="selected"></gr-cursor-manager>
</template>
<script src="gr-file-list.js"></script>
</dom-module>

72
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
View File

@@ -35,10 +35,6 @@
drafts: Object,
revisions: Object,
projectConfig: Object,
selectedIndex: {
type: Number,
notify: true,
},
keyEventTarget: {
type: Object,
value: function() { return document.body; },
@@ -253,7 +249,7 @@
this.set(['_shownFiles', i, '__expanded'], false);
this.set(['_files', i, '__expanded'], false);
}
this.$.cursor.handleDiffUpdate();
this.$.diffCursor.handleDiffUpdate();
},
_computeCommentsString: function(comments, patchNum, path) {
@@ -326,7 +322,7 @@
if (!this._showInlineDiffs) { return; }
e.preventDefault();
this.$.cursor.moveLeft();
this.$.diffCursor.moveLeft();
},
_handleShiftRightKey: function(e) {
@@ -334,20 +330,20 @@
if (!this._showInlineDiffs) { return; }
e.preventDefault();
this.$.cursor.moveRight();
this.$.diffCursor.moveRight();
},
_handleIKey: function(e) {
if (this.shouldSuppressKeyboardShortcut(e)) { return; }
if (this.selectedIndex === undefined) { return; }
if (this.$.fileCursor.index === -1) { return; }
e.preventDefault();
var expanded = this._files[this.selectedIndex].__expanded;
var expanded = this._files[this.$.fileCursor.index].__expanded;
// Until Polymer 2.0, manual management of reflection between _files
// and _shownFiles is necessary.
this.set(['_shownFiles', this.selectedIndex, '__expanded'],
this.set(['_shownFiles', this.$.fileCursor.index, '__expanded'],
!expanded);
this.set(['_files', this.selectedIndex, '__expanded'], !expanded);
this.set(['_files', this.$.fileCursor.index, '__expanded'], !expanded);
},
_handleCapitalIKey: function(e) {
@@ -359,14 +355,11 @@
_handleDownKey: function(e) {
if (this.shouldSuppressKeyboardShortcut(e)) { return; }
e.preventDefault();
if (this._showInlineDiffs) {
this.$.cursor.moveDown();
this.$.diffCursor.moveDown();
} else {
this.selectedIndex =
Math.min(this._numFilesShown, this.selectedIndex + 1);
this._scrollToSelectedFile();
this.$.fileCursor.next();
}
},
@@ -375,10 +368,9 @@
e.preventDefault();
if (this._showInlineDiffs) {
this.$.cursor.moveUp();
this.$.diffCursor.moveUp();
} else {
this.selectedIndex = Math.max(0, this.selectedIndex - 1);
this._scrollToSelectedFile();
this.$.fileCursor.previous();
}
},
@@ -425,9 +417,9 @@
e.preventDefault();
if (e.shiftKey) {
this.$.cursor.moveToNextCommentThread();
this.$.diffCursor.moveToNextCommentThread();
} else {
this.$.cursor.moveToNextChunk();
this.$.diffCursor.moveToNextChunk();
}
},
@@ -437,9 +429,9 @@
e.preventDefault();
if (e.shiftKey) {
this.$.cursor.moveToPreviousCommentThread();
this.$.diffCursor.moveToPreviousCommentThread();
} else {
this.$.cursor.moveToPreviousChunk();
this.$.diffCursor.moveToPreviousChunk();
}
},
@@ -461,43 +453,27 @@
},
_openCursorFile: function() {
var diff = this.$.cursor.getTargetDiffElement();
var diff = this.$.diffCursor.getTargetDiffElement();
page.show(this._computeDiffURL(diff.changeNum, diff.patchRange,
diff.path));
},
_openSelectedFile: function(opt_index) {
if (opt_index != null) {
this.selectedIndex = opt_index;
this.$.fileCursor.setCursorAtIndex(opt_index);
}
page.show(this._computeDiffURL(this.changeNum, this.patchRange,
this._files[this.selectedIndex].__path));
this._files[this.$.fileCursor.index].__path));
},
_addDraftAtTarget: function() {
var diff = this.$.cursor.getTargetDiffElement();
var target = this.$.cursor.getTargetLineElement();
var diff = this.$.diffCursor.getTargetDiffElement();
var target = this.$.diffCursor.getTargetLineElement();
if (diff && target) {
diff.addDraftAtLine(target);
}
},
_scrollToSelectedFile: function() {
var el = this.$$('.row[selected]');
var top = 0;
for (var node = el; node; node = node.offsetParent) {
top += node.offsetTop;
}
// Don't scroll if it's already in view.
if (top > window.pageYOffset &&
top < window.pageYOffset + window.innerHeight - el.clientHeight) {
return;
}
window.scrollTo(0, top - document.body.clientHeight / 2);
},
_shouldHideChangeTotals: function(_patchChange) {
return _patchChange.inserted === 0 && _patchChange.deleted === 0;
},
@@ -576,8 +552,12 @@
var diffElements = Polymer.dom(this.root).querySelectorAll('gr-diff');
// Overwrite the cursor's list of diffs:
this.$.cursor.splice.apply(this.$.cursor,
['diffs', 0, this.$.cursor.diffs.length].concat(diffElements));
this.$.diffCursor.splice.apply(this.$.diffCursor,
['diffs', 0, this.$.diffCursor.diffs.length].concat(diffElements));
var files = Polymer.dom(this.root).querySelectorAll('.file-row');
this.$.fileCursor.stops = files;
if (this.$.fileCursor.index === -1) { this.$.fileCursor.moveToStart(); }
}.bind(this), 1);
},

40
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
View File

@@ -279,7 +279,7 @@ limitations under the License.
basePatchNum: 'PARENT',
patchNum: '2',
};
element.selectedIndex = 0;
element.$.fileCursor.setCursorAtIndex(0);
});
test('toggle left diff via shortcut', function() {
@@ -298,24 +298,26 @@ limitations under the License.
test('keyboard shortcuts', function() {
flushAsynchronousOperations();
var elementItems = Polymer.dom(element.root).querySelectorAll(
'.row:not(.header)');
assert.equal(elementItems.length, 5);
assert.isTrue(elementItems[0].hasAttribute('selected'));
assert.isFalse(elementItems[1].hasAttribute('selected'));
assert.isFalse(elementItems[2].hasAttribute('selected'));
var items = Polymer.dom(element.root).querySelectorAll('.file-row');
element.$.fileCursor.stops = items;
element.$.fileCursor.setCursorAtIndex(0);
assert.equal(items.length, 3);
assert.isTrue(items[0].classList.contains('selected'));
assert.isFalse(items[1].classList.contains('selected'));
assert.isFalse(items[2].classList.contains('selected'));
MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
assert.equal(element.selectedIndex, 1);
assert.equal(element.$.fileCursor.index, 1);
MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
var showStub = sandbox.stub(page, 'show');
assert.equal(element.selectedIndex, 2);
assert.equal(element.$.fileCursor.index, 2);
MockInteractions.pressAndReleaseKeyOn(element, 13, null, 'enter');
assert(showStub.lastCall.calledWith('/c/42/2/myfile.txt'),
'Should navigate to /c/42/2/myfile.txt');
MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
assert.equal(element.selectedIndex, 1);
assert.equal(element.$.fileCursor.index, 1);
MockInteractions.pressAndReleaseKeyOn(element, 79, null, 'o');
assert(showStub.lastCall.calledWith('/c/42/2/file_added_in_rev2.txt'),
'Should navigate to /c/42/2/file_added_in_rev2.txt');
@@ -323,20 +325,20 @@ limitations under the License.
MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
MockInteractions.pressAndReleaseKeyOn(element, 75, null, 'k');
assert.equal(element.selectedIndex, 0);
showStub.restore();
assert.equal(element.$.fileCursor.index, 0);
});
test('i key shows/hides selected inline diff', function() {
element.selectedIndex = 0;
flushAsynchronousOperations();
element.$.fileCursor.stops = element.diffs;
element.$.fileCursor.setCursorAtIndex(0);
MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i');
flushAsynchronousOperations();
assert.isFalse(element.diffs[0].hasAttribute('hidden'));
MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i');
flushAsynchronousOperations();
assert.isTrue(element.diffs[0].hasAttribute('hidden'));
element.selectedIndex = 1;
element.$.fileCursor.setCursorAtIndex(1);
MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i');
flushAsynchronousOperations();
assert.isFalse(element.diffs[1].hasAttribute('hidden'));
@@ -416,7 +418,7 @@ limitations under the License.
basePatchNum: 'PARENT',
patchNum: '2',
};
element.selectedIndex = 0;
element.$.fileCursor.setCursorAtIndex(0);
flushAsynchronousOperations();
var fileRows =
@@ -499,7 +501,7 @@ limitations under the License.
basePatchNum: 'PARENT',
patchNum: '2',
};
element.selectedIndex = 0;
element.$.fileCursor.setCursorAtIndex(0);
flushAsynchronousOperations();
var fileRows =
Polymer.dom(element.root).querySelectorAll('.row:not(.header)');
@@ -537,7 +539,7 @@ limitations under the License.
basePatchNum: 'PARENT',
patchNum: '2',
};
element.selectedIndex = 0;
element.$.fileCursor.setCursorAtIndex(0);
flushAsynchronousOperations();
var diffDisplay = element.diffs[0];
element._userPrefs = {diff_view: 'SIDE_BY_SIDE'};
@@ -580,7 +582,7 @@ limitations under the License.
patchNum: '2',
};
var computeSpy = sandbox.spy(element, '_fileListActionsVisible');
element.selectedIndex = 0;
element.$.fileCursor.setCursorAtIndex(0);
element._numFilesShown = 1;
flush(function() {
assert.isTrue(computeSpy.lastCall.returnValue);

2
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.html
View File

@@ -21,7 +21,7 @@ limitations under the License.
<template>
<gr-cursor-manager
id="cursorManager"
scroll="[[_scrollBehavior]]"
scroll-behavior="[[_scrollBehavior]]"
cursor-target-class="target-row"
target="{{diffRow}}"></gr-cursor-manager>
</template>

12
polygerrit-ui/app/elements/shared/gr-cursor-manager/gr-cursor-manager.js
View File

@@ -59,7 +59,7 @@
* 'keep-visible'. 'keep-visible' will only scroll if the cursor is beyond
* the viewport.
*/
scroll: {
scrollBehavior: {
type: String,
value: ScrollBehavior.NEVER,
},
@@ -108,6 +108,10 @@
}
},
setCursorAtIndex: function(index) {
this.setCursor(this.stops[index]);
},
/**
* Move the cursor forward or backward by delta. Noop if moving past either
* end of the stop list.
@@ -194,7 +198,9 @@
},
_scrollToTarget: function() {
if (!this.target || this.scroll === ScrollBehavior.NEVER) { return; }
if (!this.target || this.scrollBehavior === ScrollBehavior.NEVER) {
return;
}
// Calculate where the element is relative to the window.
var top = this.target.offsetTop;
@@ -204,7 +210,7 @@
top += offsetParent.offsetTop;
}
if (this.scroll === ScrollBehavior.KEEP_VISIBLE &&
if (this.scrollBehavior === ScrollBehavior.KEEP_VISIBLE &&
top > window.pageYOffset &&
top < window.pageYOffset + window.innerHeight) { return; }