Closed Bug 1364784 Opened 7 years ago Closed 6 years ago

WebExtensions: key combination for "command" limited to (Alt|Ctrl)+Shift

Categories

(WebExtensions :: Frontend, defect, P5)

Product:
WebExtensions
Component:
Frontend
Version:
53 Branch
Type:
defect

Tracking

(firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: aurora2borealis, Assigned: plaice.adam+persona, Mentored)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

Bug 1364784 - Allow more modifier combinations for webextensions commands key;
59 bytes, text/x-review-board-request
mstriemer
: review+
mixedpuppy
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170504105526

Steps to reproduce:

The key combination match pattern
               "^\s*(Alt|Ctrl|Com…
               mand|MacCtrl)\s*\+…
               \s*(Shift\s*\+\s*)…
               ?([A-Z0-9]|Comma|P…
               eriod|Home|End|Pag…
               eUp|PageDown|Space…
               |Insert|Delete|Up|…
               Down|Left|Right)\s…
               *$"

            does not allow a key combination of "Ctrl+Alt+AnyKey" for
the command, which is for Windows and Linux a often used key combination.


Actual results:

The addon is not loaded because of this syntax error in the manifest.json file


Expected results:

Change the key combination match pattern to allow Ctrl+Alt+AnyKey key combinations for "commands" .
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
good first bug for someone that knows regex
Keywords: good-first-bug
Priority: -- → P5
Whiteboard: triaged
Mentor: mwein
Status: UNCONFIRMED → NEW
Component: WebExtensions: General → WebExtensions: Frontend
Ever confirmed: true
Hi,

I'd like to work on this bug.
I've looked at the command key shortcuts documentation and regex and have some questions:

- While this bug mentions only Ctrl+Alt+AnyKey, bug 1387526 mentions longer combination of modifiers. Which one of these two should I implement? I'm assuming the latter. I've looked at XUL's <keyset> documentation, and it looks like it can handle an arbitrary combination of modifier keys like Ctrl+Alt+Shift+X.

- Should repeated combinations like "Alt+Alt+Alt+X" be allowed in manifest.json? Forbidding repetition will probably make the regex quite a bit more complex. I've verified these currently work just fine when I allow them at validation.
Flags: needinfo?(matthewjwein)
It looks like Chrome explicitly disallowed Ctrl+Alt:

https://developer.chrome.com/apps/commands

"Combinations that involve Ctrl+Alt are not permitted in order to avoid conflicts with the AltGr key."
There is a similar issue which is not mentioned here : The Tab key is not supported.

