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 441308 900eabf8fceeec876fe998b1b91d5ddd09bb20f2
parent 441307 29ecf05f6a7bcb7d9f11fc0cdc9fe69ab4489f3e
child 441309 e18f1acd0782f6333ee892c7ebc9711d9a5e550a
push id71022
push usermstriemer@mozilla.com
push dateMon, 15 Oct 2018 19:47:54 +0000
treeherderautoland@900eabf8fcee [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstriemer, mixedpuppy
bugs1424708
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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: {