Move most of shortcut mixin features into service
Make the keyboard shortcut mixin (and thus all of Gerrit) independent
from IronA11yKeysBehavior. The major change needed for this was
introducing `addShortcut` to the ShortcutsService as a replacement for
`addOwnKeyBinding`.
The views implementing `keyboardShortcuts()` needed to be changed to
return a proper callback instead of just a string.
We don't use `keyUp` events anymore. Everything is just `keyDown`.
`keyUp` was only introduced because of troubles handling combo keys,
which is now properly dealt with.
Along with `IronA11yKeysBehavior` the `IronKeyboardEvent` is also gone.
All code is just using the standard `KeyboardEvent` instead. Also the
`keyEventTarget` does not have any meaning anymore. The service adds
all listeners to `document.body` by default. If the need arises, then
we can introduce an option later for allowing shortcuts to also be
registered on a more "local" level.
The hundreds of `modifierPressed` could be removed, because of how
`eventMatchesShortcut` is implemented. If the modifiers are not matched
properly, then the shortcut will not be triggered. That also means that
some `shiftKey` checks could be removed.
We are generally suppressing `repeat` keydown events. If processing them
is desired, then we can bring them back as an explicit option. But
ignoring them seems like the best default.
If we are triggering a shortcut callback, then we are always calling
`preventDefault()` and `stopPropagation()` on the keyboard event. That
also seems a great default and allows to remove tons of individual
`preventDefault()` calls. If that turns out to be undesired in specific
scenarios, then we can introduce a dedicated option.
We are also always checking `shouldSuppress()` for every shortcut, so
the tons of individual checks could also be removed.
Some handlers were handling lowercase and uppercase chars in one method,
e.g. 'n' and 'N'. We have split those up in two dedicated handlers.
The combo key handling could be completely moved into the service. Those
special shortcuts are treated as normal shortcuts, but when checking a
shortcut against an event we also check whether the current combo state
matches the shortcut definition. This simplifies matters a lot.
Instead of calling addShortcut() directly for every shortcut, the mixin
just passes all information into `attachHost`, and then the service can
iterate over all the shortcuts and bindings. And thus also be
responsible for the cleanups (i.e. calling removeEventListener).
There are still a lot of potential follow-up cleanups and refactorings
that are staring into your face, but obviously this change is already
fairly large, so let's leave them for later. The major accomplishment
here is that replicating the mixin's functionality in a Lit controller
has become trivial. So the Lit migration of Polymer elements that
depend on the keyboard mixin is unblocked. Also note that the entire
"observer" related code in the mixin is something that we probably want
to get rid of altogether and replace it by something clearer and
simpler, e.g. the service knowing which view is current.
Change-Id: I5cf82feb1c8af16c579eab2120381c85e9607969
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
index d8aa11e..05c4f53 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service_test.ts
@@ -16,12 +16,14 @@
*/
import '../../test/common-test-setup-karma';
import {
- ShortcutsService,
COMBO_TIMEOUT_MS,
+ describeBinding,
+ ShortcutsService,
} from '../../services/shortcuts/shortcuts-service';
import {Shortcut, ShortcutSection} from './shortcuts-config';
import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions';
import {SinonFakeTimers} from 'sinon';
+import {Key, Modifier} from '../../utils/dom-util';
async function keyEventOn(
el: HTMLElement,
@@ -96,13 +98,12 @@
});
test('getShortcut', () => {
- const NEXT_FILE = Shortcut.NEXT_FILE;
- assert.equal(service.getShortcut(NEXT_FILE), ']');
- });
-
- test('getShortcut with modifiers', () => {
- const NEXT_FILE = Shortcut.TOGGLE_LEFT_PANE;
- assert.equal(service.getShortcut(NEXT_FILE), 'Shift+a');
+ assert.equal(service.getShortcut(Shortcut.NEXT_FILE), ']');
+ assert.equal(service.getShortcut(Shortcut.TOGGLE_LEFT_PANE), 'A');
+ assert.equal(
+ service.getShortcut(Shortcut.SEND_REPLY),
+ 'Ctrl+Enter,Meta/Cmd+Enter'
+ );
});
suite('binding descriptions', () => {
@@ -113,14 +114,18 @@
}
test('single combo description', () => {
- assert.deepEqual(service.describeBinding('a'), ['a']);
- assert.deepEqual(service.describeBinding('a:keyup'), ['a']);
- assert.deepEqual(service.describeBinding('ctrl+a'), ['Ctrl', 'a']);
- assert.deepEqual(service.describeBinding('ctrl+shift+up:keyup'), [
- 'Ctrl',
- 'Shift',
- '↑',
- ]);
+ assert.deepEqual(describeBinding({key: 'a'}), ['a']);
+ assert.deepEqual(
+ describeBinding({key: 'a', modifiers: [Modifier.CTRL_KEY]}),
+ ['Ctrl', 'a']
+ );
+ assert.deepEqual(
+ describeBinding({
+ key: Key.UP,
+ modifiers: [Modifier.CTRL_KEY, Modifier.SHIFT_KEY],
+ }),
+ ['Shift', 'Ctrl', '↑']
+ );
});
test('combo set description', () => {
@@ -130,9 +135,9 @@
);
assert.deepEqual(service.describeBindings(Shortcut.SAVE_COMMENT), [
['Ctrl', 'Enter'],
- ['Meta', 'Enter'],
+ ['Meta/Cmd', 'Enter'],
['Ctrl', 's'],
- ['Meta', 's'],
+ ['Meta/Cmd', 's'],
]);
assert.deepEqual(service.describeBindings(Shortcut.PREV_FILE), [['[']]);
});
@@ -187,67 +192,68 @@
test('active shortcuts by section', () => {
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {});
- service.attachHost({}, new Map([[Shortcut.NEXT_FILE, 'null']]));
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.NEXT_FILE, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
- service.attachHost({}, new Map([[Shortcut.NEXT_LINE, 'null']]));
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.NEXT_LINE, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.DIFFS]: [
{
shortcut: Shortcut.NEXT_LINE,
text: 'Go to next line',
- bindings: ['j', 'down'],
+ bindings: [{key: 'j'}, {key: 'ArrowDown'}],
},
],
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
- service.attachHost(
- {},
- new Map([
- [Shortcut.SEARCH, 'null'],
- [Shortcut.GO_TO_OPENED_CHANGES, 'null'],
- ])
- );
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.SEARCH, listener: _ => {}},
+ {shortcut: Shortcut.GO_TO_OPENED_CHANGES, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.activeShortcutsBySection()), {
[ShortcutSection.DIFFS]: [
{
shortcut: Shortcut.NEXT_LINE,
text: 'Go to next line',
- bindings: ['j', 'down'],
+ bindings: [{key: 'j'}, {key: 'ArrowDown'}],
},
],
[ShortcutSection.EVERYWHERE]: [
{
shortcut: Shortcut.SEARCH,
text: 'Search',
- bindings: ['/'],
+ bindings: [{key: '/'}],
},
{
shortcut: Shortcut.GO_TO_OPENED_CHANGES,
text: 'Go to Opened Changes',
- bindings: ['GO_KEY', 'o'],
+ bindings: [{key: 'o', combo: 'g'}],
},
],
[ShortcutSection.NAVIGATION]: [
{
shortcut: Shortcut.NEXT_FILE,
text: 'Go to next file',
- bindings: [']'],
+ bindings: [{key: ']'}],
},
],
});
@@ -256,33 +262,31 @@
test('directory view', () => {
assert.deepEqual(mapToObject(service.directoryView()), {});
- service.attachHost(
- {},
- new Map([
- [Shortcut.GO_TO_OPENED_CHANGES, 'null'],
- [Shortcut.NEXT_FILE, 'null'],
- [Shortcut.NEXT_LINE, 'null'],
- [Shortcut.SAVE_COMMENT, 'null'],
- [Shortcut.SEARCH, 'null'],
- ])
- );
+ service.attachHost(document.createElement('div'), [
+ {shortcut: Shortcut.GO_TO_OPENED_CHANGES, listener: _ => {}},
+ {shortcut: Shortcut.NEXT_FILE, listener: _ => {}},
+ {shortcut: Shortcut.NEXT_LINE, listener: _ => {}},
+ {shortcut: Shortcut.SAVE_COMMENT, listener: _ => {}},
+ {shortcut: Shortcut.SEARCH, listener: _ => {}},
+ ]);
assert.deepEqual(mapToObject(service.directoryView()), {
[ShortcutSection.DIFFS]: [
{binding: [['j'], ['↓']], text: 'Go to next line'},
{
- binding: [
- ['Ctrl', 'Enter'],
- ['Meta', 'Enter'],
- ],
+ binding: [['Ctrl', 'Enter']],
text: 'Save comment',
},
{
binding: [
+ ['Meta/Cmd', 'Enter'],
['Ctrl', 's'],
- ['Meta', 's'],
],
text: 'Save comment',
},
+ {
+ binding: [['Meta/Cmd', 's']],
+ text: 'Save comment',
+ },
],
[ShortcutSection.EVERYWHERE]: [
{binding: [['/']], text: 'Search'},
