1421811 - Provide a way for an extension to update the shortcut for its command

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Mark Striemer [:mstriemer]]

An extension may want to provide a way for a user to change the shortcut for a command. This would require a new API in commands to update the shortcut.

Proposed update:

    browser.commands.setShortcut(name, shortcut)

This API would update the shortcut for an already-defined command.

Comment 1

[Mark Striemer [:mstriemer]]

@mixedpuppy do you think we should provide a way to update the description while we're doing this?

    browser.commands.update(name, {description, shortcut})
    // or
    browser.commands.update({name, description, shortcut})

Comment 2

[Shane Caraveo (:mixedpuppy)]

Of those two, I would prefer browser.commands.update({name, description, shortcut})

I think we may as well allow description updates.  One might totally change the shortcut to something unrelated to the original description.

Comment 3

[Eric Promislow]

I didn't find a need to update the description - it felt like doing that would unnecessarily complicate the UI, when all the user wants to do is set/change the shortcut -- see the Chrome UI as a reference.

I've been hearing about this change from many Tabhunter users. Many have reported wanting to change the shortcut, or to even view it to verify that it is what they think it is. No one's mentioned the description :|

Comment 4

[Eric Promislow]

@mstreimer ... and the setShortcut command looks fine. Is coming up with a method name and signature that will be acceptable to the wider WebExtensions vendor community an issue?

Comment 5

[Eric Promislow]

