API Design

[lukemelia]

Rendered

We're implementing in this PR as well. Here's the check list:

• on-key helper basic implementation
• on-key modifier support for input, textarea, select elements
• on-key modifier defaults to triggering click if handler function is not specified
• deprecate on-keyboard modifier
• deprecate keyboard-shortcut modifier
• deprecate keyboard-press component
• decide on new responder API (currently has/trigger via Evented mixin) that leverages isKey function
• update API-DESIGN.md with new responder API https://github.com/adopted-ember-addons/ember-keyboard/blob/rfc/api-change/API-DESIGN.md#responder-api
• implement basic isKey function d50035c
• move new API to new responder API
• preserve fallback to old responder API with deprecation warning
• Support for stopPropagation/stopImmediatePropagation from on-key helper
• Support for stopPropagation/stopImmediatePropagation from on-key modifier
• implement if-key helper
• implement _all support in isKey function
• implement key mapping support in isKey function
• assertion for callback argument to on-key helper being a function
• implement decorator support for ES6 classes
• implement decorator support for classic classes
• document decorator usage
• deprecate mixins
• documentation review
• write a V5 -> v6 migration guide
• ship v6.0.0-beta.1
• ship v6.0.0

[mattmcmanus]

I like where this is headed. I'd be in favor of proposal 3 for a couple reasons.

1. The key mode will be the intuitive default and from my uneducated hunch, will be used by the majority of users
2. I like defaulting to keydown.
3. From an end-user perspective, I like using the shorter and potentially more expressive "Alt+c" approach. As for key/code normalization and other things, have you considered backing that by a library from the larger JS ecosystem like Mousetrap? It seems like it's worked a lot of that stuff out and has some extra feature which would be nice to have.

There are some things that feel intuitive to me about these changes though.

• Using on as the attribute when changing to keyup doesn't feel ideal. on is starting to feel a bit overloaded. What about when="keyup"?
• Can you flip the order of the positional params? Having the function first, then the key combo feels backwards to me.
• I don't love the name on-keyboard. Why not use on-keypress or even on-key?
• It's not immediately obvious to me what the keyboard-shortcut modifier does? It requires some other {{on}} action to be wired up to work? Will it validate that something exists?
• Having a component and a modifier with the same name feels risky.
  • What about the component being named keyboard-shortcut since that what it's setting up?
  • Another thought, what about precedence when a modifier and component potentially conflict? I need to think about this some more, but it seems like having just the modifier or just the component would simplify the mental model. Mousetraps documentation provides some ideas for this: https://craig.is/killing/mice#api.handleKey

Ok. That's all I've got!

[lukemelia]

@mattmcmanus Thanks for the excellent feedback:

As for key/code normalization and other things, have you considered backing that by a library from the larger JS ecosystem like Mousetrap? It seems like it's worked a lot of that stuff out and has some extra feature which would be nice to have.

I looked into mousetrap and keymaster to see how they handled key vs code and whether we could/should use them. In both cases, I found that they rely on now-deprecated keyboard event properties (which and keyCode). e.g. ccampbell/mousetrap#474. For that reason, I don't think that's a good path at the moment.

Using on as the attribute when changing to keyup doesn't feel ideal. on is starting to feel a bit overloaded. What about when="keyup"?

Good point, or maybe event="keyup".

Can you flip the order of the positional params? Having the function first, then the key combo feels backwards to me.

If we do this, we lose the ability to attach additional shortcuts to one action by adding key combo strings as additional arguments. I think that flexibility is worth it, but absent that I agree the order would read better flipped.

I don't love the name on-keyboard. Why not use on-keypress or even on-key?

I think on-keypress is out because keypress is the name of a deprecated event, so there is potential for confusion. on-key could work. Personally I like on-keyboard slightly more because the class of events we're handling are Keyboard events in W3C parlance.

It's not immediately obvious to me what the keyboard-shortcut modifier does? It requires some other {{on}} action to be wired up to work? Will it validate that something exists?

It triggers a click on the element it is attached to. I'll update the doc to clarify this.

Having a component and a modifier with the same name feels risky. What about the component being named keyboard-shortcut since that what it's setting up?