Before WebExt and Quantum, a wonderful extension called "Ctrl+Tab" existed and allowed us to switch from a tab to the last displayed one, and the last one before, etc (like in IDEs for example) and this is not feasible anymore because of these limitations (I was trying to port it to WebExt when I realized I simply couldn't).

Chrome disallowed this for the following reason : "Tab key was removed from list of supported keys in Chrome version 33 and above for accessibility reasons.".

Ok, why not? But, more, why not add a permission to allow, or not, an extension to modify accessibility commands and be able to improve keypress-based tab navigation?

For years, the best argument in favor of Firefox was the quality of its extensions compared to chrome ones. This looks like a leveling down of these.

There is something else which looks really disturbing to me: When you set "Ctrl+Tab" as the key to your command, it doesn't work, ok, but no error is thrown during the extension reloading so debugging is particularly painful.

Hoping you'll decide to implement something more developer-friendly than Chrome's chioces!


PS: Also, please tell me if I should open a new bug for the Tab case.
This is related to bug 1426958 which is suggesting the Ctrl+Alt combination but this is suggesting any combination of the 3. Making this depend on bug 1426958 but it may be easy enough to do this instead of that one.
Mentor: matthewjwein → mstriemer
Depends on: 1426958
Keywords: good-first-bug
Also I removed the good-first-bug flag since it seems like this will get a little hairy. Contributions are still welcome but probably not a first bug.

Please needinfo me if you'd like to work on this and have any questions.
Flags: needinfo?(matthewjwein)
This regex has gotten rather unwieldy, it might make sense to convert it to a format at this point.

It will be easier to see what's allowed in the code that way and I'm not sure how much help dumping out this huge regex would have in aiding someone debugging, either.
Thanks for the comments! You're right that such a long set of regexes is both unreadable and unhelpful for debugging.

I've converted it to a "format" and removed the negative lookahead from the regexes, as they were probably the least readable component. Unfortunately, the function is rather verbose and I can't think of simple ways to trim it down (without returning to the problematic regex).

Perhaps moving the entire logic back into the regular expressions might not result in _too_ unreadable code, now that we can split the regex into multiple lines? The two relevant regular expressions would then be:

    const BASICKEYS =
          new RegExp("^\\s*"
                     + "(?:(?:(Alt|Ctrl)(?:\\s*\\+\\s*(?!\\1)(Alt|Ctrl))?)"
                     + "|(?:(Alt|Command|MacCtrl)(?:\\s*\\+\\s*(?!\\3)(Alt|Command|MacCtrl))?))"
                     + "\\s*\\+\\s*"
                     + "(Shift\\s*\\+\\s*)?"
                     + "([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)\\s*$")

    const FUNCTIONKEYS =
          new RegExp("^\\s*"
                     + "(?:(?:(?:(Alt|Ctrl)(?:\\s*\\+\\s*(?!\\1)(Alt|Ctrl))?)"
                     + "|(?:(Alt|Command|MacCtrl)(?:\\s*\\+\\s*(?!\\3)(Alt|Command|MacCtrl))?))"
                     + "\\s*\\+\\s*)?"
                     + "(Shift\\s*\\+\\s*)?(F[1-9]|F1[0-2])\\s*$");

(the MEDIAKEYS would be unchanged).

The only thing that the format manifestShortcutKey function would need to (in this a case) is match the string against these three regular expressions.

Two further questions:

1. Is the position of the Shift modifier relative to the "primary" modifiers intentional? (i.e. is forbidding, say, "Shift+Ctrl+B", while "Ctrl+Shift+B" is allowed, deliberate or just an accident of the current regex implementation? FWIW chromium does allow combinations of the form "Shift+Ctrl+B".)

2. Is "Shifty+Y" supposed to have been "Shift+Y" in the test browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js ?
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js#10
I'm guessing "Shifty+Y" was supposed to be "Shift+Y". I don't think we want to allow shift on its own.

I think it would be clearer to drop most of the regexes and just keep the ones for the keys. Media keys, A-Z, PageUp, etc. Then you can check the modifiers against a set of allowed modifiers without having to deal with a regex.


I'm thinking more like:

  1. Split the string on + and trim the parts.
  2. Check if there is only one part and it's a valid media key.
    * Return if valid.
  3. Check that the last part is a valid key.
    * Error if not valid.
  4. Check that there are 1-2 modifiers and that they are valid.
    * Error if not all modifiers are valid, like Shifty.
    * Error if there are 0 or more than 2 modifiers.
    * Error if there is only 1 modifier and it's Shift.
  5. Check that the modifiers aren't the same.
    * Error if the modifiers are the same.
    * getModifiersAttribute [1] might be handy to use here.
    * Ctrl+MacCtrl would be different keys, but is a bit silly.
    * Ctrl+Command would be the same.
    * Ctrl+Ctrl is of course the same.
  6. String is valid, return it.

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/components/extensions/parent/ext-commands.js#292
Thanks very much for the suggestions! I've ended up not using getModifiersAttribute, as I'm not sure whether it's worth importing browser/components/extensions/parent/ext-commands.js just for the one function especially as it wouldn't help with the Ctrl+MacCtrl case (also fwiw I'm not entirely sure how one would import ext-commands.js — naively just doing 

ChromeUtils.import("resource:///chrome/browser/content/browser/parent/ext-commands.js"); 

doesn't work).
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

https://reviewboard.mozilla.org/r/245520/#review254872


Code analysis found 9 defects in this patch:
 - 9 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/Schemas.jsm:994
(Diff revision 3)
> +    let errorMessage = (`Value "${string}" must consist of `
> +                        + `either a combination of one or two modifiers, including `
> +                        + `a mandatory primary modifier and a key, separated by '+', `
> +                        + `or a media key. For details see: `
> +                        + `https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands#Key_combinations`);
> +    let shortcutKeys = string.split('+').map(s => s.trim())

Error: Strings must use doublequote. [eslint: quotes]

::: toolkit/components/extensions/Schemas.jsm:994
(Diff revision 3)
> +    let errorMessage = (`Value "${string}" must consist of `
> +                        + `either a combination of one or two modifiers, including `
> +                        + `a mandatory primary modifier and a key, separated by '+', `
> +                        + `or a media key. For details see: `
> +                        + `https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands#Key_combinations`);
> +    let shortcutKeys = string.split('+').map(s => s.trim())

Error: Missing semicolon. [eslint: semi]

::: toolkit/components/extensions/Schemas.jsm:996
(Diff revision 3)
> +                        + `a mandatory primary modifier and a key, separated by '+', `
> +                        + `or a media key. For details see: `
> +                        + `https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands#Key_combinations`);
> +    let shortcutKeys = string.split('+').map(s => s.trim())
> +    // number of modifiers
> +    let n = shortcutKeys.length - 1

Error: Missing semicolon. [eslint: semi]

::: toolkit/components/extensions/Schemas.jsm:1006
(Diff revision 3)
> +      throw new Error(errorMessage);
> +    }
> +    if (n > 2) {
> +      throw new Error(errorMessage);
> +    }
> +    for (var i = 0; i < n; i++) {

Error: Unexpected var, use let or const instead. [eslint: mozilla/var-only-at-top-level]

::: toolkit/components/extensions/Schemas.jsm:1014
(Diff revision 3)
> +      }
> +    }
> +    let primary_modifier_required = BASICKEYS.test(shortcutKeys[n]);
> +    if (primary_modifier_required) {
> +      if ((n == 0)
> +          || (n == 1) && (shortcutKeys[n-1] === "Shift")) {

Error: Unexpected mix of '||' and '&&'. [eslint: no-mixed-operators]

::: toolkit/components/extensions/Schemas.jsm:1014
(Diff revision 3)
> +      }
> +    }
> +    let primary_modifier_required = BASICKEYS.test(shortcutKeys[n]);
> +    if (primary_modifier_required) {
> +      if ((n == 0)
> +          || (n == 1) && (shortcutKeys[n-1] === "Shift")) {

Error: Unexpected mix of '||' and '&&'. [eslint: no-mixed-operators]

::: toolkit/components/extensions/Schemas.jsm:1014
(Diff revision 3)
> +      }
> +    }
> +    let primary_modifier_required = BASICKEYS.test(shortcutKeys[n]);
> +    if (primary_modifier_required) {
> +      if ((n == 0)
> +          || (n == 1) && (shortcutKeys[n-1] === "Shift")) {

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: toolkit/components/extensions/Schemas.jsm:1026
(Diff revision 3)
> +      }
> +      if ((shortcutKeys[0] == "Ctrl" &&
> +           (shortcutKeys[1] == "MacCtrl" || shortcutKeys[1] == "Command"))
> +          || (shortcutKeys[1] == "Ctrl" &&
> +              (shortcutKeys[0] == "MacCtrl" || shortcutKeys[0] == "Command"))) {
> +	throw new Error(errorMessage);

Error: Expected indentation of 8 spaces but found 1 tab. [eslint: indent]

::: toolkit/components/extensions/Schemas.jsm:1026
(Diff revision 3)
> +      }
> +      if ((shortcutKeys[0] == "Ctrl" &&
> +           (shortcutKeys[1] == "MacCtrl" || shortcutKeys[1] == "Command"))
> +          || (shortcutKeys[1] == "Ctrl" &&
> +              (shortcutKeys[0] == "MacCtrl" || shortcutKeys[0] == "Command"))) {
> +	throw new Error(errorMessage);

Error: Unexpected tab character. [eslint: no-tabs]
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

https://reviewboard.mozilla.org/r/245520/#review255546

This is looking much nicer than the regexes, thanks! A couple more comments, I think using the chrome modifier map from ext-commands.js would clean up the second half of this function a lot so you can move it to ExtensionUtils as I described below.

Let me know if you have any questions or concerns.

::: toolkit/components/extensions/Schemas.jsm:985
(Diff revision 4)
>      return string;
>    },
> +
> +  manifestShortcutKey(string, context) {
> +    // A valid shortcut key for a webextension manifest
> +    const MEDIAKEYS = /^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$/;

Can you please use `MEDIA_KEYS`, `BASIC_KEYS`, etc for these.

::: toolkit/components/extensions/Schemas.jsm:996
(Diff revision 4)
> +                        + `a mandatory primary modifier and a key, separated by '+', `
> +                        + `or a media key. For details see: `
> +                        + `https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands#Key_combinations`);
> +    let shortcutKeys = string.split("+").map(s => s.trim());
> +    // number of modifiers
> +    let n = shortcutKeys.length - 1;

Instead of storing `n` you can pop the key off the list which should make this a bit clearer. Might make sense to move it after `MEDIAKEYS` too.

```
if (MEDIAKEYS.test(string.trim())) {
  return string;
}

let modifiers = string.split("+").map(s => s.trim());
let key = modifiers.pop();

if (!BASICKEYS.test(key) && !FUNCTIONKEYS.test(key)) {
  throw new Error(errorMessage);
}
```

::: toolkit/components/extensions/Schemas.jsm:1006
(Diff revision 4)
> +      throw new Error(errorMessage);
> +    }
> +    if (n > 2) {
> +      throw new Error(errorMessage);
> +    }
> +    for (let i = 0; i < n; i++) {

If you bring in the modifier map from ext-commands.js then you can map to the chrome commands instead of testing each one against the regex.

You can move it from ext-commands.js to ExtensionUtils.jsm next to `makeWidgetId`. Both ext-commands.js and Schemas.jsm already import from ExtensionUtils so you can update the imports.

```
// Update the import in this file to:
var {
  chromeModifierKeyMap,
  DefaultMap,
  DefaultWeakMap,
} = ExtensionUtils;

// Then use it instead of this for loop.
let chromeModifiers = modifiers.map(m => chromeModifierKeyMap[m]);

// If the modifier wasn't found it will be undefined.
if (chromeModifiers.some(modifier => !modifier)) {
  throw new Error(errorMessage);
}
```

::: toolkit/components/extensions/Schemas.jsm:1022
(Diff revision 4)
> +    }
> +    if (n == 2) {
> +      if (shortcutKeys[0] === shortcutKeys[1]) {
> +        throw new Error(errorMessage);
> +      }
> +      if ((shortcutKeys[0] == "Ctrl" &&

You can use the chrome modifiers isntead of checking that the original commands would be the same here. Combining the `primary_modifier_required` test probably makes sense too.

```
if (modifiers.length == 1) {
  // Shift is only allowed on its own with function keys.
  if (chromeModifiers[0] == "shift" && !FUNCTIONKEYS.test(key)) {
    throw new Error(errorMessage);
  }
} else if (chromeModifiers[0] == chromeModifiers[1]) {
  throw new Error(errorMessage);
}

return string;
Attachment #8979361 - Flags: review?(mstriemer)
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

https://reviewboard.mozilla.org/r/245520/#review255546

Thanks for the clear and extensive help!

> You can use the chrome modifiers isntead of checking that the original commands would be the same here. Combining the `primary_modifier_required` test probably makes sense too.
> 
> ```
> if (modifiers.length == 1) {
>   // Shift is only allowed on its own with function keys.
>   if (chromeModifiers[0] == "shift" && !FUNCTIONKEYS.test(key)) {
>     throw new Error(errorMessage);
>   }
> } else if (chromeModifiers[0] == chromeModifiers[1]) {
>   throw new Error(errorMessage);
> }
> 
> return string;

I've slightly adapted this, allowing for the case of unmodified function keys.
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

https://reviewboard.mozilla.org/r/245520/#review256288

This is looking great! The switch statement was a good call, thanks!

Can you add some more tests to browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js to check things like Ctrl+Alt is allowed and Ctrl+Command is disallowed, function keys, etc? In my opinion there should have been more tests here in the first place but we should write them now to verify it's working the way we want.

I'll give this a quick look after the tests are added and then redirect to :mixedpuppy for a final review.
Attachment #8979361 - Flags: review?(mstriemer)
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

https://reviewboard.mozilla.org/r/245520/#review256288

I've added some tests to browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js and one to browser/components/extensions/test/browser/browser_ext_commands_onCommand.js. I hope that they're enough.

Thanks again for all the help!
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

https://reviewboard.mozilla.org/r/245520/#review256740

This is looks good, thanks for the tests!

::: browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js:7
(Diff revision 6)
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
>  
>  add_task(async function test_manifest_commands() {
> +  const shortcuts = [

Can you make two lists instead of one big object? That should make it easier to see which commands are valid and add more later if we want.

```
const validShortcuts = ["Ctrl+Y", "MacCtrl+Y", ...];
const invalidShortcuts = ["Shift+Y", "Y", ...];

// Then pull that loop body into a function.
for (let shortcut of validShortcuts) {
  validateShortcut({shortcut, valid: true});
}
for (let shortcut of invalidShortcuts) {
  validateShortcut({shortcut, valid: false});
}
Attachment #8979361 - Flags: review?(mstriemer) → review+
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

https://reviewboard.mozilla.org/r/245520/#review256740

> Can you make two lists instead of one big object? That should make it easier to see which commands are valid and add more later if we want.
> 
> ```
> const validShortcuts = ["Ctrl+Y", "MacCtrl+Y", ...];
> const invalidShortcuts = ["Shift+Y", "Y", ...];
> 
> // Then pull that loop body into a function.
> for (let shortcut of validShortcuts) {
>   validateShortcut({shortcut, valid: true});
> }
> for (let shortcut of invalidShortcuts) {
>   validateShortcut({shortcut, valid: false});
> }

That is indeed much cleaner! Thanks!
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

Thanks for the updates! I can't give final sign off so redirecting to Shane.
Attachment #8979361 - Flags: review?(mixedpuppy)
Is there anything else that I need to do with this (for the time being at least)?
Product: Toolkit → WebExtensions
Sorry for the delay.

I briefly discussed this with Shane last week and he said he'd take a look at it. We were at the Mozilla All-Hands last week which makes it harder to get to reviews. I don't think we need any changes right now.

Thanks!
Comment on attachment 8979361 [details]
Bug 1364784 - Allow more modifier combinations for webextensions commands key;

https://reviewboard.mozilla.org/r/245520/#review258506
Attachment #8979361 - Flags: review?(mixedpuppy) → review+
Thanks very much for looking at this and for all the help! Would it be possible for you to push the patch to the try server?
How do I deal with the OS X failed tests?

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

They appear to all be linked to bugs that are described as "Intermittent", but the tests themselves aren't categorised as intermittent. Unfortunately, I don't have access to a Mac.

(Alternatively, as this may be something that you don't usually work on, where/whom do I ask? IRC?)

Sorry for the trouble!
I don't see anything related in that try, so don't worry about it.   Thanks and sorry for the delay, you ran up against our all-hands.

Landing this should wait until merge happens next week.  After that you can add the checkin-needed keyword.
OK, I'll do that. Thanks again to you and Mark!
Unfortunately, it appears that I don't have the permission to add or change keywords, either, so could I ask somebody to add the "checkin-needed" keyword, please?
Assignee: nobody → plaice.adam+persona
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9912e5af51b
Allow more modifier combinations for webextensions commands key; r=mixedpuppy,mstriemer
https://hg.mozilla.org/mozilla-central/rev/e9912e5af51b
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
I was able to reproduce this issue on Firefox 62.0b9 (20180713213322) under Win 10 64-bit.

This issue is verified as fixed on Firefox 63.0a1(2018-07-17) under Win 10 64-bit Mac OS X 10.13.3. 

Tested the bug using both Alt/Shift + key combinations.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: