Consider support for tab key in WebExtension command shortcut keys

NEW
Unassigned

Status

()

Toolkit
WebExtensions: General
P5
normal
7 months ago
4 days ago

People

(Reporter: jkt, Unassigned)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
Currently command can accept many keys, however I see that tab isn't permitted:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands

In my extension: https://github.com/jonathanKingston/sea-containers

I would like to change how Ctrl+Tab and Ctrl+Shift+Tab change the tab ordering. Instead of going through the tab ordering as they were added, it would first pick from the current container in the usual order.

This would help with extensions wishing to experiment with how the browser moves from one tab to the next.
(Reporter)

Comment 1

7 months ago
I couldn't find the code that is restricting Ctrl+Tab however modifying the regex to allow Tab browser/components/extensions/schemas/commands.json makes Alt+Tab work.

It looked like browser/base/content/browser-ctrlTab.js was breaking the Ctrl+Tab setup however I'm not sure.
(Reporter)

Comment 2

7 months ago
Ah the code is here that breaks setting Ctrl+Tab: http://searchfox.org/mozilla-central/source/toolkit/content/widgets/tabbox.xml#153
(Reporter)

Updated

7 months ago
Assignee: nobody → jkt
Comment hidden (mozreview-request)
(Reporter)

Comment 4

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8ba2219001e
(Reporter)

Comment 5

7 months ago
Ah just realised there is also a test file here: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js

I will wait for this try to run and update with some tests there.
Comment hidden (mozreview-request)
(Reporter)

Comment 7

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee651e620ac
Comment hidden (mozreview-request)

Comment 9

7 months ago
mozreview-review
Comment on attachment 8867162 [details]
Bug 1364251 - Adding Tab key modifier support for onCommand in web extensions

https://reviewboard.mozilla.org/r/138760/#review142102

If this was simply "overriding Firefox keyboard bindings", it would be fine, since presumably we'll implement a UI for dealing with conflicts at some point in the future.  But this is also changing configuration and breaking browser functionality, so it needs additional considerations through triage and design-decision-needed.

::: browser/components/extensions/ext-commands.js:62
(Diff revision 3)
>    unregister() {
>      for (let window of windowTracker.browserWindows()) {
>        if (this.keysetsMap.has(window)) {
>          this.keysetsMap.get(window).remove();
>        }
> +      // Restore initial handleCtrlTab setup

This is missing a test.

::: browser/components/extensions/ext-commands.js:107
(Diff revision 3)
> +          handleCtrlTab = window.gBrowser.mTabBox.handleCtrlTab;
> +          window.gBrowser.mTabBox.handleCtrlTab = false;

What happens if you have two extensions?  Hint: they race to read and overwrite each other's values.
Attachment #8867162 - Flags: review-
Comment on attachment 8867162 [details]
Bug 1364251 - Adding Tab key modifier support for onCommand in web extensions

https://reviewboard.mozilla.org/r/138760/#review142716

Sorry for the delay. This looks good based on a quick glance, but clearing the review flag until you have an r+ from zombie.
Attachment #8867162 - Flags: review?(kmaglione+bmo)

Updated

6 months ago
Priority: -- → P2
Whiteboard: triaged
(Reporter)

Comment 11

3 months ago
Hey :zombie,

As you may have guessed I haven't been working on this, should we discuss the impacts on changing browser shortcuts in a design meeting? Personally I think there isn't any security concerns above the wide reaching abilities of extensions.

Agreed that managing shortcuts would also be something we really should have in the long run. Perhaps the interim to controlling them would just be listing them.

I can address your comments after there is approval from the design meeting that this can be implemented.
Flags: needinfo?(tomica)
Flags: needinfo?(cneiman)
Yes, I believe this should be discussed since it disables browser functionality (we have bug 1342584 for exposing this).
Flags: needinfo?(tomica)
Hi Jonathan, this has been added to the agenda for the September 5, 2017 WebExtensions APIs triage meeting. Let me know if you're able to attend! 

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

Agenda: https://docs.google.com/document/d/13gmYyN0qCjzV7YAsqGpbeeHr3al0yiWP7ayqKJPLS2w/edit#
Flags: needinfo?(cneiman)
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(amckay)

Comment 14

3 months ago
I don't think there's any problem with this. The concerns about overriding built in commands could probably be duped to bug 1325692 which covers checking the shortcut already exists and has got design-approval. I think our summary in the meeting, was that this could be landed as is if we do something with bug 1325692.
Flags: needinfo?(amckay)
Depends on: 1325692
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 15

3 months ago
Removing assignment until other bug is fixed.
Assignee: jkt → nobody
(Reporter)

Comment 16

3 months ago
Also it looks like the patch could probably use ExtensionSettings to manage which extension is in charge of the tab setting, perhaps there are other avenues this could be taken though.

Updated

4 days ago
Priority: P2 → P5
You need to log in before you can comment on or make changes to this bug.