1364251 - Consider support for tab key in WebExtension command shortcut keys

Status: NEW

(WebExtensions :: General, enhancement, P5)

Description

[Jonathan Kingston [:jkt] he/him]

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.

Comment 1

[Jonathan Kingston [:jkt] he/him]

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.

Comment 2

[Jonathan Kingston [:jkt] he/him]

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

Comment 3

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1364251 - Adding Tab key modifier support for onCommand in web extensions

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

Comment 4

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8ba2219001e

Comment 5

[Jonathan Kingston [:jkt] he/him]

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 6

[Jonathan Kingston [:jkt] he/him]

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

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

Comment 7

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee651e620ac

Comment 8

[Jonathan Kingston [:jkt] he/him]

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

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

Comment 9

[Tomislav Jovanovic :zombie]

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.

Comment 10

[Kris Maglione [:kmag]]

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.

Comment 11

[Jonathan Kingston [:jkt] he/him]

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.

Comment 12

[Tomislav Jovanovic :zombie]

Yes, I believe this should be discussed since it disables browser functionality (we have bug 1342584 for exposing this).

Comment 13

[Caitlin Neiman [:caitmuenster]]

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#

Comment 14

[Andy McKay]

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.

Comment 15

[Jonathan Kingston [:jkt] he/him]

Removing assignment until other bug is fixed.

Comment 16

[Jonathan Kingston [:jkt] he/him]

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.