1272198 - Many "commands" shortcuts do not trigger onCommand

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Will Bamberg [:wbamberg]]

Attachment: commands-tester.zip

I've attached an zip containing an add-on, that creates lots of commands, triggered with different shortcuts:

Ctrl+[1-9]
Alt+[1-9]
Ctrl+Shift+[1-9]
Alt+Shift+[1-9]
Ctrl+Shift+[various letters]

On OS X, the only combinations that are reliably triggering onCommand are Alt+[1-9] and Ctrl+Shift+[various letters].

(I know many shortcut combinations are already reserved by the browser, but I'm surprised that so many of these are not getting through.

Comment 1

[:shell escalante]

if simple uplift - want to do.

Comment 2

[Matthew Wein [:mattw]]

Attachment: Bug 1272198 - Add tests for accepted modifiers.

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

Comment 3

[Matthew Wein [:mattw]]

Kris, the change is the same as the one in Bug 1271777, but it adds additional tests for all of the possible modifier combinations.

Comment 4

[Matthew Wein [:mattw]]

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

Comment 5

[Matthew Wein [:mattw]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

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

Comment 6

[Matthew Wein [:mattw]]

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

Comment 7

[Matthew Wein [:mattw]]

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

Comment 8

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/55654/#review54124

::: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js:10
(Diff revision 2)
> -  let win1 = yield BrowserTestUtils.openNewBrowserWindow();
> -  yield BrowserTestUtils.loadURI(win1.gBrowser.selectedBrowser, "about:robots");
> -  yield BrowserTestUtils.browserLoaded(win1.gBrowser.selectedBrowser);
>  
> -  let extension = ExtensionTestUtils.loadExtension({
> -    manifest: {
> +  const testCommands = [
> +    // Ctrl Shortcuts

I suspect that several of these are going to clash with builtin shortcuts.

::: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js:205
(Diff revision 2)
> +  yield BrowserTestUtils.browserLoaded(win1.gBrowser.selectedBrowser);
> +
> +  let commands = {};
> +
> +  for (let testCommand of testCommands) {
> +    if (testCommand.platform == "macosx" && AppConstants.platform != "macosx") {

We should still include the command in the manifest, we just shouldn't try to trigger it. Or, preferably, include an OS-X shortcut with MacCtrl a default shortcut with Ctrl.

Also, you don't have a `testCommand.platform` property, just an optional `testCommand.macosx` property.

::: browser/components/extensions/test/browser/browser_ext_commands_onCommand.js:247
(Diff revision 2)
> +    for (let testCommand of testCommands) {
> +      if (testCommand.platform == "macosx" && AppConstants.platform != "macosx") {
> +        continue;
> +      }
> +      EventUtils.synthesizeKey(testCommand.key, testCommand.modifiers);
> +      message = yield extension.awaitMessage("oncommand");

`let`

Comment 9

[dw-dev]

I am currently migrating my Tile Tabs add-on to the new WebExtensions API.

I have done some quite extensive testing of which shortcut key combinations work and which don't appear to work.

Ctrl+Shift+Letter - F,L,O,U,X,Y,Z work.  All of the other letters are already assigned to existing commands.

Ctrl+Shift+Number - None of these work and (as far as I know) they are not assigned to existing commands. Possible bug.

Ctrl+Shift+Special - Insert works. Space doesn't work.  I think all of the others are assigned to existing commands.

Alt+Shift+Letter - Most seem to work.

Alt+Shift+Number - 0-9 work.

Alt+Shift+Special - Arrow keys work. Space assigned to existing command. Others not tested.

Comment 10

[Matthew Wein [:mattw]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

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

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

https://reviewboard.mozilla.org/r/55654/#review62026

Comment 12

[Matthew Wein [:mattw]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

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

Comment 13

[Matthew Wein [:mattw]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55654/diff/4-5/

Comment 14

[Matthew Wein [:mattw]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55654/diff/5-6/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/5f60270bf1bb
Add tests for accepted modifiers. r=kmag

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f60270bf1bb

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/ed8e23b5e0c7
Backed out changeset 5f60270bf1bb for breaking mochitests

Comment 18

[Carsten Book [:Tomcat]]

sorry had to back this out for breaking mochitests like https://treeherder.mozilla.org/logviewer.html#?job_id=10666356&repo=fx-team

Comment 19

[Matthew Wein [:mattw]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55654/diff/6-7/

Comment 20

[Matthew Wein [:mattw]]

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

Comment 21

[Matthew Wein [:mattw]]

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

Comment 22

[Matthew Wein [:mattw]]

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

Comment 23

[Matthew Wein [:mattw]]

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

Comment 24

[Matthew Wein [:mattw]]

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

Comment 25

[Matthew Wein [:mattw]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55654/diff/7-8/

Comment 26

[Matthew Wein [:mattw]]

Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55654/diff/8-9/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/bb565b02a794
Add tests for accepted modifiers. r=kmag

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb565b02a794