I can see what it would be confusing. The difference between the two is that the component listens and is triggerable whenever it is rendered while the modifier only triggers when the element is focused. I think we need both, though they don't necessarily need to be named the same thing.

Another thought, what about precedence when a modifier and component potentially conflict? I need to think about this some more, but it seems like having just the modifier or just the component would simplify the mental model. Mousetraps documentation provides some ideas for this: https://craig.is/killing/mice#api.handleKey

It's not in this API document currently, but ember-keyboard has a bunch of existing support for managing priority and propagation that I think we would want to preserve as-is.

[Alonski]

@lukemelia Have you publically shared this on Discord and Twitter? Maybe others can chime in :)
I don't have much usage of this addon so not sure how much my thoughts will be good here :)

[lukemelia]

@Alonski Yes, I have. If you can help amplify, that would be great: https://twitter.com/lukemelia/status/1256439569181900800

[seanCodes]

Just to chime in, I'm also a fan of proposal 3.

I wonder though if mode="code" is even needed, since it seems the combo string itself can define the mode based on whether a key code value is used. For example, "f" could indicate key mode and alt+KeyC could indicate code mode (which sounds backwards but follows the native KeyboardEvent key code naming).

I don't love the name on-keyboard. Why not use on-keypress or even on-key?

I think on-keypress is out because keypress is the name of a deprecated event, so there is potential for confusion. on-key could work. Personally I like on-keyboard slightly more because the class of events we're handling are Keyboard events in W3C parlance.

+1 to {{on-key}} vs {{on-keyboard}}. I had the same thought while reading through the RFC as on-keyboard was a bit verbose and clunky to read in the examples.

I’m a big fan of aligning with native terminology whenever possible, but in this case something like on-key or on-keys just seems so much more ergonomic and intuitive.

Having a component and a modifier with the same name feels risky. What about the component being named keyboard-shortcut since that what it's setting up?

I can see what it would be confusing. The difference between the two is that the component listens and is triggerable whenever it is rendered while the modifier only triggers when the element is focused. I think we need both, though they don't necessarily need to be named the same thing.

I didn't find {{keyboard-shortcut}} too confusing, but I think there might be a better name there as well. Maybe {{keybinding}} or {{bind-to-key}}? Or something more literal like {{trigger-on-key}}?

Can you flip the order of the positional params? Having the function first, then the key combo feels backwards to me.

If we do this, we lose the ability to attach additional shortcuts to one action by adding key combo strings as additional arguments. I think that flexibility is worth it, but absent that I agree the order would read better flipped.

I also think flipped order would be ideal... Not only does it feel more natural, it’s also a pattern users are familiar with because the {{on}} modifier works the same way. Could multiple key combos not be comma or space separated instead? Or optionally an array? The on modifier requires multiple {{on ...}} statements for each event, so that would also be an option.

As for key/code normalization and other things, have you considered backing that by a library from the larger JS ecosystem like Mousetrap? It seems like it's worked a lot of that stuff out and has some extra feature which would be nice to have.

I looked into mousetrap and keymaster to see how they handled key vs code and whether we could/should use them. In both cases, I found that they rely on now-deprecated keyboard event properties (which and keyCode). e.g. ccampbell/mousetrap#474. For that reason, I don't think that's a good path at the moment.

It’s a bummer that there’s no other solid JS keyboard libraries out there that could be leveraged. Mousetrap has always been my favorite but it’s definitely outdated and neglected. Hotkeys (a continuation of Keymaster) seems to be the only one actively maintained, but it still uses which. I’m amazed that this is the only project trying to use the new API. (Maybe we create a vanilla library based on this code that could then be leveraged by this addon? 😄)

[lukemelia]

@seanCodes That's some excellent feedback!

I wonder though if mode="code" is even needed, since it seems the combo string itself can define the mode based on whether a key code value is used. For example, "f" could indicate key mode and alt+KeyC could indicate code mode (which sounds backwards but follows the native KeyboardEvent key code naming).

I really like this idea. I just checked the list of code values and it doesn't seem like there is any overlap. In addition, it means that people new to keyboard events can largely get what they need done without having to go down this particular rabbit hole right away and likely become confused and frustrated.

I am going to update the document to take this approach.

