1475043 - Allow commands.update() to un-set a shortcut, disabling the command

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Matthew Tylee Atkinson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_5) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/11.1.1 Safari/605.1.15

Steps to reproduce:

I tried to un-set a keyboard shortcut using browser.commands.update()


Actual results:

I was not able to un-set a keyboard shortcut for a command that already had one set.


// Problem

It is not possible to un-set the shortcut for a command programatically. This means that there's no way to disable commands to prevent conflict between extensions, nor is there any way to disable commands that users don't want to use.


// Background

Firefox allows an extension's manifest to declare any number of commands, and they can all have default keyboard shortcuts. There is no way for end-users, nor developers, to disable keyboard shortcuts, so conflicts could arise.

Chrome allows any number of commands to be specified in the manifest, but only four can have default keyboard shortcuts. Chrome's built-in UI for modifying extensions' shortcut keys allows the shortcuts to be removed, disabling the underlying commands.

I've been trying to achieve the same effect with browser.commands.update() in Firefox, but it doesn't seem to accept anything but a string with valid format. (Putting `shortcut: null` in the details object doesn't raise an exception, but it also doesn't have any effect.)


Expected results:

// Proposal

Support `shortcut: null` in the details object passed to command.update() - this would cause the shortcut to be un-set, disabling that command.

I think this would give Firefox a great balance between allowing extension developers to provide commands that work "out-of-the-box", but also allowing users to disable commands they don't need.


// Alternative: why not just declare the commands in the manifest without shortcuts?

This could be an option, but it has a big disadvantage: if the user ever sets a shortcut, they can't un-set it, so the command can't be disabled.

It also makes it harder to provide a consistent experience cross-browser, though Chrome's four-default-shortcuts limitation is acknowledged.


// Alternative: why not have a separate API for enabling/disabling commands?

In some ways this may be cleaner, but in others it's not:

1. It could complicate the Command type, by adding an "enabled" member or similar. At the least it would add a new function to the commands API.

2. The idea of un-setting a shortcut to disable a command has precedent in Chrome - and even though Chrome doesn't provide an API for this, it would mean that Chrome's extension keyboard shortcuts UI would match up with Firefox's commands.update() API, which seems neat.


// Related bugs

* Bug 1348589 asks for dynamic commands, which feels like a superset of this bug (as this one is only asking to enable/disable already-declared commands).
* Bug 1421811 is the implementation of the commands.update() method, which this builds upon.

Comment 1

[Mike Conca [:mconca]]

This sounds like a good idea. Mark and Emanuela, is there anything in the development or UX work going on right now that permits a keyboard shortcut to be removed?

Comment 2

[Mark Striemer [:mstriemer]]

I have noticed this as well in the work I'm doing to provide a UI for this in about:addons. I think it would be a good thing to support, but I don't believe there are any mocks for it right now.

Chrome provides an X in the edit shortcut UI that removes the assigned shortcut. Supporting `shortcut: null` and providing something similar in our new UI seems like a good idea.

Comment 3

[Mike Conca [:mconca]]

Thanks, Mark. I'm going to attach this to the meta bug for keyboard shortcuts.  I assume this one will become the API support piece, and that a new one will need to be submitted for the UI piece.

Comment 4

[Matthew Tylee Atkinson]

Thanks David, Mike and Mark for your updates.

I should add that there is a disadvantage to the approach I proposed: if the user wanted to temporarily disable a command, but also keep their currently-set shortcut (which may of course differ from the developer-suggested default one), in case they wanted to re-enable it later without having to remember the shortcut they were using, then they couldn't.

In order to support disabling a command whilst keeping a potentially user-customised shortcut, an explicit enable/disable API would be needed. If that approach were to be taken, I imagine the future browser-based UI to be like Chrome's, but instead of having an "X" button apparently inside the keyboard shortcut input, to completely clear it, there would be some sort of checkbox on that row of the form to disable the command without clearing the shortcut field.

I suggested the proposed approach on the basis that it seems to be the simplest and it feels like it would keep cross-browser API differences minimal; this particular UX benefit to the explicit enable/disable API has occurred to me since.

Comment 5

[YUKI "Piro" Hiroshi]

FYI: Even if this become fixed, this will still happen on ESR60. So I've made a helper library to provide keyboard shortcut features including deassignable default shortcuts as a workaround:
https://github.com/piroor/webextensions-lib-shortcut-customize-ui
The main purpose of the library is providing configuration UI for keybaord shortcuts. It reads shortcut definitions from "description" field of commands and set them as user-defined shortcut on the initial startup. Because they are not defined by "suggested_key" they can be cleared by browser.commands.reset(). I hope this will help addon authors.

Comment 6

[emanuela [ux]]

(In reply to Mark Striemer [:mstriemer] from comment #2)
> I have noticed this as well in the work I'm doing to provide a UI for this
> in about:addons. I think it would be a good thing to support, but I don't
> believe there are any mocks for it right now.
> 
> Chrome provides an X in the edit shortcut UI that removes the assigned
> shortcut. Supporting `shortcut: null` and providing something similar in our
> new UI seems like a good idea.

I agree with Mark. Should we include in the next iration? 

Yuki: I'm going to check your extension, thank you for contribution!

Comment 8

[Rob Wu [:robwu]]

The commands.getAll method is documented to be empty when a shortcut is not set. In Chrome, this is an empty string, in Firefox it's currently null. I'm going to fix this by using an empty string, and update commands.update to accept empty strings as well.

The schema also allows the implementation to distinguish between undefined/unset and null values. While it would also have been possible to accept null, I decided to use "" (empty string) instead, for consistency with the getAll method.

Any discussion about browser UI to change the shortcut can be moved to bug 1520119.

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1475043 - Allow "" as shortcut in commands.update

• Allow empty string ("") as a value for "shortcut" in the
  commands.update API.

• Use an empty string instead of null for an unset shortcut in
  commands.getAll, to match the documented behavior (and for
  consistency with Chrome).

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:robwu, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[Rob Wu [:robwu]]

I want to land this together with the work of bug 1520119.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68e50adb1734
Allow "" as shortcut in commands.update r=rpl

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/68e50adb1734

Comment 14

[Rob Wu [:robwu]]

Need to document that it is possible to clear a shortcut at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/commands/update

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6c2764c30d16
Port bug 1475043 - Allow "" as shortcut in commands.update. rs=bustage-fix

Comment 16

[Philipp Kewisch [:Fallen][☁️📆]]

The docs say "an empty string to clear the shortcut", is this enough in terms of documentation Rob?

Comment 17

[Rob Wu [:robwu]]

Yes, thanks Richard!