Properly bookend file-list hotkey scrolling
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
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index f607481..3142733 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -21,6 +21,7 @@
<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 @@
.row:not(.header):hover {
background-color: #f5fafd;
}
- .row[selected] {
+ .row.selected {
background-color: #ebf5fb;
}
.path {
@@ -145,7 +146,7 @@
max-width: 25em;
}
@media screen and (max-width: 50em) {
- .row[selected] {
+ .row.selected {
background-color: transparent;
}
.stats {
@@ -207,7 +208,7 @@
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 @@
</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>
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 3f13494..b423f50 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
@@ -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);
},
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
index b2c69a7..bc7c0c7 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -279,7 +279,7 @@
basePatchNum: 'PARENT',
patchNum: '2',
};
- element.selectedIndex = 0;
+ element.$.fileCursor.setCursorAtIndex(0);
});
test('toggle left diff via shortcut', function() {
@@ -298,24 +298,26 @@
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 @@
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 @@
basePatchNum: 'PARENT',
patchNum: '2',
};
- element.selectedIndex = 0;
+ element.$.fileCursor.setCursorAtIndex(0);
flushAsynchronousOperations();
var fileRows =
@@ -499,7 +501,7 @@
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 @@
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 @@
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);
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 c8eea3c..2d0786a 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="[[_scrollBehavior]]"
+ scroll-behavior="[[_scrollBehavior]]"
cursor-target-class="target-row"
target="{{diffRow}}"></gr-cursor-manager>
</template>
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 7b3bc23..81f5186 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
@@ -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; }