I’m a big fan of aligning with native terminology whenever possible, but in this case something like on-key or on-keys just seems so much more ergonomic and intuitive.

OK, I find your and @mattmcmanus' feedback persuasive on this. I'll update.

I also think flipped order would be ideal... Not only does it feel more natural, it’s also a pattern users are familiar with because the {{on}} modifier works the same way. Could multiple key combos not be comma or space separated instead? Or optionally an array? The on modifier requires multiple {{on ...}} statements for each event, so that would also be an option.

This is a good point. I will update to align to the style of the {{on}} modifier and we can defer binding multiple keys in one invocation until we see real-world needs.

It’s a bummer that there’s no other solid JS keyboard libraries out there that could be leveraged. Mousetrap has always been my favorite but it’s definitely outdated and neglected. Hotkeys (a continuation of Keymaster) seems to be the only one actively maintained, but it still uses which. I’m amazed that this is the only project trying to use the new API.

Seriously. I think it's probably due to IE 11 not supporting these APIs, but still -- it's surprising.

(Maybe we create a vanilla library based on this code that could then be leveraged by this addon? 😄)

Please do implement. ;-)

[lukemelia]

I've incorporated feedback from @mattmcmanus and @seanCodes into Proposal 4, which is my new favorite.

It has one not-yet-discussed idea in it as well, which is to use the same {{on-key ...}} modifier instead of {{keyboard-shortcut ...}} but infer the "trigger a click" behavior if the developer does not pass an action argument.

[mattmcmanus]

@lukemelia @seanCodes Love it! Proposal 4 feels so much more ergonomic and intuitive.

Now my questions are:

1. Will there be an easy way to query the list of currently available shortcuts? This would be used to support the Shift+/ or ? shortcut to bring up a list of all the currently available shortcuts
2. The simpler alt+t way of defining a shortcut opens up the ability to do sequences as well. I'm thinking along the lines of gmail's g i to go to the inbox
3. Is a codemod from the old API to the new even possible?

[bendemboski]

I very much like proposal 4, but have one suggestion that I think is relatively minor in terms of the work/implementation, but would make it quite a bit more flexible. I think that the core key-mapping engine that takes DOM events and recognizes key patterns should be exposed independently of the DOM event handling, since I think that's the core value that this would be providing. So in a template, it would allow something like

{{!-- attach your own event handler using the {{on}} modifier --}}
<div {{on "keypress" (dispatch-key "alt+c" this.doThing)}}></div>

{{! use ember-on-helper }}
{{on-document "keydown" (dispatch-key "alt+x" this.doThing)}}

{{! use some third-party component API }}
<SomeComponent @onKey={{dispatch-key "alt+x" this.doThing}}/>

and in Javascript maybe

function onEvent(e) {
  if (keyEventMatches(e, 'alt+x')) {
    this.handleAltX();
  }
}

Absolutely still provide the higher-level API you've specified as the primary API for the addon, but also expose these lower-level primitives so if you don't anticipate some particular pattern for capturing DOM events, it's not a blocker and user can still leverage the harder-to-duplicate DOM-event parsing logic.

[lukemelia]

@mattmcmanus wrote:

Will there be an easy way to query the list of currently available shortcuts? This would be used to support the Shift+/ or ? shortcut to bring up a list of all the currently available shortcuts

Registered key handlers would be available via the the keyboard service, but I think it is likely that an interface like you've described would be maintained independently. Things like order, layout and a description of each key combos function would be impossible to standardize in a one-size-fits-most way.

@mattmcmanus wrote:

The simpler alt+t way of defining a shortcut opens up the ability to do sequences as well. I'm thinking along the lines of gmail's g i to go to the inbox

True. This could be a cool future enhancement.

@mattmcmanus wrote:

Is a codemod from the old API to the new even possible?

I don't think so. The issue that prompted this rethink was that we were conceptually mixing key and code treatment, so the existing behavior is something that we would not want to recreate. That said, since we are leaning toward brand new API, it could be possible to leave the existing API in place with the existing behavior and deprecate it. This would allow people to upgrade and incrementally move to the new API.

@bendemboski wrote:

I think that the core key-mapping engine that takes DOM events and recognizes key patterns should be exposed independently of the DOM event handling, since I think that's the core value that this would be providing.

