Bug 1424708 - trigger a shortcut even when the number pad is used for the key r=mstriemer,mixedpuppy
authortb120 <tb120@users.noreply.github.com>
Mon, 15 Oct 2018 16:43:41 +0000
changeset 489680 900eabf8fceeec876fe998b1b91d5ddd09bb20f2
parent 489679 29ecf05f6a7bcb7d9f11fc0cdc9fe69ab4489f3e
child 489681 e18f1acd0782f6333ee892c7ebc9711d9a5e550a
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersmstriemer, mixedpuppy
bugs1424708
milestone64.0a1
Bug 1424708 - trigger a shortcut even when the number pad is used for the key r=mstriemer,mixedpuppy Prior to this patch, if a user attempted to use a keyboard shortcut that used a numeric key, the action would not be triggered if they used the numeric key from the number pad. This patch triggers the shortcut regardless of the source. Differential Revision: https://phabricator.services.mozilla.com/D8535
browser/components/extensions/parent/ext-commands.js
browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
browser/components/extensions/test/browser/browser_ext_commands_update.js
--- a/browser/components/extensions/parent/ext-commands.js
+++ b/browser/components/extensions/parent/ext-commands.js
@@ -155,16 +155,27 @@ this.commands = class extends ExtensionA
     let keyset = doc.createXULElement("keyset");
     keyset.id = `ext-keyset-id-${this.id}`;
     if (this.keysetsMap.has(window)) {
       this.keysetsMap.get(window).remove();
     }
     let sidebarKey;
     commands.forEach((command, name) => {
       if (command.shortcut) {
+        let parts = command.shortcut.split("+");
+
+        // The key is always the last element.
+        let key = parts.pop();
+
+        if (/^[0-9]$/.test(key)) {
+          let shortcutWithNumpad = command.shortcut.replace(/[0-9]$/, "Numpad$&");
+          let numpadKeyElement = this.buildKey(doc, name, shortcutWithNumpad);
+          keyset.appendChild(numpadKeyElement);
+        }
+
         let keyElement = this.buildKey(doc, name, command.shortcut);
         keyset.appendChild(keyElement);
         if (name == EXECUTE_SIDEBAR_ACTION) {
           sidebarKey = keyElement;
         }
       }
     });
     doc.documentElement.appendChild(keyset);
@@ -234,17 +245,20 @@ this.commands = class extends ExtensionA
 
     let parts = shortcut.split("+");
 
     // The key is always the last element.
     let chromeKey = parts.pop();
 
     // The modifiers are the remaining elements.
     keyElement.setAttribute("modifiers", this.getModifiersAttribute(parts));