And see bug 1348589 with a proposed patch (not that it's flawless)

Comment 6

[Mark Striemer [:mstriemer]]

As Eric mentioned, changing the description probably isn't all that useful. Adding and removing commands is probably a better solution for changing what a shortcut is for.

I think I'll go with just updating the shortcut for now and we can discuss if we want to support changing the description or adding and removing commands in another bug.

Does that work for you, Shane?

Comment 7

[Shane Caraveo (:mixedpuppy)]

If this can already be accomplished by remove/add, do we really need an update?  What is that providing over remove/add?

Comment 8

[Eric Promislow]

Currently the only methods on the commands object are getAll and the onCommand event handler. What is 'remove/add'?

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Eric Promislow from comment #8)
> Currently the only methods on the commands object are getAll and the
> onCommand event handler. What is 'remove/add'?

Sorry, just crash landed into the bug. That was a response to comment 6 where add/remove is mentioned.

Comment 10

[recover89]

How about adding a page to let the user specify the shortcuts they want?

Chrome lets you pick shortcuts on a dedicated page: chrome://extensions/configureCommands

Comment 11

[Mark Striemer [:mstriemer]]

Attachment: Bug 1421811 - Part 1: Support commands.update() to modify a command

Review commit: https://reviewboard.mozilla.org/r/213770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213770/

Comment 12

[Mark Striemer [:mstriemer]]

Comment on attachment 8943440 [details]
Bug 1421811 - Part 1: Support commands.update() to modify a command

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213770/diff/1-2/

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8943440 [details]
Bug 1421811 - Part 1: Support commands.update() to modify a command

https://reviewboard.mozilla.org/r/213770/#review220264

::: browser/components/extensions/ext-commands.js:118
(Diff revision 2)
>        });
>      }
>      return commands;
>    }
>  
> +  async loadCommandsFromStorage(id) {

s/id/extensionID/

::: browser/components/extensions/ext-commands.js:284
(Diff revision 2)
>                description: command.description,
>                shortcut: command.shortcut,
>              });
>            }));
>          },
> +        update: async ({name, description, shortcut}) => {

Lets also add a reset function so the extension can remove the setting entirely.

::: browser/components/extensions/schemas/commands.json:130
(Diff revision 2)
>      ],
>      "functions": [
>        {
> +        "name": "update",
> +        "type": "function",
> +        "async": "callback",

async: true.  

Also just fyi; async: callback use should have a callback function parameter (like getAll below).  We only use callback from compat with chrome.* apis.

::: browser/components/extensions/test/browser/browser_ext_commands_update.js:40
(Diff revision 2)
> +  });
> +}
> +
> +add_task(async function test_update_defined_command() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    useAddonManager: "permanent",

If something in this test fails and we don't unload, is it going to leak over to other tests?  If so, we should have a cleanup function that ensures the extensions are unloaded.

::: browser/components/extensions/test/browser/browser_ext_commands_update.js:146
(Diff revision 2)
> +  await extension.awaitMessage("ready");
> +  extension.sendMessage("run");
> +  await extension.awaitFinish("commands");
> +
> +  // Check that the <key> has been updated.
> +  // TODO: checkKey(extension.id, "Alt+Shift+P");

This TODO should either be resolved now, or if this is going to be a followup, "TODO Bug #".

::: browser/components/extensions/test/browser/browser_ext_commands_update.js:214
(Diff revision 2)
> +  await extension.unload();
> +  await updatedExtension.unload();
> +
> +  // Check that ESS is cleaned up on uninstall.
> +  storedCommands = ExtensionSettingsStore.getAllForExtension(extension.id, "commands");
> +  is(storedCommands.length, 0, "There are no stored commands after unload");

Perhaps put this into a cleanup function.

Comment 14

[Mark Striemer [:mstriemer]]

Attachment: Bug 1421811 - Part 2: Support commands.reset() to reset a command's updates

Review commit: https://reviewboard.mozilla.org/r/217006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217006/

Comment 15

[Mark Striemer [:mstriemer]]

Comment on attachment 8943440 [details]
Bug 1421811 - Part 1: Support commands.update() to modify a command

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213770/diff/2-3/

Comment 16

[Mark Striemer [:mstriemer]]

I noticed that my test add-on was logging the manifest's command shortcuts on startup and then they were being updated, so `this.commands` is a `Promise<Map>` now. Some of the register commands now take the commands to avoid making them async.

I added the reset function. So now an extension can update a command that it has set and reset it back to what was defined in the manifest (both the shortcut and description get reset).

Comment 17

[Mark Striemer [:mstriemer]]

Also the settings were keyed off the shortcut before because we thought we might want to use the ExtensionSettingsStore to track which extension is in control of a shortcut. This doesn't work though since two commands can have the same shortcut for the same extension. If we want to start tracking which extension is in control of a shortcut we'll have to do that in another patch.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8947259 [details]
Bug 1421811 - Part 2: Support commands.reset() to reset a command's updates

https://reviewboard.mozilla.org/r/217006/#review222994

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8943440 [details]
Bug 1421811 - Part 1: Support commands.update() to modify a command

https://reviewboard.mozilla.org/r/213770/#review222996

Comment 20

[Mark Striemer [:mstriemer]]

Comment on attachment 8943440 [details]
Bug 1421811 - Part 1: Support commands.update() to modify a command

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213770/diff/3-4/

Comment 21

[Mark Striemer [:mstriemer]]

Comment on attachment 8947259 [details]
Bug 1421811 - Part 2: Support commands.reset() to reset a command's updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217006/diff/1-2/

Comment 22

[Mark Striemer [:mstriemer]]

Comment on attachment 8943440 [details]
Bug 1421811 - Part 1: Support commands.update() to modify a command

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213770/diff/4-5/

Comment 23

[Mark Striemer [:mstriemer]]

Comment on attachment 8947259 [details]
Bug 1421811 - Part 2: Support commands.reset() to reset a command's updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217006/diff/2-3/

Comment 24

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d8f20e15bbbb3e3ca630a514594d0b82b0fd1d51 -d d2875a349020: rebasing 445493:d8f20e15bbbb "Bug 1421811 - Part 1: Support commands.update() to modify a command r=mixedpuppy"
merging browser/components/extensions/ext-browser.json
merging browser/components/extensions/ext-commands.js
merging browser/components/extensions/test/browser/browser-common.ini
warning: conflicts while merging browser/components/extensions/ext-commands.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 25

[Arthur Iakab [arthur_iakab]]

Mark Striemer please update the patch.

Comment 26

[Mark Striemer [:mstriemer]]

Attachment: Bug 1421811 - Part 3: Update shortcut in sidebar on update

Review commit: https://reviewboard.mozilla.org/r/218194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218194/

Comment 27

[Mark Striemer [:mstriemer]]

Comment on attachment 8943440 [details]
Bug 1421811 - Part 1: Support commands.update() to modify a command

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213770/diff/5-6/

Comment 28

[Mark Striemer [:mstriemer]]

Comment on attachment 8947259 [details]
Bug 1421811 - Part 2: Support commands.reset() to reset a command's updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217006/diff/3-4/

Comment 29

[Mark Striemer [:mstriemer]]

I added another commit after the rebase to handle updating the shortcut in the sidebar when the shortcut changes. This touches SidebarUI since it seems to make sense to have SidebarUI insert the extension button and set the shortcut rather than having extensions code reaching into SidebarUI's markup and notifying SidebarUI that something changed.

Gijs, I flagged you for review since you seemed to review code in this area last, but feel free to redirect.

Comment 30

[:Gijs (he/him)]

Comment on attachment 8948822 [details]
Bug 1421811 - Part 3: Update shortcut in sidebar on update

https://reviewboard.mozilla.org/r/218194/#review224150

I'm really sorry, I lack context here, but from what I can tell this isn't right. It'd be helpful if there was a test add-on for this review...

::: browser/base/content/browser-sidebar.js:58
(Diff revision 1)
>      this._icon = document.getElementById("sidebar-icon");
>      this._reversePositionButton = document.getElementById("sidebar-reverse-position");
> -    this._switcherPanel = document.getElementById("sidebarMenu-popup");
>      this._switcherTarget = document.getElementById("sidebar-switcher-target");
>      this._switcherArrow = document.getElementById("sidebar-switcher-arrow");
> +    // this._switcherPanel and this._switcherExtensionSeparator defined as lazy getters.

Out of interest, why make these lazy?

If we need to be able to call SidebarUI.updateShortcut() or whatever without .init() having been called then this seems like a footgun, because all the other element properties will be null. So at some point someone will come along and add code that relies on them being not null and not notice the race condition underlying that change.

So either we should make all of these lazy getters, or we should just ensure .init() has been called in all the methods that we're relying on here.

::: browser/base/content/browser-sidebar.js:133
(Diff revision 1)
> +    if (!keyId) {
> +      return;
> +    }
> -      let key = document.getElementById(keyId);
> +    let key = document.getElementById(keyId);
> -      if (!key) {
> +    if (!key) {
> -        continue;
> +      return;
> +    }
> +    button.setAttribute("shortcut", ShortcutUtils.prettifyShortcut(key));

It's a bit code-smelly that we have two methods here that do roughly the same thing (update the shortcut for a given button/key combination).

Can we unify these somehow?

::: browser/components/extensions/ext-sidebarAction.js
(Diff revision 1)
> -    let id = `ext-key-id-${this.id}`;
> -    broadcaster.setAttribute("key", id);

This won't show the key from the main menu's sidebar submenu. (View > Sidebars)

That doesn't seem right?

::: browser/components/extensions/ext-sidebarAction.js:185
(Diff revision 1)
>      // Insert a toolbarbutton for the sidebar dropdown selector.
>      let toolbarbutton = document.createElementNS(XUL_NS, "toolbarbutton");
>      toolbarbutton.setAttribute("id", this.buttonId);
>      toolbarbutton.setAttribute("observes", this.id);
>      toolbarbutton.setAttribute("class", "subviewbutton subviewbutton-iconic webextension-menuitem");
> -    this.setMenuIcon(toolbarbutton, details);
> +    let id = `ext-key-id-${this.id}`;

Why do we no longer need to set the icon on the toolbar item here?

::: browser/components/extensions/ext-sidebarAction.js:190
(Diff revision 1)
> -    this.setMenuIcon(toolbarbutton, details);
> +    let id = `ext-key-id-${this.id}`;
> +    toolbarbutton.setAttribute("key", id);
>  
>      document.getElementById("mainBroadcasterSet").appendChild(broadcaster);
>      document.getElementById("viewSidebarMenu").appendChild(menuitem);
> -    let separator = document.getElementById("sidebar-extensions-separator");
> +    SidebarUI.addExtensionButton(toolbarbutton);

You added this method but it only has 1 callsite, which is this one. Perhaps we could just leave this as it is?

::: browser/components/extensions/test/browser/browser_ext_commands_update.js:5
(Diff revision 1)
> -XPCOMUtils.defineLazyModuleGetter(this, "ExtensionSettingsStore",
> +ChromeUtils.defineModuleGetter(this, "ExtensionSettingsStore",
> -                                  "resource://gre/modules/ExtensionSettingsStore.jsm");
> +                               "resource://gre/modules/ExtensionSettingsStore.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
> +ChromeUtils.defineModuleGetter(this, "AddonManager",
> -                                  "resource://gre/modules/AddonManager.jsm");
> +                               "resource://gre/modules/AddonManager.jsm");

This should be in the earlier cset, I think.

Comment 31

[Mark Striemer [:mstriemer]]

Comment on attachment 8943440 [details]
Bug 1421811 - Part 1: Support commands.update() to modify a command

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213770/diff/6-7/

Comment 32

[Mark Striemer [:mstriemer]]

Comment on attachment 8947259 [details]
Bug 1421811 - Part 2: Support commands.reset() to reset a command's updates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217006/diff/4-5/

Comment 33

[Mark Striemer [:mstriemer]]

Comment on attachment 8948822 [details]
Bug 1421811 - Part 3: Update shortcut in sidebar on update

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218194/diff/1-2/

Comment 34

[Mark Striemer [:mstriemer]]

Attachment: commands_update-0.1.zip

Here's a test add-on you can use. Type a new shortcut into the browser action popup and click update. Check browser console for "acommand" and use the sidebar shortcut to toggle the sidebar. Shortcut is updated for the sidebar dropdown and in the View > Sidebar menu.

Example video: https://www.dropbox.com/s/w8ekps3ybvhlhvg/commands_update_shortcuts.mov?dl=0

Comment 35

[:Gijs (he/him)]

Comment on attachment 8948822 [details]
Bug 1421811 - Part 3: Update shortcut in sidebar on update

https://reviewboard.mozilla.org/r/218194/#review224406

This looks better, thanks! I'm still concerned that we're setting the key attribute twice. I have a suggestion that might avoid this below. If that doesn't work, please add a comment before setting the key attribute on the toolbarbutton explaining why this is necessary (which isn't immediately obvious to me from reading the patch).

I haven't reviewed the tests, I'll leave that for :mixedpuppy :-)

