584064 - Keyboard Shortcut API

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Dietrich Ayala (:dietrich)]

The SDK needs an API for easily adding keyboard shortcuts.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: wip

WIP patch. sketching out a basic shortcut API.

currently looks something like:

require("shortcuts").register({
  key: "a",
  modifiers: ["SHIFT"],
  handler: function() {
    doStuff();
  }
});

Comment 2

[Drew Willcoxon :adw]

Have you thought about how to hook up UI elements to keys?  Like, if I have a context menu item and want to add a key shortcut to it, this might be nice:

  require("context-menu").Item({
    label: "My Item",
    onClick: doMyCommand,
    key: ???
  });

Of course sometimes key shortcuts don't involve UI elements at all, so the approach in comment 1 is necessary, but I'm just wondering if you have thoughts about how to marry the two uses cases.  XUL does this pretty well IMO with <command> and <key>.  Maybe in our case <command>s are just functions.

Comment 3

[Dietrich Ayala (:dietrich)]

Not working on this at the moment, unassigning.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 5

[PaulV]

Attachment: Updated version of shortcuts.js based on attachment 462464

Updated shortcuts.js to actually call handlers. It's ugly, but it works for me on Firefox 4.0 beta 12 on Mac OS X 10.6. Probably needs work to be fit for inclusion, but may be helpful for other addon developers.

Comment 6

[Dietrich Ayala (:dietrich)]

Thanks Paul!

In other news, Chromeless has a hotkey module now: http://mozilla.github.com/chromeless/#module/chromeless-kit/hotkey

Comment 8

[Dietrich Ayala (:dietrich)]

One thing I like about the Chromeless module is that key combinations are specified with a single simple string format instead of different options parameters.

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to pull request

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

So I did not knew about this patch, neither I knew about the implementations that was already in chromeless.

From what I see this patch implements something very close to my proposal.

I also was thinking something along the lines what chromeless has, but then I thought it was too implicit:
http://mozilla.github.com/chromeless/#module/chromeless-kit/hotkey

That being said I actually like the idea of registering command id's instead of functions as its easier to distribute across the processes. I don't really see how to register commands in chromeless yet but I'll investigate.

I also find it useful to have an ability to register multiple shortcuts at a time so I'd propose to tweak proposed API like this then:

require("{{whatever/id/will/be}}").{{register|define|create}}({
   "accel-s": "command#1",
   "meta-shift-i": "command#2"
});

 
require("{{whatever/id/will/be}}").{{bindings|shortcuts}}

Will be an object that contains all the registered commands. Note than reason to have an objects as a values is to make it possible to check for registered shortcuts without writing a parser.
 
{
  "accel-s": {
    modifiers: ["accel"],
    key: "s"
  },
  "meta-shift-i": {
    modifiers: ["meta", "shift"],
    key: "i"
  }
}

Alternatively we may decide not to expose this at all and just throw on attempt to re-register shortcut.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #1)
> require("shortcuts").register({
>   key: "a",
>   modifiers: ["SHIFT"],
>   handler: function() {
>     doStuff();
>   }
> });

This seems reasonable, but its additional structure makes it more complicated than Chromeless's version, and it isn't clear how the additional structure buys us anything.