I like this idea a lot. It would also help us ensure this part of the codebase is well-isolated and can be covered by a thorough test suite. This low-level API would by necessity not participate in other ember-keyboard features like priority and propagation options but that doesn't obviate its utility. I think I would find if-key to be clearer than dispatch-key if I'm understanding the intended behavior clearly.

[bendemboski]

I think I would find if-key to be clearer than dispatch-key if I'm understanding the intended behavior clearly.

Sure, makes sense. I called it dispatch-key because the helper took a whole table of key mappings, not just one. The only advantage I see to that is in reducing the number of DOM event listeners that are needed, but this seems like a pretty low priority given that I don't think people will be setting up, like, hundreds (or even tens) of key handlers. So if it's just defining one mapping, I agree that if-key seems clearer.

[optikalefx]

Chiming in here super late, sorry everyone. Had gotten pulled away from my upgrade which included this PR.

I'm a big fan of the evolution to Proposal 4. However I think I prefer the mode='code' approach from 3.

I think it comes down to documentation at this point. "alt+c" and "alt+KeyC" aren't immediately obvious. Given just these you really don't know for sure what the difference between "alt+c" and "alt+KeyC" is. Users will need to go read the docs and discover what it means.

However, "alt+c" mode="key" and "alt+c" mode="code" does say exactly what is happening. But, requires you to know about key vs code in the first place. So perhaps either way you need to read more.

It's really down to "how much magic" do we want here mode is less magic. Is that better? Hard to say - I think the way of Octane has been to be less magic.

[lukemelia]

@optikalefx wrote:

However, "alt+c" mode="key" and "alt+c" mode="code" does say exactly what is happening. But, requires you to know about key vs code in the first place. So perhaps either way you need to read more.

My thinking is that "alt+c" mode="code" would not be valid in Proposal 3. The developer would instead have to specify "alt+KeyC" mode="code"because KeyC is a valid code value and c is not. Moreover, because the set of valid key values and valid code values are non-overlapping, having an explicit mode as in Proposal 3 is redundant since it can be inferred from the provided key combo string.

[bendemboski]

I think it's definitely good to consider this question, but I'm still finding myself in favor of not having the mode argument.

Let's think about how users are going to first encounter this. If they read the documentation first because they are trying to start using the addon, then no problem - the documentation can be made clear.

If they encounter a usage of it in the wild, there are two possible cases:

1. Wherever they encounter it uses one or the other form consistently
2. Wherever they encounter it uses a mix of the two forms

In case 1, having an extra mode= makes it less clear and obvious - I start wondering what mode means only to find that it doesn't change my understanding of what's going on - my initial thought that it's just handing a c keypress was correct and it hasn't aided my understanding or learning other than to force me to learn something that I don't need to know right now. Moreover, there's the risk that I don't go and learn what mode means, and then copy and paste a usage with mode="code" (ignoring it because I don't understand or notice it) and change 'KeyUp' to 'w' and suddenly it doesn't work because I have the wrong mode.

In case 2 having mode= is potentially helpful because it makes it really clear that there are two modes... But even here the value is limited. As soon as I see the two different forms without the extra mode argument, if I'm confused I'll check the documentation and learn what I need to know, and if I'm not confused then I don't have an extra question to answer.

I think case 1 is going to be the much more common case, so given that the advantages mode= provides for case two are pretty minor, and it has significant drawbacks for our probably more common case 1, I don't think it's the way to go.

[lukemelia]

@bendemboski Great breakdown and scenario gaming. 👍

[lukemelia]

Last call for comments. I think we arrived at a great plan and I intend to start implementing it soon and see what happens when the plan collides with reality!

[lukemelia]

Implementation has begun. I've added a checklist to the description of this PR. If anyone wants to contribute, or to pair on any of this, let me know!

[lukemelia]

OK, I think we are good to go for a new beta. I'll leave this for a day or two before I merge in case anyone wants to do some more code review.

[NullVoxPopuli]

Just upgraded emberclear to v6. Nice work everyone! :D
upgrading was easy, and I was able to delete a bunch of my own code around handling key-events! 🎉

[lukemelia]

@NullVoxPopuli Great to hear! 👍