opendev/gerrit
Code Issues Proposed changes

Some keyboard shortcuts for change list/change view/diff

Browse Source 
This implements the minimum required keyboard shortcuts for
the MVP. All other changes are made to support these
shortcuts working properly.

The reasoning behind moving to iron-a11y-keys-behavior is
that the iron-a11y-keys element didn't support [ and ] as
keyboard shortcuts (oddly).

Feature: Issue 3647

Change-Id: I6473a962811c19f78ba4f6829e644d7b3cbeffc7
This commit is contained in:
Andrew Bonventre
2015-12-01 01:02:00 -05:00
parent 5711981711
commit 547b8ab9d0
11 changed files with 268 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
lib/js/BUCK
View File

@@ -68,25 +68,13 @@ npm_binary(
# Use the same procedure as for adding dependencies, except just change the
# version number of the existing bower_component rather than adding a new rule.
bower_component(
name = 'iron-a11y-keys',
package = 'polymerelements/iron-a11y-keys',
version = '1.0.3',
deps = [
':iron-a11y-keys-behavior',
':polymer',
],
license = 'polymer',
sha1 = 'cf7fdf9ffa3349d28632fc3e86b84300d1439e29',
)
bower_component(
name = 'iron-a11y-keys-behavior',
package = 'polymerelements/iron-a11y-keys-behavior',
version = '1.0.8',
package = 'PolymerElements/iron-a11y-keys-behavior',
version = '1.1.0',
deps = [':polymer'],
license = 'polymer',
sha1 = '2f1ea0b4542e2949de195dff5cbe02b7cb953eff',
sha1 = '0b7962ed8409336652da4b4e83d052dbe53d4e1a',
)
bower_component(

2
polygerrit-ui/BUCK
View File

@@ -6,7 +6,7 @@ bower_components(
'//lib/js:polymer',
'//lib/js:page',
'//lib/js:iron-ajax',
'//lib/js:iron-a11y-keys',
'//lib/js:iron-a11y-keys-behavior',
'//lib/js:iron-input',
],
)

2
polygerrit-ui/app/elements/gr-account-dropdown.html
View File

@@ -44,7 +44,7 @@ limitations under the License.
_loginTapHandler: function(e) {
e.preventDefault();
page('/login/' + encodeURIComponent(
page.show('/login/' + encodeURIComponent(
window.location.pathname + window.location.hash));
},

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

@@ -15,7 +15,7 @@ limitations under the License.
-->
<link rel="import" href="../bower_components/polymer/polymer.html">
<link rel="import" href="../bower_components/iron-a11y-keys/iron-a11y-keys.html">
<link rel="import" href="../bower_components/iron-a11y-keys-behavior/iron-a11y-keys-behavior.html">
<link rel="import" href="gr-change-list-item.html">
<dom-module id="gr-change-list">
@@ -26,12 +26,6 @@ limitations under the License.
border-collapse: collapse;
}
</style>
<iron-a11y-keys
target="[[keyTarget]]"
keys="j k enter"
on-keys-pressed="_handleKey"></iron-a11y-keys>
<gr-change-list-item header></gr-change-list-item>
<template is="dom-repeat" items="{{changes}}" as="change">
<gr-change-list-item change="[[change]]"
@@ -46,6 +40,10 @@ limitations under the License.
Polymer({
is: 'gr-change-list',
behaviors: [
Polymer.IronA11yKeysBehavior
],
hostAttributes: {
tabindex: 0,
},
@@ -56,7 +54,7 @@ limitations under the License.
* https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-info
*/
changes: Array,
keyTarget: {
keyEventTarget: {
type: Object,
value: function() {
return document.body;
@@ -69,6 +67,10 @@ limitations under the License.
},
},
keyBindings: {
'j k o enter': '_handleKey',
},
_isSelected: function(index) {
return index == this.selectedIndex;
},
@@ -92,8 +94,9 @@ limitations under the License.
if (this.selectedIndex == 0) { return; }
this.selectedIndex -= 1;
break;
case 'o':
case 'enter':
page(this._changeURLForIndex(this.selectedIndex));
page.show(this._changeURLForIndex(this.selectedIndex));
break;
}
},

30
polygerrit-ui/app/elements/gr-change-view.html
View File

@@ -15,6 +15,7 @@ limitations under the License.
-->
<link rel="import" href="../bower_components/polymer/polymer.html">
<link rel="import" href="../bower_components/iron-a11y-keys-behavior/iron-a11y-keys-behavior.html">
<link rel="import" href="../bower_components/iron-ajax/iron-ajax.html">
<link rel="import" href="gr-date-formatter.html">
<link rel="import" href="gr-file-list.html">
@@ -133,7 +134,17 @@ limitations under the License.
Polymer({
is: 'gr-change-view',
behaviors: [
Polymer.IronA11yKeysBehavior
],
properties: {
keyEventTarget: {
type: Object,
value: function() {
return document.body;
},
},
/**
* URL params passed from the router.
*/
@@ -145,6 +156,10 @@ limitations under the License.
changeNum: Number,
},
keyBindings: {
'a u': '_handleKey',
},
_paramsChanged: function(value) {
this.changeNum = value.changeNum;
if (!this.changeNum) {
@@ -197,6 +212,21 @@ limitations under the License.
});
},
_handleKey: function(e) {
switch(e.detail.combo) {
case 'a':
this._showReplyField();
break;
case 'u':
page.show('/');
break;
}
},
_showReplyField: function() {
// TODO(andybons): Implement reply field.
},
});
})();
</script>

84
polygerrit-ui/app/elements/gr-diff-view.html
View File

@@ -15,6 +15,7 @@ limitations under the License.
-->
<link rel="import" href="../bower_components/polymer/polymer.html">
<link rel="import" href="../bower_components/iron-a11y-keys-behavior/iron-a11y-keys-behavior.html">
<link rel="import" href="gr-diff-comment-thread.html">
<dom-module id="gr-diff-view">
@@ -125,6 +126,13 @@ limitations under the License.
url="[[_computeCommentsPath(_changeNum, _patchNum)]]"
json-prefix=")]}'"
on-response="_handleRightCommentsResponse"></iron-ajax>
<!-- TODO(andybons): This is populated in gr-change-view. Use that instead
of incurring an extra ajax call. -->
<iron-ajax
id="filesXHR"
url="[[_computeFilesPath(_changeNum, _patchNum)]]"
json-prefix=")]}'"
on-response="_handleFilesResponse"></iron-ajax>
<h3>
<a href$="[[_computeChangePath(_changeNum)]]">[[_changeNum]]</a><span>:</span>
<span>[[_change.subject]]</span> — <span>[[params.path]]</span>
@@ -145,7 +153,17 @@ limitations under the License.
Polymer({
is: 'gr-diff-view',
behaviors: [
Polymer.IronA11yKeysBehavior
],
properties: {
keyEventTarget: {
type: Object,
value: function() {
return document.body;
},
},
/**
* URL params passed from the router.
*/
@@ -158,21 +176,29 @@ limitations under the License.
value: 80,
observer: '_rulerWidthChanged',
},
_basePatchNum: String,
_change: Object,
_changeNum: String,
_diff: Object,
_basePatchNum: String,
_fileList: {
type: Array,
value: [],
},
_leftComments: Array,
_patchNum: String,
_path: String,
_leftComments: Array,
_rightComments: Array,
_rendered: Boolean,
_rightComments: Array,
},
listeners: {
'diffContainer.tap': '_diffContainerTapHandler',
},
keyBindings: {
'[ ] u': '_handleKey',
},
_paramsChanged: function(value) {
this._changeNum = value.changeNum;
this._patchNum = value.patchNum;
@@ -198,6 +224,7 @@ limitations under the License.
this.$.leftCommentsXHR.generateRequest();
}
this.$.rightCommentsXHR.generateRequest();
this.$.filesXHR.generateRequest();
},
_rulerWidthChanged: function(newValue, oldValue) {
@@ -242,6 +269,10 @@ limitations under the License.
return '/changes/' + changeNum + '/revisions/' + patchNum + '/comments';
},
_computeFilesPath: function(changeNum, patchNum) {
return '/changes/' + changeNum + '/revisions/' + patchNum + '/files';
},
_diffQueryParams: function(basePatchNum) {
var params = {
context: 'ALL',
@@ -272,12 +303,48 @@ limitations under the License.
this._rightComments);
},
_handleFilesResponse: function(e, req) {
this._fileList = Object.keys(e.detail.response).sort();
},
_handleDiffResponse: function(e, req) {
this._diff = e.detail.response;
this._maybeRenderDiff(this._diff, this._leftComments,
this._rightComments);
},
_handleKey: function(e) {
switch(e.detail.combo) {
case '[':
this._navToFile(this._fileList, -1);
break;
case ']':
this._navToFile(this._fileList, 1);
break;
case 'u':
if (this._changeNum) {
page.show(this._computeChangePath(this._changeNum));
}
break;
}
},
_navToFile: function(fileList, direction) {
if (fileList.length == 0) { return; }
var idx = fileList.indexOf(this._path) + direction;
if (idx < 0 || idx > fileList.length - 1) {
page.show(this._computeChangePath(this._changeNum));
return;
}
page.show(
this._diffURL(this._changeNum, this._patchNum, fileList[idx]));
},
_diffURL: function(changeNum, patchNum, path) {
return '/c/' + changeNum + '/' + patchNum + '/' + path;
},
_threadID: function(patchNum, lineNum) {
return 'thread-' + patchNum + '-' + lineNum;
},
@@ -351,9 +418,18 @@ limitations under the License.
}
},
_clearChildren: function(el) {
while (el.firstChild) {
el.removeChild(el.firstChild);
}
},
_maybeRenderDiff: function(diff, leftComments, rightComments) {
if (this._rendered) {
throw Error('diff has already been rendered');
this._clearChildren(this.$.leftDiffNumbers);
this._clearChildren(this.$.leftDiffContent);
this._clearChildren(this.$.rightDiffNumbers);
this._clearChildren(this.$.rightDiffContent);
}
if (!diff || !diff.content) { return; }
if (this._basePatchNum && leftComments == null) { return; }

19
polygerrit-ui/app/test/gr-change-list-test.html
View File

@@ -20,6 +20,7 @@ limitations under the License.
<script src="../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
<script src="../../bower_components/web-component-tester/browser.js"></script>
<script src="../../bower_components/page/page.js"></script>
<script src="../scripts/fake-app.js"></script>
<link rel="import" href="../../bower_components/iron-test-helpers/iron-test-helpers.html">
@@ -52,21 +53,27 @@ limitations under the License.
assert.equal(changeList.selectedIndex, 0);
MockInteractions.pressAndReleaseKeyOn(changeList, 74); // 'j'
flushAsynchronousOperations();
assert.equal(changeList.selectedIndex, 1);
MockInteractions.pressAndReleaseKeyOn(changeList, 74); // 'j'
flushAsynchronousOperations();
var showStub = sinon.stub(page, 'show');
assert.equal(changeList.selectedIndex, 2);
MockInteractions.pressAndReleaseKeyOn(changeList, 13); // 'enter'
assert(showStub.lastCall.calledWithExactly('/c/2/'),
'Should navigate to /c/2/');
MockInteractions.pressAndReleaseKeyOn(changeList, 75); // 'k'
flushAsynchronousOperations();
assert.equal(changeList.selectedIndex, 1);
MockInteractions.pressAndReleaseKeyOn(changeList, 13); // 'enter'
assert(showStub.lastCall.calledWithExactly('/c/1/'),
'Should navigate to /c/1/');
MockInteractions.pressAndReleaseKeyOn(changeList, 75); // 'k'
flushAsynchronousOperations();
MockInteractions.pressAndReleaseKeyOn(changeList, 75); // 'k'
flushAsynchronousOperations();
MockInteractions.pressAndReleaseKeyOn(changeList, 75); // 'k'
flushAsynchronousOperations();
assert.equal(changeList.selectedIndex, 0);
showStub.restore();
});
});