(In reply to comment #2)
> Have you thought about how to hook up UI elements to keys?  Like, if I have a
> context menu item and want to add a key shortcut to it, this might be nice:
> 
>   require("context-menu").Item({
>     label: "My Item",
>     onClick: doMyCommand,
>     key: ???
>   });
> 
> Of course sometimes key shortcuts don't involve UI elements at all, so the
> approach in comment 1 is necessary, but I'm just wondering if you have thoughts
> about how to marry the two uses cases.  XUL does this pretty well IMO with
> <command> and <key>.  Maybe in our case <command>s are just functions.

`key` could mean two things here: the "hotkey" that invokes the command whether or not the menu is open (f.e. "Accel+C" for the Copy item), and the "accesskey" that invokes the command when the menu is open (f.e. "C" for the Copy item).

I think you were suggesting a way of registering a hotkey, which might indeed be a useful simplification, but I would bias toward composability to start with and just require developers to register the hotkey separately, i.e.:

  function doMyCommand() { ... }
  require("context-menu").Item({
    ...
    onClick: doMyCommand
  });
  require("shortcuts").register({
    ...
    onInvoke: doMyCommand
  });


(In reply to comment #10)
> I also was thinking something along the lines what chromeless has, but then I
> thought it was too implicit:
> http://mozilla.github.com/chromeless/#module/chromeless-kit/hotkey

It isn't clear how it's too implicit, although it is less structured.  Nevertheless, the format Chromeless requires seems structured enough, in that it permits no ambiguity regarding valid and invalid key specifications nor between multiple valid specifications.


> That being said I actually like the idea of registering command id's instead of
> functions as its easier to distribute across the processes. I don't really see
> how to register commands in chromeless yet but I'll investigate.

Commands would complicate this interface, particularly for simple addons.  And commands don't obviate the need for callbacks, they just move callback registration elsewhere (to some command registration interface).  And we should be able to make a callback-based interface work across processes using handles or by assigning unique IDs to callbacks.  So callbacks (which Chromeless also supports) are better.


> I also find it useful to have an ability to register multiple shortcuts at a
> time so I'd propose to tweak proposed API like this then:
> 
> require("{{whatever/id/will/be}}").{{register|define|create}}({
>    "accel-s": "command#1",
>    "meta-shift-i": "command#2"
> });

Adding this capability is fine as long as it doesn't complicate the simple case; otherwise, it'd be better to require addons that want to register multiple shortcuts to call the registration method multiple times, which is not much more complicated.


> require("{{whatever/id/will/be}}").{{bindings|shortcuts}}
> 
> Will be an object that contains all the registered commands. Note than reason
> to have an objects as a values is to make it possible to check for registered
> shortcuts without writing a parser.
> 
> {
>   "accel-s": {
>     modifiers: ["accel"],
>     key: "s"
>   },
>   "meta-shift-i": {
>     modifiers: ["meta", "shift"],
>     key: "i"
>   }
> }
> 
> Alternatively we may decide not to expose this at all and just throw on attempt
> to re-register shortcut.

Both options require addons to manage conflicts themselves.  But that is hard for them to do, because the possible conflicts, and their potential solutions, are highly variable.

We should push such management lower down the stack, into our platform, as well as up to the user as appropriate.

The simplest variant of this is to either silently fail, i.e. don't register the shortcut, but also don't throw an exception or otherwise indicate this to the addon; or alternately to override earlier-registered shortcuts with later-registered ones.

Both options are problematic for users, who may expect the conflicting shortcut to invoke a different command than the one it actually invokes.  But since the shortcut will invoke some command, at least it'll be evident why.

A more sophisticated approach would be to register a conflict-resolution handler that displays a popup panel to the user for selecting the desired command:

+------------------------------------------+
| You pressed Ctrl+C, which could mean:    |
|                                          |
| 1: [Copy]                                |
| 2: [Conjoin]                             |
|                                          |
| Press the number next to the command     |
| you meant to invoke!                     |
|                                          |
| [Cancel]                     (Configure) |
+------------------------------------------+

Optionally, we would then additionally provide users with a way to configure shortcuts to resolve such conflicts.

But that stuff is complicated and not necessary in a first version.  Let's start by silently failing (better) or overriding (good enough, if silent failure is hard to implement).


Overall, Chromeless's API seems the best here, with the following modifications:

1. The second parameter to `register()` and `unregister()` should only accept a callback function (we can add support for commands later as appropriate).

2. There should be no third `id` parameter (custom IDs don't make sense in an interface like ours that doesn't give API consumers direct access to the target DOM).

3. To satisfy the non-printable/keycode case, the `hotkey` parameter should also accept keycodes, as defined by <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMKeyEvent.idl>, but without the codes that represent modifier keys, and without the leading `DOM_VK_`, i.e. registering `Ctrl+Space` would invoke a callback when the user held down the Control key and pressed the Space key.

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #11)
> (In reply to comment #1)
> > require("shortcuts").register({
> >   key: "a",
> >   modifiers: ["SHIFT"],
> >   handler: function() {
> >     doStuff();
> >   }
> > });
> 
> This seems reasonable, but its additional structure makes it more complicated
> than Chromeless's version, and it isn't clear how the additional structure buys
> us anything.
> 
> 
> (In reply to comment #2)
> > Have you thought about how to hook up UI elements to keys?  Like, if I have a
> > context menu item and want to add a key shortcut to it, this might be nice:
> > 
> >   require("context-menu").Item({
> >     label: "My Item",
> >     onClick: doMyCommand,
> >     key: ???
> >   });
> > 
> > Of course sometimes key shortcuts don't involve UI elements at all, so the
> > approach in comment 1 is necessary, but I'm just wondering if you have thoughts
> > about how to marry the two uses cases.  XUL does this pretty well IMO with
> > <command> and <key>.  Maybe in our case <command>s are just functions.
> 
> `key` could mean two things here: the "hotkey" that invokes the command whether
> or not the menu is open (f.e. "Accel+C" for the Copy item), and the "accesskey"
> that invokes the command when the menu is open (f.e. "C" for the Copy item).
> 
> I think you were suggesting a way of registering a hotkey, which might indeed
> be a useful simplification, but I would bias toward composability to start with
> and just require developers to register the hotkey separately, i.e.:
> 
>   function doMyCommand() { ... }
>   require("context-menu").Item({
>     ...
>     onClick: doMyCommand
>   });
>   require("shortcuts").register({
>     ...
>     onInvoke: doMyCommand
>   });
> 
> 
> (In reply to comment #10)
> > I also was thinking something along the lines what chromeless has, but then I
> > thought it was too implicit:
> > http://mozilla.github.com/chromeless/#module/chromeless-kit/hotkey
> 
> It isn't clear how it's too implicit, although it is less structured. 
> Nevertheless, the format Chromeless requires seems structured enough, in that
> it permits no ambiguity regarding valid and invalid key specifications nor
> between multiple valid specifications.
> 

What I meant by that was that all of the following are the same:

control-alt-d
alt-control-d
control-alt-D

Also it's not clear how to define shortcut with "-"

control--      ?
control-minus  ?

Also one could define

control-d-b-c

That's why I thought that having explicit properties will be more helpful.

> 
> > That being said I actually like the idea of registering command id's instead of
> > functions as its easier to distribute across the processes. I don't really see
> > how to register commands in chromeless yet but I'll investigate.
> 
> Commands would complicate this interface, particularly for simple addons.  And
> commands don't obviate the need for callbacks, they just move callback
> registration elsewhere (to some command registration interface).  And we should
> be able to make a callback-based interface work across processes using handles
> or by assigning unique IDs to callbacks.  So callbacks (which Chromeless also
> supports) are better.
> 

Agreed.

> 
> > I also find it useful to have an ability to register multiple shortcuts at a
> > time so I'd propose to tweak proposed API like this then:
> > 
> > require("{{whatever/id/will/be}}").{{register|define|create}}({
> >    "accel-s": "command#1",
> >    "meta-shift-i": "command#2"
> > });
> 
> Adding this capability is fine as long as it doesn't complicate the simple
> case; otherwise, it'd be better to require addons that want to register
> multiple shortcuts to call the registration method multiple times, which is not
> much more complicated.
> 

I was just thinking that it's just harder to make mistake:

register({
  name: ...
  name: ...
})
 
then 

register(name, ...)
register(name, ...)

As in js you just won't be able to do the first one. Also it required less typing.

In any case I agree that advantage is not significant and anyone will be able to implement one API on top of another so it's fine by me either way.
 
> 
> > require("{{whatever/id/will/be}}").{{bindings|shortcuts}}
> > 
> > Will be an object that contains all the registered commands. Note than reason
> > to have an objects as a values is to make it possible to check for registered
> > shortcuts without writing a parser.
> > 
> > {
> >   "accel-s": {
> >     modifiers: ["accel"],
> >     key: "s"
> >   },
> >   "meta-shift-i": {
> >     modifiers: ["meta", "shift"],
> >     key: "i"
> >   }
> > }
> > 
> > Alternatively we may decide not to expose this at all and just throw on attempt
> > to re-register shortcut.
> 
> Both options require addons to manage conflicts themselves.  But that is hard
> for them to do, because the possible conflicts, and their potential solutions,
> are highly variable.
> 
> We should push such management lower down the stack, into our platform, as well
> as up to the user as appropriate.
> 
> The simplest variant of this is to either silently fail, i.e. don't register
> the shortcut, but also don't throw an exception or otherwise indicate this to
> the addon; or alternately to override earlier-registered shortcuts with
> later-registered ones.
> 
> Both options are problematic for users, who may expect the conflicting shortcut
> to invoke a different command than the one it actually invokes.  But since the
> shortcut will invoke some command, at least it'll be evident why.
> 
> A more sophisticated approach would be to register a conflict-resolution
> handler that displays a popup panel to the user for selecting the desired
> command:
> 
> +------------------------------------------+
> | You pressed Ctrl+C, which could mean:    |
> |                                          |
> | 1: [Copy]                                |
> | 2: [Conjoin]                             |
> |                                          |
> | Press the number next to the command     |
> | you meant to invoke!                     |
> |                                          |
> | [Cancel]                     (Configure) |
> +------------------------------------------+
> 
> Optionally, we would then additionally provide users with a way to configure
> shortcuts to resolve such conflicts.
> 
> But that stuff is complicated and not necessary in a first version.  Let's
> start by silently failing (better) or overriding (good enough, if silent
> failure is hard to implement).
> 

Yeah user conflict resolution will be overkill IMO for us and more importantly for a users. In any case since it's not on table for this version I'd prefer to keep at list some way to find out about conflicts silent override or failure just doesn't gives a chance to avoid conflicts. I would alternatively suggest:

1. Keep bindings / shortcuts object containing all registered shortcuts and fail silently on attempt to re-register.
2. Throw exception on attempt to re-register (optionally we can keep shortcuts object as well). 
3. Add method `isRegistered` or similar to check if shortcut is defined and fail silently on attempt to re-register.

> 
> Overall, Chromeless's API seems the best here, with the following
> modifications:
> 
> 1. The second parameter to `register()` and `unregister()` should only accept a
> callback function (we can add support for commands later as appropriate).
> 

Agreed.

> 2. There should be no third `id` parameter (custom IDs don't make sense in an
> interface like ours that doesn't give API consumers direct access to the target
> DOM).
> 

Agreed.

> 3. To satisfy the non-printable/keycode case, the `hotkey` parameter should
> also accept keycodes, as defined by
> <http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMKeyEvent.idl>,
> but without the codes that represent modifier keys, and without the leading
> `DOM_VK_`, i.e. registering `Ctrl+Space` would invoke a callback when the user
> held down the Control key and pressed the Space key.

Agreed, also please note that in chromeless it will be `control-space` instead.

I also think it will be beneficial for community if both jetpack and chromeless would stick to the same format.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to pull request

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 519474 [details]
Pointer to pull request

This pull request contains work in progress implementation, as tests and more docs have to be added.

Comment 15

[Dietrich Ayala (:dietrich)]

Comment 14 says this patch is a WIP, but you've asked for review. Is there more work that needs to happen here or no?

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Sorry it was confusing.

So when I made pull request and attached it here it was WIP. Later I updated it and asked for a review.

Comment 17

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Dietrich I've addressed all the comments noted in the review, also went through the API with Myk and adjusted implementation to address his comments. Pull request is ready for another review round.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 519474 [details]
Pointer to pull request

did another pass. r=me, with the noted changes fixed!