Many "commands" shortcuts do not trigger onCommand

RESOLVED FIXED in Firefox 50

Status

P1
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: wbamberg, Assigned: mattw)

Tracking

unspecified
mozilla50

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [chrome commands] triaged)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8751574 [details]
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → mwein

Comment 1

2 years ago
if simple uplift - want to do.
Priority: -- → P1
Whiteboard: [chrome commands] triaged
(Assignee)

Comment 2

2 years ago
Created attachment 8757108 [details]
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/
Attachment #8757108 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

2 years ago
Kris, the change is the same as the one in Bug 1271777, but it adds additional tests for all of the possible modifier combinations.
(Assignee)

Comment 5

2 years ago
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/
Attachment #8757108 - Attachment description: MozReview Request: Bug 1272198 - Add tests for accepted modifiers. r?kmag → MozReview Request: Bug 1272198 - Add tests for accepted modifiers.
(Assignee)

Updated

2 years ago
Attachment #8757108 - Flags: review?(kmaglione+bmo)
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

2 years ago
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.
(Assignee)

Comment 10

2 years ago
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/
Attachment #8757108 - Attachment description: MozReview Request: Bug 1272198 - Add tests for accepted modifiers. → Bug 1272198 - Add tests for accepted modifiers. `
Attachment #8757108 - Flags: review?(kmaglione+bmo)
Comment on attachment 8757108 [details]
Bug 1272198 - Add tests for accepted modifiers.

https://reviewboard.mozilla.org/r/55654/#review62026
Attachment #8757108 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 12

2 years ago
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/
Attachment #8757108 - Attachment description: Bug 1272198 - Add tests for accepted modifiers. ` → Bug 1272198 - Add tests for accepted modifiers.
(Assignee)

Comment 13

2 years ago
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/
(Assignee)

Comment 14

2 years ago
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/
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 15

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5f60270bf1bb
Add tests for accepted modifiers. r=kmag
Keywords: checkin-needed

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5f60270bf1bb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Comment 17

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/ed8e23b5e0c7
Backed out changeset 5f60270bf1bb for breaking mochitests
sorry had to back this out for breaking mochitests like https://treeherder.mozilla.org/logviewer.html#?job_id=10666356&repo=fx-team
Flags: needinfo?(mwein)

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 19

2 years ago
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/
(Assignee)

Updated

2 years ago
Flags: needinfo?(mwein)
Keywords: checkin-needed
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Comment 26

2 years ago
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

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/bb565b02a794
Add tests for accepted modifiers. r=kmag
Keywords: checkin-needed

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb565b02a794
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED

Updated

3 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.