Closed Bug 1475043 Opened 6 years ago Closed 4 years ago

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

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(firefox74 fixed)

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: mbugzillam, Assigned: robwu)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

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.
Flags: needinfo?(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?
Severity: normal → enhancement
Flags: needinfo?(mstriemer)
Flags: needinfo?(mconca)
Flags: needinfo?(emanuela)
Priority: -- → P3
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.
Flags: needinfo?(mstriemer)
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.
Blocks: 1215061
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.
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.
(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!
Flags: needinfo?(emanuela)

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.

Assignee: nobody → rob
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
See Also: → 1520119
  • 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).

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.

Flags: needinfo?(rob)

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

Flags: needinfo?(rob)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/68e50adb1734
Allow "" as shortcut in commands.update r=rpl
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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

Flags: qe-verify-
Keywords: dev-doc-needed
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/6c2764c30d16
Port bug 1475043 - Allow "" as shortcut in commands.update. rs=bustage-fix

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

Flags: needinfo?(rob)

Yes, thanks Richard!

Flags: needinfo?(rob)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: