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

VERIFIED FIXED in Firefox 60

Status

P2
normal
VERIFIED FIXED
a year ago
5 months ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

(Blocks: 3 bugs, {dev-doc-complete})

unspecified
mozilla60
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox60 verified)

Details

Attachments

(5 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
@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})
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

a year ago
Assignee: nobody → mstriemer
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.
Flags: needinfo?(mixedpuppy)

Comment 3

a year ago
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

a year ago
@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?

Updated

a year ago
See Also: → bug 1418984

Comment 5

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

Comment 6

a year ago
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?
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

a year ago
Blocks: 1215061
If this can already be accomplished by remove/add, do we really need an update?  What is that providing over remove/add?
Flags: needinfo?(mixedpuppy)

Comment 8

a year ago
Currently the only methods on the commands object are getAll and the onCommand event handler. What is 'remove/add'?
(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

11 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

10 months ago
mozreview-review
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.
Attachment #8943440 - Flags: review?(mixedpuppy)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

10 months ago
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).
(Assignee)

Comment 17

10 months ago
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

10 months ago
mozreview-review
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
Attachment #8947259 - Flags: review?(mixedpuppy) → review+

Comment 19

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

https://reviewboard.mozilla.org/r/213770/#review222996
Attachment #8943440 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 24

10 months ago
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)
Mark Striemer please update the patch.
Flags: needinfo?(mstriemer)
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

9 months ago
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.
Flags: needinfo?(mstriemer)

Comment 30

9 months ago
mozreview-review
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.
Attachment #8948822 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

9 months ago
Created attachment 8949228 [details]
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

9 months ago
mozreview-review
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.
Attachment #8948822 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 36

9 months ago
mozreview-review
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
Attachment #8948822 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 38

9 months ago
Pushed by archaeopteryx@coole-files.de:
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
Keywords: checkin-needed

Comment 39

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f515f0fb671
https://hg.mozilla.org/mozilla-central/rev/c8f509a24089
https://hg.mozilla.org/mozilla-central/rev/827264a47790
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Comment 40

9 months ago
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.
Keywords: dev-doc-needed

Comment 41

9 months ago
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

9 months ago
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?
Flags: needinfo?(mstriemer)
(Assignee)

Comment 43

9 months ago
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.
Flags: needinfo?(mstriemer)
(Assignee)

Updated

9 months ago
See Also: → bug 1442335

Comment 44

9 months ago
Created attachment 8956114 [details]
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.

Updated

9 months ago
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified

Comment 45

8 months ago
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

8 months ago
(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.
(Assignee)

Comment 48

8 months ago
This looks great, thanks!
Flags: needinfo?(mstriemer)
Keywords: dev-doc-needed → dev-doc-complete

Comment 49

7 months ago
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.
(Assignee)

Comment 50

7 months ago
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

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

Updated

5 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.