feat(keyboard): allow to override keyboard handlers #289

barmac · 2018-10-22T09:06:25Z

This PR allows to add listeners which override default keyboard handlers.

use KeyboardEvent.key value by default
allow to register KeyboardEvent listeners for specified keys
allow to use priority for KeyboardEvents
default listeners will use key values instead of keyCode
preserve previous way of registering listeners

There are three ways to register a listener now using keyboard.addListener:

Pass key or array of keys, then priority and the callback
Pass key or array of keys, then callback (default priority will be used)
Pass sole callback for global keyboard listener (lower priority than the latter)

Closes #226

nikku

Added a few thoughts; not meant as a full review.

In general, if the goal should be allow to override keyboard handlers this solution accomplishes way to much / is to big and complicated.

If it should be to overhaul the existing keyboard implementation there is still room for improvement, i.e. make it simpler to react to combinations (CTRL+Y, CTRL+SHIFT+Z).

nikku · 2018-10-23T06:33:08Z

lib/features/keyboard/DiagramKeyBindings.js

+ */
+export default function DiagramKeyBindings(keyboard, editorActions) {
+
+  var moveCanvasLeft = MoveCanvasFactory('left', keyboard._config, editorActions);


Why don't we plug in move canvas keyboard shortcuts via the MoveCanvas feature?

This would potentially allow us to easier override it (i.e. to restore the old behavior).

It is actually done using the MoveCanvas feature, see: https://github.com/bpmn-io/diagram-js/pull/289/files#diff-ab467edcc671fb2f7e5cb7a91b06437d
The factory function was added to keep code DRY as the only difference between the functions responsible for the movement is the direction property passed to MoveCanvas feature. Alternatively, we could move the factory function to DiagramKeyBindings.js as a helper at the end of the file.

However, this does not seem to make it more difficult to override the listeners, because the way to do that is always the same: add listeners for the same keys but with higher priority.

Please let me know what you think.

I'd also go for a MoveCanvas feature instead of a factory function. This would allow the injection of config and editorActions and would also allow the configuration of the feature specifically which I think makes sense because the speed is not the speed of the keyboard or anything, it's the speed of moving the canvas.

new Diagram({ keyboard: { bindTo: document }, moveCanvas: { speed: 9000 } });

Maybe even better?

new Diagram({ keyboard: { bindTo: document }, moveCanvas: { keyboardMoveSpeed: 9000 } });

lib/features/keyboard/Keyboard.js

nikku · 2018-10-23T06:36:26Z

lib/features/keyboard/Keyboard.js

-  this._config = config || {};
+  this._config = assign({
+    speed: DEFAULT_SPEED,
+    invertY: false


As mentioned above, the move canvas specifics belong to a move canvas keybinding if you ask me.

As an alternative we could make the detection implicit, i.e. apply it once we detect MacOS.

The speed was already there, but it wasn't clearly expressed in the config declaraction. As the fallback, the implementation used a magic number. Otherwise, we could have a separate configuration for each key binding. I think it is a good idea.

I think a better place to move the config is in #376. I'll change it, but in another PR.

Same here. Speed shouldn't be part of the generic keyboard configuration. Other features might implement something that they want to do at a certain speed. If the configured speed doesn't work for two independent features they can't make use of it anyway.

nikku · 2018-10-23T06:41:57Z

lib/features/keyboard/Keyboard.js

+
+  // run global listeners only if no listeners for specific keys reacted
+  if (!eventBusResult) {
+    forEach(this._globalListeners, function(listener) {


Global listeners are being called with the code while local listeners are being called with the key? I'd rethink this behavior as I don't think it is intuitive.

I agree that it's not intuitive. We use keyCode for globalListeners only in order to preserve the backward compatibility, i.e. to not break for example the BpmnKeyBindings.
Alternatively, we could drop the old keyboard related behaviour as KeyboardEvent.keyCode is already a deprecated feature.

Please investigate how much of a breaking change that is. We are breaking with this release anyway and it could make sense to break this feature for the reason you mentioned, too.

There is no blog post regarding the way to add a keyboard listener (one mention of the feature); the examples don't address this issue either.
There are two forum topics regarding the keyboard issues: 1, 2

The change to key only would break BpmnKeyBindings as it uses keyCode. However, the feature itself needs refactor as it is a one big listener for each of Bpmn-js-specific key bindings.

nikku · 2018-10-23T06:43:38Z

lib/features/keyboard/Keyboard.js


 /**
 * A keyboard abstraction that may be activated and
 * deactivated by users at will, consuming key events
 * and triggering diagram actions.
 *
+ * For pressed keys, keyboard fires an event according to following scheme:
+ * `keyboard.keydown.[event.key || event.keyCode]`


When will this be called with the keyCode? I.e. how to separate the 1 key from the 1 keyCode?

keyCode will be used only for old browsers which don't support KeyboardEvent.key. However, it's supported since IE9 so probably we should just get rid of the fallback.

Removed the fallback.

nikku · 2018-10-23T06:46:15Z

lib/features/keyboard/DiagramKeyBindings.js

+  var moveCanvasRight = MoveCanvasFactory('right', keyboard._config, editorActions);
+  var moveCanvasDown = MoveCanvasFactory('down', keyboard._config, editorActions);
+
+  keyboard.addListener(copyKeys, DEFAULT_PRIORITY, copy);


Could we support something even more powerful for our users here:

keyboard.addCombo([ `ctrl+A`, `ArrowUp` ], doSomething)

This would be a true simplification for the API.

Inspired by hotshot.

Thank you, I will have a look at that.

I think this is a good candidate for a new feature. Not sure though, if we should add it in this PR or maybe a new one. What about if we add an issue for this?

barmac · 2018-10-23T07:37:15Z

Thank you for the feedback @nikku.

Indeed, the PR seems to add a lot of code to the library. However, the previous implementation seemed to rely too much on hardcoded order of listeners, that's why I decided to use EventBus instead. Probably I should just change the PR's name. What do you think?

barmac · 2018-10-23T09:19:14Z

Removed keyCode fallback for the new-style listeners. Linted the block import statement.

nikku · 2018-10-23T10:30:11Z

If you want to move fast I suggest you to only apply the relevant changes to get priorities to the old behavior.

It should be trivial to add priorities to the old Keyboard#addListener implementation. Even implementing the whole listener logic via the eventBus should be an easy and straight forward thing to do.

Next up we could take our time to think about whether a better keyboard API makes sense, introduce breaking changes and so forth.

nikku · 2018-10-23T10:38:32Z

Imagine every key event went via the eventBus. In order to get the hotshot behavior a simple addition could be a utility that matches key strokes (or event sequences) using a pattern:

// add a cut listener with high priority
keyboard.addListener(5000, combo('ctrl+x', function(event) {
  // `combo` utility ensures that this method is only called if key stroke matches combo
}));

// add listener for sequence of events
keyboard.addListener(5000, sequence('a b', function(event) {
  // `sequence` utility ensures that this method is only called if two keys in sequence matched <a> and <b>
}));

// or register via eventBus directly

// do we actually need a distinction between key{up|down}?
// or is it enough to support a simple key event?
eventBus.on('keyboard.key{up,down}', 5000, function(event) {
  // called on any key; may be wrapped to filter only relevant invocations
});

nikku · 2018-10-23T10:40:18Z

I totally agree separating the keyboard from the default key events. This should be a separate commit though, as it is a clear non-breaking refactoring.

barmac · 2018-10-23T13:41:52Z

Imagine every key event went via the eventBus. In order to get the hotshot behavior a simple addition could be a utility that matches key strokes (or event sequences) using a pattern:

// add a cut listener with high priority
keyboard.addListener(5000, combo('ctrl+x', function(event) {
  // `combo` utility ensures that this method is only called if key stroke matches combo
}));

// add listener for sequence of events
keyboard.addListener(5000, sequence('a b', function(event) {
  // `sequence` utility ensures that this method is only called if two keys in sequence matched <a> and <b>
}));

// or register via eventBus directly

// do we actually need a distinction between key{up|down}?
// or is it enough to support a simple key event?
eventBus.on('keyboard.key{up,down}', 5000, function(event) {
  // called on any key; may be wrapped to filter only relevant invocations
});

Okay, but this way we always trigger all (or almost all) the event listeners and let them pass silently, if they are not interested in the pressed key, correct?

nikku · 2018-10-23T14:08:25Z

Okay, but this way we always trigger all (or almost all) the event listeners and let them pass silently, if they are not interested in the pressed key, correct?

Yes. Did you benchmark if this is an issue? I assume it is not.

barmac · 2018-10-23T15:58:48Z

I've rewritten this PR so that we use EventBus for all events, introducing this way a breaking change to Keyboard. The listeners will be able to decide whether to react basing on keyCode or key or both as they will receive KeyboardEvent. Also, the change is now split up between two commits: one for the breaking change, another for moving the default listeners to a separate file.

nikku

Final thought: What is going to be the user case to replace / override the the keyboard binding?

If users should provide a single keybinding service we could call the DiagramKeyBindings something like KeyboardBindings instead.

lib/features/keyboard/Keyboard.js

barmac · 2018-10-24T08:33:59Z

Applied changes suggested by @nikku. Thanks for the feedback :)

nikku

Looks good for me, what about you @philippfromme?

Changes applied.

BREAKING CHANGE: keyboard listeners receive only the `KeyboardEvent` Instead of (keyCode, event), the keyboard event listeners will receive object with `KeyboardEvent` event as only property. * It is now possible to set priority of an event handler by passing (priority, listener) to `Keyboard#addListener`. * The default listeners are adjusted to handle events the new way.

BREAKING CHANGE: lifecycle listeners will no longer receive field listeners * Listeners are no longer stored inside Keyboard. * Field `listeners` was removed from lifecycle events. Closes #226

`KeyboardEvent.keyCode` is deprecated. Default listeners will use `KeyboardEvent.key` instead.

barmac · 2018-10-25T09:15:43Z

Added missing dependency.

philippfromme

Added a few changes regarding coding style. Looks 🍰 now.

barmac force-pushed the 226-enable-to-override-key-bindings branch 2 times, most recently from f87b4c9 to 3684633 Compare October 22, 2018 09:57

ghost assigned barmac Oct 22, 2018

ghost added the needs review Review pending label Oct 22, 2018

barmac requested a review from philippfromme October 22, 2018 11:02

nikku reviewed Oct 23, 2018

View reviewed changes

barmac force-pushed the 226-enable-to-override-key-bindings branch from 3684633 to 8fc097a Compare October 23, 2018 09:12

barmac force-pushed the 226-enable-to-override-key-bindings branch 2 times, most recently from d5fc320 to 97ec05d Compare October 23, 2018 09:21

barmac force-pushed the 226-enable-to-override-key-bindings branch 2 times, most recently from a86cd2d to c130732 Compare October 23, 2018 15:47

nikku previously requested changes Oct 23, 2018

View reviewed changes

lib/features/keyboard/Keyboard.js Outdated Show resolved Hide resolved

lib/features/keyboard/Keyboard.js Show resolved Hide resolved

barmac force-pushed the 226-enable-to-override-key-bindings branch from c130732 to 65cba50 Compare October 24, 2018 08:30

barmac force-pushed the 226-enable-to-override-key-bindings branch 3 times, most recently from 690b2ea to 8b1dcda Compare October 24, 2018 10:59

nikku reviewed Oct 24, 2018

View reviewed changes

barmac added 3 commits October 25, 2018 10:59

chore(keyboard): move default event listeners to a separate file

3606802

feat(keyboard): remove listeners from lifecycle events

0d89045

BREAKING CHANGE: lifecycle listeners will no longer receive field listeners * Listeners are no longer stored inside Keyboard. * Field `listeners` was removed from lifecycle events. Closes #226

feat(keyboard): use key value for default listeners

9c78bfb

`KeyboardEvent.keyCode` is deprecated. Default listeners will use `KeyboardEvent.key` instead.

barmac force-pushed the 226-enable-to-override-key-bindings branch from 9313f60 to 9c78bfb Compare October 25, 2018 09:14

ghost assigned philippfromme Oct 25, 2018

chore(keyboard): coding style

d3aed57

philippfromme force-pushed the 226-enable-to-override-key-bindings branch from 1d256ba to d3aed57 Compare October 25, 2018 14:02

philippfromme approved these changes Oct 25, 2018

View reviewed changes

merge-me bot merged commit b967046 into master Oct 25, 2018

delete-merged-branch bot deleted the 226-enable-to-override-key-bindings branch October 25, 2018 14:06

ghost removed the needs review Review pending label Oct 25, 2018

barmac mentioned this pull request Oct 26, 2018

Adjust keybindings to new api bpmn-io/bpmn-js#885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(keyboard): allow to override keyboard handlers #289

feat(keyboard): allow to override keyboard handlers #289

barmac commented Oct 22, 2018 •

edited

Loading

nikku left a comment •

edited

Loading

nikku Oct 23, 2018

barmac Oct 23, 2018

philippfromme Oct 23, 2018 •

edited

Loading

nikku Oct 23, 2018

nikku Oct 23, 2018

barmac Oct 23, 2018

barmac Oct 23, 2018

philippfromme Oct 23, 2018

nikku Oct 23, 2018

barmac Oct 23, 2018

nikku Oct 23, 2018

barmac Oct 23, 2018

nikku Oct 23, 2018

barmac Oct 23, 2018

barmac Oct 23, 2018

nikku Oct 23, 2018 •

edited by philippfromme

Loading

barmac Oct 23, 2018

barmac Oct 23, 2018

barmac commented Oct 23, 2018

barmac commented Oct 23, 2018

nikku commented Oct 23, 2018

nikku commented Oct 23, 2018

nikku commented Oct 23, 2018

barmac commented Oct 23, 2018

nikku commented Oct 23, 2018

barmac commented Oct 23, 2018

nikku left a comment

barmac commented Oct 24, 2018

nikku left a comment

barmac commented Oct 25, 2018

philippfromme left a comment

feat(keyboard): allow to override keyboard handlers #289

feat(keyboard): allow to override keyboard handlers #289

Conversation

barmac commented Oct 22, 2018 • edited Loading

nikku left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippfromme Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikku Oct 23, 2018 • edited by philippfromme Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barmac commented Oct 23, 2018

barmac commented Oct 23, 2018

nikku commented Oct 23, 2018

nikku commented Oct 23, 2018

nikku commented Oct 23, 2018

barmac commented Oct 23, 2018

nikku commented Oct 23, 2018

barmac commented Oct 23, 2018

nikku left a comment

Choose a reason for hiding this comment

barmac commented Oct 24, 2018

nikku left a comment

Choose a reason for hiding this comment

barmac commented Oct 25, 2018

philippfromme left a comment

Choose a reason for hiding this comment

barmac commented Oct 22, 2018 •

edited

Loading

nikku left a comment •

edited

Loading

philippfromme Oct 23, 2018 •

edited

Loading

nikku Oct 23, 2018 •

edited by philippfromme

Loading