Keyboard Shortcut API

RESOLVED FIXED in 1.0b5

Status

P1
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dietrich, Assigned: irakli)

Tracking

unspecified
1.0b5
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

9 years ago
The SDK needs an API for easily adding keyboard shortcuts.
(Reporter)

Comment 1

9 years ago
Created attachment 462464 [details] [diff] [review]
wip

WIP patch. sketching out a basic shortcut API.

currently looks something like:

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

Updated

9 years ago
OS: Linux → All
Hardware: x86 → All
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.
(Reporter)

Comment 3

8 years ago
Not working on this at the moment, unassigning.
Assignee: dietrich → nobody
Assignee: nobody → felipc
Status: NEW → ASSIGNED
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified

Comment 5

8 years ago
Created attachment 516156 [details]
Updated version of shortcuts.js based on attachment 462464 [details] [diff] [review]

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.
(Reporter)

Comment 6

8 years ago
Thanks Paul!

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

Updated

8 years ago
Duplicate of this bug: 638721
(Reporter)

Updated

8 years ago
Assignee: felipc → nobody
(Reporter)

Comment 8

8 years ago
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.
Assignee: nobody → rFobic
Created attachment 516876 [details]
Pointer to pull request
Attachment #516876 - Flags: review?(myk)
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.
(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.
Attachment #516876 - Flags: review?(myk) → review-
(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.
Created attachment 519474 [details]
Pointer to pull request
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.
Attachment #519474 - Flags: review?(dietrich)
(Reporter)

Comment 15

8 years ago
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?
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.
Priority: -- → P1
Target Milestone: --- → 1.0b5
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.
(Reporter)

Comment 18

8 years ago
Comment on attachment 519474 [details]
Pointer to pull request

did another pass. r=me, with the noted changes fixed!
Attachment #519474 - Flags: review?(dietrich) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.