61
polygerrit-ui/app/test/gr-change-view-test.html Normal file
View File

@@ -0,0 +1,61 @@
<!DOCTYPE html>
<!--
Copyright (C) 2015 The Android Open Source Project
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
<title>gr-change-view</title>
<script src="../../bower_components/webcomponentsjs/webcomponents.min.js"></script>
<script src="../../bower_components/web-component-tester/browser.js"></script>
<script src="../../bower_components/page/page.js"></script>
<script src="../scripts/changes.js"></script>
<link rel="import" href="../../bower_components/iron-test-helpers/iron-test-helpers.html">
<link rel="import" href="../elements/gr-change-view.html">
<test-fixture id="basic">
<template>
<gr-change-view></gr-change-view>
</template>
</test-fixture>
<script>
suite('gr-change-view tests', function() {
var element;
setup(function() {
element = fixture('basic');
});
test('keyboard shortcuts', function(done) {
var showStub = sinon.stub(page, 'show');
MockInteractions.pressAndReleaseKeyOn(element, 85); // 'u'
assert(showStub.lastCall.calledWithExactly('/'),
'Should navigate to /');
showStub.restore();
// TODO(andybons): Actually test the effect once _showReplyField is
// implemented.
sinon.stub(element, '_showReplyField', function() {
element._showReplyField.restore();
done();
});
MockInteractions.pressAndReleaseKeyOn(element, 65); // 'a'
});
});
</script>

