Open Bug 1348589 Opened 3 years ago Updated 10 months ago

[commands] Support dynamic commands

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(Not tracked)

UNCONFIRMED

People

(Reporter: ryan.hendrickson, Unassigned, Mentored)

References

Details

(Whiteboard: [design-decision-approved] triaged [commands])

Attachments

(1 file)

Some extensions won't have a static list of commands that they can declare in their manifest. Other extensions want to configure their own shortcuts for their commands, instead of relying on static platform defaults.

As an example of both issues, consider an extension that lets users assign hotkeys to certain websites. This hypothetical extension shouldn't have a limit on the number of websites that can be hotkeyed (other than the practical limit of the available unique key combinations), and should accept user choices for what the hotkeys are.

With the Add-on SDK, require('sdk/hotkeys').Hotkey objects could be created and destroyed to achieve the above. But there's no way to implement this with the current WebExtensions commands API, right?

I'd like methods on the commands API that enable new commands to be added and existing commands (for this extension) to be removed by WebExtension code. Modifying existing commands in place would be nice, but obviously optional given the other two.

Kinda related: bug 1303384, which asks for a UI to modify existing commands.
potentially related to work collin doing on extended keyboard
Whiteboard: [design-decision-needed] triaged
Hi Ryan, this has been added to the agenda for the June 6 WebExtension APIs triage meeting. Would you be able to join us? 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Archive

Agenda: https://docs.google.com/document/d/1pTNjK5i_8gHt3EeiUiu5KCUVeRcfwn7ybCPDBSx6CLM/edit#
Flags: needinfo?(amckay)
This would probably be part of the work in bug 1303384, it would be hard not to do one without the other. Eventually it might get duped to that, but yes this is something we should do.
Flags: needinfo?(amckay)
Whiteboard: [design-decision-needed] triaged → [design-decision-approved] triaged
Priority: -- → P3
Whiteboard: [design-decision-approved] triaged → [design-decision-approved] triaged [commands]
I'm thinking about adding commands.update to the API, and wrote out my plans and a couple of questions at https://gist.github.com/ericpromislow/79d9e1aeb4232499800a4de2502871b3 -- would appreciate some feedback .
I'm not sure how to persist prefs in this world -- is browser.storage.local extension-scoped? If so, it would be perfect to just use that, but I don't see any of the other API implementation files persisting anything.

BTW, this is for the API only -- bug 1303384 could use it.
Ah, local storage looks to be volatile. So I guess it's prefs:

XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                  "resource://gre/modules/Services.jsm");
...
var prefName = `extensions.commands.overrides.${this.id}`;

// use Strings.stringPref(prefName) set to a json encoding of the custom commands hash
Bob, the description of the handling of key bindings per the gist in comment 4 seems similar to the algorithm used by browser settings.  Do you think some of that work could be applied to this?
Mentor: mixedpuppy
Flags: needinfo?(bob.silverberg)
(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> Bob, the description of the handling of key bindings per the gist in comment
> 4 seems similar to the algorithm used by browser settings.  Do you think
> some of that work could be applied to this?

It does seem like the logic is similar, so yes, I think some of the work can be reused, but it doesn't sound like the ExtensionSettingsStore (ESS) could be used for this as it is currently implemented.

The ESS is designed to record a single setting for an extension. What the gist is suggesting is that a single extension would be overriding itself (command set via manifest vs. command set via commands.update()), whereas the ESS is designed to deal with one extension overriding another extension. It's not currently designed to deal with an extension storing multiple versions of the same setting, and needing to determine which one takes precedence.

The ESS does have the notion of a setting type, so commands could be stored there using a special "commands" type which would make it easier for looking up all the commands for an extension, but that doesn't address the issue just mentioned above.

It could be that something quite similar can be created to support this custom command hierarchy, or perhaps we want to revisit the design of the ESS to see if it can be altered to support this different use case.
Flags: needinfo?(bob.silverberg)
None of this deals with keyboard shortcut conflicts between extensions though. Is that in a separate bug somewhere?
I had a look at the experiments section, but it looked moire straightforward to implement this feature. Not sure where this goes from here, especially as it's a an addition to the API. Tested with my extension on macos and ubuntu. I don't have access to a Windows box with a dev environment, so I did nothing on that platform.
Attachment #8932594 - Flags: feedback?
Depends on: 1421811
Thanks for the patch, Eric!

I've been looking at adding a method to update the shortcut of a command as I just suggest in bug 1421811. I haven't solved the persistence problem yet, but I think using ExtensionSettingsStore rather than prefs would be a better solution for that.

For now I'm just looking to implement the setShortcut() API. I think at this point we haven't made a decision on if we should allow adding new commands yet.

Please let me know how the setShortcut() method looks in bug 1421811.
My code allowed adding new commands, but they're never registered. I could have thrown an exception...

I could look at the ExtensionSettingsStore, but it sounds like you're going to plow ahead with the 
commands.setShortcut method.
Comment on attachment 8932594 [details] [diff] [review]
Implement and test commands.update

Dropping feedback request with empty requestee as it looks like feedback has already been given.
Attachment #8932594 - Flags: feedback?
I wouldn't say feedback has been given. I haven't received any specific feedback on my patch, and there is no proposed patch for bug 1421811.
Mark, can you help out with extra feedback / ways forward here?
Flags: needinfo?(mstriemer)
Mark, any progress on this? Being able to configure shortcuts is needed for cross browser support where keyboard short cuts are needed.
I just got another bug report today that the windows shortcut in the manifest conflicts with another builtin and is ignored. I still haven't gotten any feedback on my patch. There's been no activity on this or a couple of other related bugs (one of which was filed in 2016, to emulate Chrome's way of setting shortcuts). 

So let me repeat again, there is no progress on this. I didn't make a WebExperiment on this because, quite frankly, it looked more straightforward to just modify the core code and submit a PR. It works for me. My users want it.  How do I escalate this?
There is progress, it's just not showing on the bug, but should start to show up shortly. Please be patient.
My main frustration here is that I solved the problem, at least to meet my own needs,
and don't know if you all could use some assistance.  I just keep getting bombarded
my bug reports from my users.
Sorry for the delay on this one, I've had a patch prepared for a while but didn't have time to loop back to it until now (work week, holidays and lately the flu).

I just added my patch to bug 1421811, it has some TODOs and FIXMEs and I have some notes that I wanted to add a couple more tests but I published it to show some progress here. The patch is rebased and I just confirmed that it is still working with an extension off of AMO.

The patch in that bug only supports updating an already defined shortcut. I believe that fixes the problems that have been encountered here, but if you have another use case you think we should support please comment.
Flags: needinfo?(mstriemer)
Product: Toolkit → WebExtensions
Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958
Component: Untriaged → General
You need to log in before you can comment on or make changes to this bug.