::: browser/components/extensions/ext-sidebarAction.js:187
(Diff revision 2)
>      // Insert a toolbarbutton for the sidebar dropdown selector.
>      let toolbarbutton = document.createElementNS(XUL_NS, "toolbarbutton");
>      toolbarbutton.setAttribute("id", this.buttonId);
>      toolbarbutton.setAttribute("observes", this.id);
>      toolbarbutton.setAttribute("class", "subviewbutton subviewbutton-iconic webextension-menuitem");
> +    toolbarbutton.setAttribute("key", id);

I'm still confused why this is necessary. The toolbarbutton has the 'observes' attribute, so it should get the right key attribute off the observed broadcaster automatically.

Perhaps you can avoid manually setting this by moving the updateShortcut call to after where you insert the toolbarbutton, so the button is in the DOM before you try to use the key attribute? If so do add a comment explaining why.

Comment 36

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8948822 [details]
Bug 1421811 - Part 3: Update shortcut in sidebar on update

https://reviewboard.mozilla.org/r/218194/#review224560

ditto on comment from Gijs

::: browser/components/extensions/test/browser/browser_ext_sidebarAction.js:160
(Diff revision 2)
>    await extension1.unload();
>    await extension2.unload();
>  });
> +
> +add_task(async function testShortcuts() {
> +  function getExtDataWithShortcut(shortcut) {

I would just make this a global getExtData function and have both tests use it. not a big deal

Comment 37

[Mark Striemer [:mstriemer]]

Comment on attachment 8948822 [details]
Bug 1421811 - Part 3: Update shortcut in sidebar on update

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218194/diff/2-3/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f515f0fb671
Part 1: Support commands.update() to modify a command r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c8f509a24089
Part 2: Support commands.reset() to reset a command's updates r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/827264a47790
Part 3: Update shortcut in sidebar on update r=Gijs,mixedpuppy

Comment 39

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4f515f0fb671
https://hg.mozilla.org/mozilla-central/rev/c8f509a24089
https://hg.mozilla.org/mozilla-central/rev/827264a47790

Comment 40

[YUKI "Piro" Hiroshi]

FYI: I've created a library to build customization UI for shortcuts, for addons.
https://github.com/piroor/webextensions-lib-shortcut-customize-ui
I think it will help addon authors until similar one is land on Firefox by default.

Comment 41

[kesselborn]

thanks a lot for this: I integrated this in my extension and everything works as expected.
@YUKI: thanks for the example -- that helped a lot.

Comment 42

[marius.santa]

I've set the same shortcut for both commands, and executing the shortcut only runs one of them, is this intended? Should we not make a warning for the user that shortcut is already assigned?

Comment 43

[Mark Striemer [:mstriemer]]

That was discussed, but is not currently supported.

I will file a bug to try and figure out if a shortcut is the "active" one for that binding.

Comment 44

[marius.santa]

Attachment: changed comand.jpg

Reproduced in Firefox 59.0a1 (20180101220336).
Retested and verified in Firefox 60.0a1 (20180305100134) on Windows 10 64Bit, Mac OS 10.13.2.

Comment 45

[Eric Promislow]

Dumb question -- how can I easily work with this?

I've got a mozilla-central dir and did an `hg pull`
but don't have these changes.  Or is there an
easy way to apply the final set of patches?

I tried doing `hg up central` as per
http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#feature-development
but got an error
"abort: unknown revision 'central'!"  It didn't
seem what I wanted.

The patches look fine, but I'd like to update
Tabhunter to test it out, and verify that my
changes will work on older versions.

Thanks for your patience.

Comment 46

[:Gijs (he/him)]

(In reply to Eric Promislow from comment #45)
> Dumb question -- how can I easily work with this?

Download a nightly (https://nightly.mozilla.org/ ) or 60 developer edition ( https://www.mozilla.org/en-US/firefox/channel/desktop/#developer ) build for your platform.

Comment 47

[Will Bamberg [:wbamberg]]

I've written some docs:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/commands/update
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/commands/reset

...and updated the example to use them: https://github.com/mdn/webextensions-examples/tree/master/commands

Please let me know if this covers it!

Comment 48

[Mark Striemer [:mstriemer]]

This looks great, thanks!

Comment 49

[wOxxOm]

Does this implementation support _execute_browser_action, _execute_page_action, _execute_sidebar_action?
Those are built-in command that show important extension parts like the popup.
AFAICT it's not, which is quite frustrating because we'll have to add a dummy command just for Firefox.

Comment 50

[Mark Striemer [:mstriemer]]

This implements support for commands that are defined in the manifest. If you have defined one of those commands then it should work.

You still need your own UI to let the user set the command shortcut though.

Comment 51

[wOxxOm]

Thank you.
Adding a dummy empty entry helped.
  "commands": {
    "_execute_browser_action": {}
  }