64
polygerrit-ui/app/test/gr-diff-view-test.html
View File

@@ -20,6 +20,7 @@ limitations under the License.
<script src="../../bower_components/webcomponentsjs/webcomponents.min.js"></script>
<script src="../../bower_components/web-component-tester/browser.js"></script>
<script src="../../bower_components/page/page.js"></script>
<script src="../scripts/changes.js"></script>
<script src="../scripts/util.js"></script>
@@ -219,5 +220,68 @@ limitations under the License.
});
});
test('keyboard shortcuts', function() {
element._changeNum = '42';
element._patchNum = '10';
element._fileList = ['chell.go', 'glados.txt', 'wheatley.md'];
element._path = 'glados.txt';
var showStub = sinon.stub(page, 'show');
MockInteractions.pressAndReleaseKeyOn(element, 85); // 'u'
assert(showStub.lastCall.calledWithExactly('/c/42'),
'Should navigate to /c/42');
pressAndReleaseKeyIdentifierOn(element, '\U+005D'); // ']'
assert(showStub.lastCall.calledWithExactly('/c/42/10/wheatley.md'),
'Should navigate to /c/42/10/wheatley.md');
element._path = 'wheatley.md';
pressAndReleaseKeyIdentifierOn(element, '\U+005B'); // '['
assert(showStub.lastCall.calledWithExactly('/c/42/10/glados.txt'),
'Should navigate to /c/42/10/glados.txt');
element._path = 'glados.txt';
pressAndReleaseKeyIdentifierOn(element, '\U+005B'); // '['
assert(showStub.lastCall.calledWithExactly('/c/42/10/chell.go'),
'Should navigate to /c/42/10/chell.go');
element._path = 'chell.go';
pressAndReleaseKeyIdentifierOn(element, '\U+005B'); // '['
assert(showStub.lastCall.calledWithExactly('/c/42'),
'Should navigate to /c/42');
showStub.restore();
// https://github.com/PolymerElements/iron-test-helpers/issues/33
function keyboardEventFor(type, keyIdentifier) {
var event = new CustomEvent(type, {
bubbles: true,
cancelable: true
});
event.keyIdentifier = keyIdentifier;
return event;
}
function keyEventOn(target, type, keyIdentifier) {
target.dispatchEvent(keyboardEventFor(type, keyIdentifier));
}
function keyDownOn(target, keyIdentifier) {
keyEventOn(target, 'keydown', keyIdentifier);
}
function keyUpOn(target, keyIdentifier) {
keyEventOn(target, 'keyup', keyIdentifier);
}
function pressAndReleaseKeyIdentifierOn(target, keyIdentifier) {
keyDownOn(target, keyIdentifier);
Polymer.Base.async(function() {
keyUpOn(target, keyIdentifier);
}, 1);
}
});
});
</script>

1
polygerrit-ui/app/test/index.html
View File

@@ -25,6 +25,7 @@ limitations under the License.
[ 'gr-change-list-item-test.html',
'gr-change-list-test.html',
'gr-change-view-test.html',
'gr-date-formatter-test.html',
'gr-diff-comment-test.html',
'gr-diff-comment-thread-test.html',

4
polygerrit-ui/bower.json
View File

@@ -7,8 +7,8 @@
"polymer": "Polymer/polymer#1.2.2",
"page": "visionmedia/page.js#1.6.4",
"iron-ajax": "PolymerElements/iron-ajax#1.1.0",
"iron-a11y-keys": "PolymerElements/iron-a11y-keys#1.0.3",
"iron-input": "PolymerElements/iron-input#1.0.6"
"iron-input": "PolymerElements/iron-input#1.0.6",
"iron-a11y-keys-behavior": "PolymerElements/iron-a11y-keys-behavior#1.1.0"
},
"devDependencies": {
"web-component-tester": "*",