-    if (name == EXECUTE_SIDEBAR_ACTION) {
+
+    // A keyElement with key "NumpadX" is created above and isn't from the
+    // manifest. The id will be set on the keyElement with key "X" only.
+    if (name == EXECUTE_SIDEBAR_ACTION && !chromeKey.startsWith("Numpad")) {
       let id = `ext-key-id-${this.id}-sidebar-action`;
       keyElement.setAttribute("id", id);
     }
 
     if (/^[A-Z]$/.test(chromeKey)) {
       // We use the key attribute for all single digits and characters.
       keyElement.setAttribute("key", chromeKey);
     } else {
@@ -264,17 +278,17 @@ this.commands = class extends ExtensionA
    *    ---------------------------------------
    *    "PageUP"  |  "VK_PAGE_UP"
    *    "Delete"  |  "VK_DELETE"
    *
    * @param {string} chromeKey The chrome key (e.g. "PageUp", "Space", ...)
    * @returns {string} The constructed value for the Key's 'keycode' attribute.
    */
   getKeycodeAttribute(chromeKey) {
-    if (/[0-9]/.test(chromeKey)) {
+    if (/^[0-9]$/.test(chromeKey)) {
       return `VK_${chromeKey}`;
     }
     return `VK${chromeKey.replace(/([A-Z])/g, "_$&").toUpperCase()}`;
   }
 
   /**
    * Determines the corresponding XUL modifiers from the chrome modifiers.
    *
--- a/browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
+++ b/browser/components/extensions/test/browser/browser_ext_commands_onCommand.js
@@ -185,16 +185,17 @@ add_task(async function test_user_define
   // 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);
 
   let commands = {};
   let isMac = AppConstants.platform == "macosx";
   let totalMacOnlyCommands = 0;
+  let numberNumericCommands = 4;
 
   for (let testCommand of testCommands) {
     let command = {
       suggested_key: {},
     };
 
     if (testCommand.shortcut) {
       command.suggested_key.default = testCommand.shortcut;
@@ -249,17 +250,17 @@ add_task(async function test_user_define
     }
   }
 
   // Create another window after the extension is loaded.
   let win2 = await BrowserTestUtils.openNewBrowserWindow();
   await BrowserTestUtils.loadURI(win2.gBrowser.selectedBrowser, "about:robots");
   await BrowserTestUtils.browserLoaded(win2.gBrowser.selectedBrowser);
 
-  let totalTestCommands = Object.keys(testCommands).length;
+  let totalTestCommands = Object.keys(testCommands).length + numberNumericCommands;
   let expectedCommandsRegistered = isMac ? totalTestCommands : totalTestCommands - totalMacOnlyCommands;
 
   // Confirm the keysets have been added to both windows.
   let keysetID = `ext-keyset-id-${makeWidgetId(extension.id)}`;
   let keyset = win1.document.getElementById(keysetID);
   ok(keyset != null, "Expected keyset to exist");
   is(keyset.children.length, expectedCommandsRegistered, "Expected keyset to have the correct number of children");
 
--- a/browser/components/extensions/test/browser/browser_ext_commands_update.js
+++ b/browser/components/extensions/test/browser/browser_ext_commands_update.js
@@ -107,26 +107,26 @@ add_task(async function test_update_defi
         browser.test.assertEq("foo", command.name, "The name is unchanged");
         browser.test.assertEq("The only command", command.description, "The description is updated");
         browser.test.assertEq("Ctrl+Shift+L", command.shortcut, "The shortcut is unchanged");
 
         // Update the description and shortcut.
         await browser.commands.update({
           name: "foo",
           description: "The new command",
-          shortcut: "   Alt+  Shift +P",
+          shortcut: "   Alt+  Shift +9",
         });
 
         // Test the updated shortcut.
         commands = await browser.commands.getAll();
         browser.test.assertEq(1, commands.length, "There is still 1 command");
         command = commands[0];
         browser.test.assertEq("foo", command.name, "The name is unchanged");
         browser.test.assertEq("The new command", command.description, "The description is updated");
-        browser.test.assertEq("Alt+Shift+P", command.shortcut, "The shortcut is updated");
+        browser.test.assertEq("Alt+Shift+9", command.shortcut, "The shortcut is updated");
 
         // Test a bad shortcut update.
         browser.test.assertThrows(
           () => browser.commands.update({name: "foo", shortcut: "Ctl+Shift+L"}),
           /Type error for parameter detail/,
           "It rejects for a bad shortcut");
 
         // Try to update a command that doesn't exist.
@@ -150,74 +150,86 @@ add_task(async function test_update_defi
   function checkKey(extensionId, shortcutKey, modifiers) {
     let keyset = extensionKeyset(extensionId);
     is(keyset.children.length, 1, "There is 1 key in the keyset");
     let key = keyset.children[0];
     is(key.getAttribute("key"), shortcutKey, "The key is correct");
     is(key.getAttribute("modifiers"), modifiers, "The modifiers are correct");
   }
 
+  function checkNumericKey(extensionId, key, modifiers) {
+    let keyset = extensionKeyset(extensionId);
+    is(keyset.children.length, 2, "There are 2 keys in the keyset now, 1 of which contains a keycode.");
+    let numpadKey = keyset.children[0];
+    is(numpadKey.getAttribute("keycode"), `VK_NUMPAD${key}`, "The numpad keycode is correct.");
+    is(numpadKey.getAttribute("modifiers"), modifiers, "The modifiers are correct");
+
+    let originalNumericKey = keyset.children[1];
+    is(originalNumericKey.getAttribute("keycode"), `VK_${key}`, "The original key is correct.");
+    is(originalNumericKey.getAttribute("modifiers"), modifiers, "The modifiers are correct");
+  }
+
   // Check that the <key> is set for the original shortcut.
   checkKey(extension.id, "I", "accel shift");
 
   await extension.awaitMessage("ready");
   extension.sendMessage("run");
   await extension.awaitFinish("commands");
 
-  // Check that the <key> has been updated.
-  checkKey(extension.id, "P", "alt shift");
+  // Check that the <keycode> has been updated.
+  checkNumericKey(extension.id, "9", "alt shift");
 
   // Check that the updated command is stored in ExtensionSettingsStore.
   let storedCommands = ExtensionSettingsStore.getAllForExtension(
     extension.id, "commands");
   is(storedCommands.length, 1, "There is only one stored command");
   let command = ExtensionSettingsStore.getSetting("commands", "foo", extension.id).value;
   is(command.description, "The new command", "The description is stored");
-  is(command.shortcut, "Alt+Shift+P", "The shortcut is stored");
+  is(command.shortcut, "Alt+Shift+9", "The shortcut is stored");
 
   // Check that the key is updated immediately.
   extension.sendMessage("update", {name: "foo", shortcut: "Ctrl+Shift+M"});
   await extension.awaitMessage("updateDone");
   checkKey(extension.id, "M", "accel shift");
 
   // Ensure all successive updates are stored.
   // Force the command to only have a description saved.
   await ExtensionSettingsStore.addSetting(
     extension.id, "commands", "foo", {description: "description only"});
   // This command now only has a description set in storage, also update the shortcut.
-  extension.sendMessage("update", {name: "foo", shortcut: "Alt+Shift+P"});
+  extension.sendMessage("update", {name: "foo", shortcut: "Alt+Shift+9"});
   await extension.awaitMessage("updateDone");
   let storedCommand = await ExtensionSettingsStore.getSetting(
     "commands", "foo", extension.id);
-  is(storedCommand.value.shortcut, "Alt+Shift+P", "The shortcut is saved correctly");
+  is(storedCommand.value.shortcut, "Alt+Shift+9", "The shortcut is saved correctly");
   is(storedCommand.value.description, "description only", "The description is saved correctly");
 
   // Calling browser.commands.reset("foo") should reset to manifest version.
   extension.sendMessage("reset", "foo");
   await extension.awaitMessage("resetDone");
 
   checkKey(extension.id, "I", "accel shift");
 
   // Check that enable/disable removes the keyset and reloads the saved command.
   let addon = await AddonManager.getAddonByID(extension.id);
   await disableAddon(addon);
   let keyset = extensionKeyset(extension.id);
   is(keyset, null, "The extension keyset is removed when disabled");
   // Add some commands to storage, only "foo" should get loaded.
   await ExtensionSettingsStore.addSetting(
-    extension.id, "commands", "foo", {shortcut: "Alt+Shift+P"});
+    extension.id, "commands", "foo", {shortcut: "Alt+Shift+9"});
   await ExtensionSettingsStore.addSetting(
     extension.id, "commands", "unknown", {shortcut: "Ctrl+Shift+P"});
   storedCommands = ExtensionSettingsStore.getAllForExtension(extension.id, "commands");
   is(storedCommands.length, 2, "There are now 2 commands stored");
   await enableAddon(addon);
   // Wait for the keyset to appear (it's async on enable).
   await TestUtils.waitForCondition(() => extensionKeyset(extension.id));
   // The keyset is back with the value from ExtensionSettingsStore.
-  checkKey(extension.id, "P", "alt shift");
+  checkNumericKey(extension.id, "9", "alt shift");
 
   // Check that an update to a shortcut in the manifest is mapped correctly.
   updatedExtension = ExtensionTestUtils.loadExtension({
     useAddonManager: "permanent",
     manifest: {
       version: "1.0",
       applications: {gecko: {id: "commands@mochi.test"}},
       commands: {
@@ -229,17 +241,17 @@ add_task(async function test_update_defi
         },
       },
     },
   });
   await updatedExtension.startup();
 
   await TestUtils.waitForCondition(() => extensionKeyset(extension.id));
   // Shortcut is unchanged since it was previously updated.
-  checkKey(extension.id, "P", "alt shift");
+  checkNumericKey(extension.id, "9", "alt shift");
 });
 
 add_task(async function updateSidebarCommand() {
   let extension = ExtensionTestUtils.loadExtension({
     useAddonManager: "temporary",
     manifest: {
       commands: {
         _execute_sidebar_action: {