Bug 1364784 - Allow more modifier combinations for webextensions commands key; r?mstriemer
This allows keyboard shortcuts containing both "Ctrl" and "Alt" in the
manifest of webextensions (in the "commands" -> "suggested_key" key),
rather than just one of these modifiers. The equivalent combinations
on MacOS (any two of "Command", "Alt" and "MacCtrl") are also allowed.
Non-sensical combinations (such as "Ctrl+Command" or "Ctrl+Ctrl") are
forbidden.
MozReview-Commit-ID: 59tC2efLm5q
--- a/browser/components/extensions/parent/ext-commands.js
+++ b/browser/components/extensions/parent/ext-commands.js
@@ -3,16 +3,17 @@
"use strict";
ChromeUtils.defineModuleGetter(this, "ExtensionParent",
"resource://gre/modules/ExtensionParent.jsm");
ChromeUtils.defineModuleGetter(this, "ExtensionSettingsStore",
"resource://gre/modules/ExtensionSettingsStore.jsm");
var {
+ chromeModifierKeyMap,
ExtensionError,
} = ExtensionUtils;
var XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
const EXECUTE_PAGE_ACTION = "_execute_page_action";
const EXECUTE_BROWSER_ACTION = "_execute_browser_action";
const EXECUTE_SIDEBAR_ACTION = "_execute_sidebar_action";
@@ -285,25 +286,18 @@ this.commands = class extends ExtensionA
* ---------------------------------------
* ["Ctrl", "Shift"] | "accel shift"
* ["MacCtrl"] | "control"
*
* @param {Array} chromeModifiers The array of chrome modifiers.
* @returns {string} The constructed value for the Key's 'modifiers' attribute.
*/
getModifiersAttribute(chromeModifiers) {
- let modifiersMap = {
- "Alt": "alt",
- "Command": "accel",
- "Ctrl": "accel",
- "MacCtrl": "control",
- "Shift": "shift",
- };
return Array.from(chromeModifiers, modifier => {
- return modifiersMap[modifier];
+ return chromeModifierKeyMap[modifier];
}).join(" ");
}
getAPI(context) {
return {
commands: {
getAll: async () => {
let commands = await this.commands;
--- a/browser/components/extensions/schemas/commands.json
+++ b/browser/components/extensions/schemas/commands.json
@@ -3,30 +3,18 @@
// found in the LICENSE file.
[
{
"namespace": "manifest",
"types": [
{
"id": "KeyName",
- "choices": [
- {
- "type": "string",
- "pattern": "^\\s*(Alt|Ctrl|Command|MacCtrl)\\s*\\+\\s*(Shift\\s*\\+\\s*)?([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)\\s*$"
- },
- {
- "type": "string",
- "pattern": "^\\s*((Alt|Ctrl|Command|MacCtrl)\\s*\\+\\s*)?(Shift\\s*\\+\\s*)?(F[1-9]|F1[0-2])\\s*$"
- },
- {
- "type": "string",
- "pattern": "^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$"
- }
- ]
+ "type": "string",
+ "format": "manifestShortcutKey"
},
{
"$extend": "WebExtensionManifest",
"properties": {
"commands": {
"type": "object",
"optional": true,
"additionalProperties": {
--- a/browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
+++ b/browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
@@ -165,16 +165,25 @@ add_task(async function test_user_define
{
name: "toggle-ctrl-period",
shortcut: "Ctrl+Period",
key: "VK_PERIOD",
modifiers: {
accelKey: true,
},
},
+ {
+ name: "toggle-ctrl-alt-v",
+ shortcut: "Ctrl+Alt+V",
+ key: "V",
+ modifiers: {
+ accelKey: true,
+ altKey: true,
+ },
+ },
];
// Create a window before the extension is loaded.
let win1 = await BrowserTestUtils.openNewBrowserWindow();
await BrowserTestUtils.loadURI(win1.gBrowser.selectedBrowser, "about:robots");
await BrowserTestUtils.browserLoaded(win1.gBrowser.selectedBrowser);
--- a/browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_manifest_commands.js
@@ -1,25 +1,43 @@
/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
/* vim: set sts=2 sw=2 et tw=80: */
"use strict";
add_task(async function test_manifest_commands() {
- let normalized = await ExtensionTestUtils.normalizeManifest({
- "commands": {
- "toggle-feature": {
- "suggested_key": {"default": "Shifty+Y"},
- "description": "Send a 'toggle-feature' event to the extension",
- },
- },
- });
+ const validShortcuts = ["Ctrl+Y", "MacCtrl+Y", "Command+Y", "Alt+Shift+Y", "Ctrl+Alt+Y", "F1", "MediaNextTrack"];
+ const invalidShortcuts = ["Shift+Y", "Y", "Ctrl+Ctrl+Y", "Ctrl+Command+Y"];
- let expectedError = (
- String.raw`commands.toggle-feature.suggested_key.default: Value "Shifty+Y" must either: ` +
- String.raw`match the pattern /^\s*(Alt|Ctrl|Command|MacCtrl)\s*\+\s*(Shift\s*\+\s*)?([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)\s*$/, ` +
- String.raw`match the pattern /^\s*((Alt|Ctrl|Command|MacCtrl)\s*\+\s*)?(Shift\s*\+\s*)?(F[1-9]|F1[0-2])\s*$/, or ` +
- String.raw`match the pattern /^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$/`
- );
+ async function validateShortcut(shortcut, isValid) {
+ let normalized = await ExtensionTestUtils.normalizeManifest({
+ "commands": {
+ "toggle-feature": {
+ "suggested_key": {"default": shortcut},
+ "description": "Send a 'toggle-feature' event to the extension",
+ },
+ },
+ });
+ if (isValid) {
+ ok(!normalized.error,
+ "There should be no manifest errors.");
+ } else {
+ let expectedError = (
+ String.raw`Error processing commands.toggle-feature.suggested_key.default: Error: ` +
+ String.raw`Value "${shortcut}" must consist of ` +
+ String.raw`either a combination of one or two modifiers, including ` +
+ String.raw`a mandatory primary modifier and a key, separated by '+', ` +
+ String.raw`or a media key. For details see: ` +
+ String.raw`https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/commands#Key_combinations`
+ );
- ok(normalized.error.includes(expectedError),
- `The manifest error ${JSON.stringify(normalized.error)} must contain ${JSON.stringify(expectedError)}`);
+ ok(normalized.error.includes(expectedError),
+ `The manifest error ${JSON.stringify(normalized.error)} must contain ${JSON.stringify(expectedError)}`);
+ }
+ }
+
+ for (let shortcut of validShortcuts) {
+ validateShortcut(shortcut, true);
+ }
+ for (let shortcut of invalidShortcuts) {
+ validateShortcut(shortcut, false);
+ }
});
--- a/toolkit/components/extensions/ExtensionUtils.jsm
+++ b/toolkit/components/extensions/ExtensionUtils.jsm
@@ -671,18 +671,27 @@ function checkLoadURL(url, principal, op
}
function makeWidgetId(id) {
id = id.toLowerCase();
// FIXME: This allows for collisions.
return id.replace(/[^a-z0-9_-]/g, "_");
}
+const chromeModifierKeyMap = {
+ "Alt": "alt",
+ "Command": "accel",
+ "Ctrl": "accel",
+ "MacCtrl": "control",
+ "Shift": "shift",
+};
+
var ExtensionUtils = {
checkLoadURL,
+ chromeModifierKeyMap,
defineLazyGetter,
flushJarCache,
getConsole,
getInnerWindowID,
getMessageManager,
getUniqueId,
filterStack,
getWinUtils,
--- a/toolkit/components/extensions/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -10,16 +10,17 @@ const global = this;
ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
ChromeUtils.import("resource://gre/modules/Services.jsm");
ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]);
ChromeUtils.import("resource://gre/modules/ExtensionUtils.jsm");
var {
+ chromeModifierKeyMap,
DefaultMap,
DefaultWeakMap,
} = ExtensionUtils;
ChromeUtils.defineModuleGetter(this, "ExtensionParent",
"resource://gre/modules/ExtensionParent.jsm");
ChromeUtils.defineModuleGetter(this, "NetUtil",
"resource://gre/modules/NetUtil.jsm");
@@ -974,16 +975,68 @@ const FORMATS = {
// Our pattern just checks the format, we could still have invalid
// values (e.g., month=99 or month=02 and day=31). Let the Date
// constructor do the dirty work of validating.
if (isNaN(new Date(string))) {
throw new Error(`Invalid date string ${string}`);
}
return string;
},
+
+ manifestShortcutKey(string, context) {
+ // A valid shortcut key for a webextension manifest
+ const MEDIA_KEYS = /^(MediaNextTrack|MediaPlayPause|MediaPrevTrack|MediaStop)$/;
+ const BASIC_KEYS = /^([A-Z0-9]|Comma|Period|Home|End|PageUp|PageDown|Space|Insert|Delete|Up|Down|Left|Right)$/;
+ const FUNCTION_KEYS = /^(F[1-9]|F1[0-2])$/;
+ 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`);
+ if (MEDIA_KEYS.test(string.trim())) {
+ return string;
+ }
+
+ let modifiers = string.split("+").map(s => s.trim());
+ let key = modifiers.pop();
+
+ if (!BASIC_KEYS.test(key) && !FUNCTION_KEYS.test(key)) {
+ throw new Error(errorMessage);
+ }
+
+ 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);
+ }
+
+ switch (modifiers.length) {
+ case 0:
+ // A lack of modifiers is only allowed with function keys.
+ if (!FUNCTION_KEYS.test(key)) {
+ throw new Error(errorMessage);
+ }
+ break;
+ case 1:
+ // Shift is only allowed on its own with function keys.
+ if (chromeModifiers[0] == "shift" && !FUNCTION_KEYS.test(key)) {
+ throw new Error(errorMessage);
+ }
+ break;
+ case 2:
+ if (chromeModifiers[0] == chromeModifiers[1]) {
+ throw new Error(errorMessage);
+ }
+ break;
+ default:
+ throw new Error(errorMessage);
+ }
+
+ return string;
+ },
};
// Schema files contain namespaces, and each namespace contains types,
// properties, functions, and events. An Entry is a base class for
// types, properties, functions, and events.
class Entry {
constructor(schema = {}) {
/**