Closed Bug 1421811 Opened 7 years ago Closed 6 years ago

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

Categories

(WebExtensions :: General, enhancement, P2)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox60 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: mstriemer, Assigned: mstriemer)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

Bug 1421811 - Part 1: Support commands.update() to modify a command
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1421811 - Part 2: Support commands.reset() to reset a command's updates
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1421811 - Part 3: Update shortcut in sidebar on update
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Gijs
: review+
Details
commands_update-0.1.zip
5.79 KB, application/zip
Details
changed comand.jpg
13.82 KB, image/jpeg
Details 
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.
@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: 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)
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 :|
@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?
See Also: → 1418984
And see bug 1348589 with a proposed patch (not that it's flawless)
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)
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)
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.
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 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)
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).
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 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 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+
Keywords: checkin-needed
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
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 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)
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 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 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+
Keywords: checkin-needed
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
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.
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.
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)
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)
See Also: → 1442335
Attached image 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.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
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.
(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.
This looks great, thanks!
Flags: needinfo?(mstriemer)
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.
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.
Thank you.
Adding a dummy empty entry helped.
  "commands": {
    "_execute_browser_action": {}
  }
Product: Toolkit → WebExtensions
Depends on: 1523114
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: