Bug 1565854 - Abstract out about:addons Manage Shortcut shortcutKeyMap into a specialized DefaultMap subclass. r=mixedpuppy
authorLuca Greco <lgreco@mozilla.com>
Fri, 30 Apr 2021 19:39:27 +0000
changeset 578213 9d0d26dacd7f8d76a7805cffb98faec5cd6aa7fa
parent 578212 14ead18ad78b78eeca0e29d20e9dcbe9de1b0aaa
child 578214 b4ca0437aa3f2a2d5531ac3941cd847c6699abfb
push id38423
push usercbrindusan@mozilla.com
push dateSat, 01 May 2021 09:32:51 +0000
treeherdermozilla-central@cd81489560e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy
bugs1565854
milestone90.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 1565854 - Abstract out about:addons Manage Shortcut shortcutKeyMap into a specialized DefaultMap subclass. r=mixedpuppy This patch does prevent about:addons "Manage Extension Shortcuts" view to miss to catch shortcut conflicts due to the special meaning of "Ctrl" on macOS systems, and its implicit conversion into "Command": - As the [documentation on MDN explicitly mention](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/commands#key_combinations): > On Macs, "Ctrl" is interpreted as "Command", so if you actually need "Ctrl", specify "MacCtrl". - and so ExtensionShortcuts.buildKeyFromShortcut calls ShortcutUtils.getModifiersAttribute ([1]) which convert both Ctrl and Command are into the same accel modifier, which is what ends into the key element appended into the chrome window document. - but we still have the original "Ctrl" as part of the shortcut string that ExtensionShortcuts keeps into its map of the defined shortcuts loaded from the extension manifests and from the one stored on disk (through ExtensionSettingsStore) - when the about:addons "Manage Extension Shortcuts" view receive a new shortcut from the user, it does convert the accelKey property from the dom event received into "Command", and then it does check if the shortcut string exists in the map of the existing shortcuts. - when the extension using the same shortcut does use "Ctrl+..." in its commands suggested shortcuts, shortcuts.js (the "Manage Extension Shortcuts" view implementation script) wouldn't find the "Command+..." shortcut in the map of the existing shortcuts and it would assume that it is not used and let the user to set a duplicate shortcut without any of the warnings or errors expected. The approach in this patch does abstract out the shortcutKeyMap used in the about:addons shortcuts.js script into a specialized DefaultMap subclass, which does internally does a "platform specific" normalization of the given shortcutString to make sure that we don't miss duplicated shortcuts on macOS due to the implicit convertion of the Ctrl modifier key. [1]: https://searchfox.org/mozilla-central/rev/3aef835f6cb12e607154d56d68726767172571e4/toolkit/modules/ShortcutUtils.jsm#185-212 Differential Revision: https://phabricator.services.mozilla.com/D113698
toolkit/components/extensions/ExtensionShortcuts.jsm
toolkit/components/extensions/test/xpcshell/test_ExtensionShortcutKeyMap.js
toolkit/components/extensions/test/xpcshell/xpcshell.ini
toolkit/mozapps/extensions/content/shortcuts.js
toolkit/mozapps/extensions/test/browser/browser_shortcuts_duplicate_check.js
--- a/toolkit/components/extensions/ExtensionShortcuts.jsm
+++ b/toolkit/components/extensions/ExtensionShortcuts.jsm
@@ -1,15 +1,15 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 "use strict";
 
 /* exported ExtensionShortcuts */
-const EXPORTED_SYMBOLS = ["ExtensionShortcuts"];
+const EXPORTED_SYMBOLS = ["ExtensionShortcuts", "ExtensionShortcutKeyMap"];
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
 const { ExtensionCommon } = ChromeUtils.import(
   "resource://gre/modules/ExtensionCommon.jsm"
 );
 const { ExtensionUtils } = ChromeUtils.import(
@@ -43,27 +43,129 @@ XPCOMUtils.defineLazyGetter(this, "brows
 });
 XPCOMUtils.defineLazyGetter(this, "pageActionFor", () => {
   return ExtensionParent.apiManager.global.pageActionFor;
 });
 XPCOMUtils.defineLazyGetter(this, "sidebarActionFor", () => {
   return ExtensionParent.apiManager.global.sidebarActionFor;
 });
 
-const { ExtensionError } = ExtensionUtils;
+const { ExtensionError, DefaultMap } = ExtensionUtils;
 const { makeWidgetId } = ExtensionCommon;
 
 const EXECUTE_PAGE_ACTION = "_execute_page_action";
 const EXECUTE_BROWSER_ACTION = "_execute_browser_action";
 const EXECUTE_SIDEBAR_ACTION = "_execute_sidebar_action";
 
 function normalizeShortcut(shortcut) {
   return shortcut ? shortcut.replace(/\s+/g, "") : "";
 }
 
+class ExtensionShortcutKeyMap extends DefaultMap {
+  async buildForAddonIds(addonIds) {
+    this.clear();
+    for (const addonId of addonIds) {
+      const policy = WebExtensionPolicy.getByID(addonId);
+      if (policy?.extension?.shortcuts) {
+        const { shortcuts } = policy.extension;
+        for (const command of await shortcuts.allCommands()) {
+          this.recordShortcut(command.shortcut, policy.name, command.name);
+        }
+      }
+    }
+  }
+
+  recordShortcut(shortcutString, addonName, commandName) {
+    if (!shortcutString) {
+      return;
+    }
+
+    const valueSet = this.get(shortcutString);
+    valueSet.add({ addonName, commandName });
+  }
+
+  removeShortcut(shortcutString, addonName, commandName) {
+    if (!this.has(shortcutString)) {
+      return;
+    }
+
+    const valueSet = this.get(shortcutString);
+    for (const entry of valueSet.values()) {
+      if (entry.addonName === addonName && entry.commandName === commandName) {
+        valueSet.delete(entry);
+      }
+    }
+    if (valueSet.size === 0) {
+      this.delete(shortcutString);
+    }
+  }
+
+  getFirstAddonName(shortcutString) {
+    if (this.has(shortcutString)) {
+      return this.get(shortcutString)
+        .values()
+        .next().value.addonName;
+    }
+    return null;
+  }
+
+  has(shortcutString) {
+    const platformShortcut = this.getPlatformShortcutString(shortcutString);
+    return super.has(platformShortcut) && super.get(platformShortcut).size > 0;
+  }
+
+  // Class internals.
+
+  constructor() {
+    super();
+
+    // Overridden in some unit test to make it easier to cover some
+    // platform specific behaviors (in particular the platform specific.
+    // normalization of the shortcuts using the Ctrl modifier on macOS).
+    this._os = ExtensionParent.PlatformInfo.os;
+  }
+
+  defaultConstructor() {
+    return new Set();
+  }
+
+  getPlatformShortcutString(shortcutString) {
+    if (this._os == "mac") {
+      // when running on macos, make sure to also track in the shortcutKeyMap
+      // (which is used to check for duplicated shortcuts) a shortcut string
+      // that replace the `Ctrl` modifiers with the `Command` modified:
+      // they are going to be the same accel in the key element generated,
+      // by tracking both of them shortcut string value would confuse the about:addons "Manager Shortcuts"
+      // view and make it unable to correctly catch conflicts on mac
+      // (See bug 1565854).
+      shortcutString = shortcutString
+        .split("+")
+        .map(p => (p === "Ctrl" ? "Command" : p))
+        .join("+");
+    }
+
+    return shortcutString;
+  }
+
+  get(shortcutString) {
+    const platformShortcut = this.getPlatformShortcutString(shortcutString);
+    return super.get(platformShortcut);
+  }
+
+  add(shortcutString, addonCommandValue) {
+    const setValue = this.get(shortcutString);
+    setValue.add(addonCommandValue);
+  }
+
+  delete(shortcutString) {
+    const platformShortcut = this.getPlatformShortcutString(shortcutString);
+    super.delete(platformShortcut);
+  }
+}
+
 /**
  * An instance of this class is assigned to the shortcuts property of each
  * active webextension that has commands defined.
  *
  * It manages loading any updated shortcuts along with the ones defined in
  * the manifest and registering them to a browser window. It also provides
  * the list, update and reset APIs for the browser.commands interface and
  * the about:addons manage shortcuts page.
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/xpcshell/test_ExtensionShortcutKeyMap.js
@@ -0,0 +1,142 @@
+/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim: set sts=2 sw=2 et tw=80: */
+"use strict";
+
+const { ExtensionShortcutKeyMap } = ChromeUtils.import(
+  "resource://gre/modules/ExtensionShortcuts.jsm"
+);
+
+add_task(function test_ExtensionShortcutKeymap() {
+  const shortcutsMap = new ExtensionShortcutKeyMap();
+
+  shortcutsMap.recordShortcut("Ctrl+Shift+1", "Addon1", "Command1");
+  shortcutsMap.recordShortcut("Ctrl+Shift+1", "Addon2", "Command2");
+  shortcutsMap.recordShortcut("Ctrl+Alt+2", "Addon2", "Command3");
+  // Empty shortcut not expected to be recorded, just ignored.
+  shortcutsMap.recordShortcut("", "Addon3", "Command4");
+
+  Assert.equal(
+    shortcutsMap.size,
+    2,
+    "Got the expected number of shortcut entries"
+  );
+  Assert.deepEqual(
+    {
+      shortcutWithTwoExtensions: shortcutsMap.getFirstAddonName("Ctrl+Shift+1"),
+      shortcutWithOnlyOneExtension: shortcutsMap.getFirstAddonName(
+        "Ctrl+Alt+2"
+      ),
+      shortcutWithNoExtension: shortcutsMap.getFirstAddonName(""),
+    },
+    {
+      shortcutWithTwoExtensions: "Addon1",
+      shortcutWithOnlyOneExtension: "Addon2",
+      shortcutWithNoExtension: null,
+    },
+    "Got the expected results from getFirstAddonName calls"
+  );
+
+  Assert.deepEqual(
+    {
+      shortcutWithTwoExtensions: shortcutsMap.has("Ctrl+Shift+1"),
+      shortcutWithOnlyOneExtension: shortcutsMap.has("Ctrl+Alt+2"),
+      shortcutWithNoExtension: shortcutsMap.has(""),
+    },
+    {
+      shortcutWithTwoExtensions: true,
+      shortcutWithOnlyOneExtension: true,
+      shortcutWithNoExtension: false,
+    },
+    "Got the expected results from `has` calls"
+  );
+
+  shortcutsMap.removeShortcut("Ctrl+Shift+1", "Addon1", "Command1");
+  Assert.equal(
+    shortcutsMap.has("Ctrl+Shift+1"),
+    true,
+    "Expect shortcut to already exist after removing one duplicate"
+  );
+  Assert.equal(
+    shortcutsMap.getFirstAddonName("Ctrl+Shift+1"),
+    "Addon2",
+    "Expect getFirstAddonName to return the remaining addon name"
+  );
+
+  shortcutsMap.removeShortcut("Ctrl+Shift+1", "Addon2", "Command2");
+  Assert.equal(
+    shortcutsMap.has("Ctrl+Shift+1"),
+    false,
+    "Expect shortcut to not exist anymore after removing last entry"
+  );
+  Assert.equal(shortcutsMap.size, 1, "Got only one shortcut as expected");
+
+  shortcutsMap.clear();
+  Assert.equal(
+    shortcutsMap.size,
+    0,
+    "Got no shortcut as expected after clearing the map"
+  );
+});
+
+// This test verify that ExtensionShortcutKeyMap does catch duplicated
+// shortcut when the two modifiers strings are associated to the same
+// key (in particular on macOS where Ctrl and Command keys are both translated
+// in the same modifier in the keyboard shortcuts).
+add_task(function test_PlatformShortcutString() {
+  const shortcutsMap = new ExtensionShortcutKeyMap();
+
+  // Make the class instance behave like it would while running on macOS.
+  // (this is just for unit testing purpose, there is a separate integration
+  // test exercising this behavior in a real "Manage Extension Shortcut"
+  // about:addons view and only running on macOS, skipped on other platforms).
+  shortcutsMap._os = "mac";
+
+  shortcutsMap.recordShortcut("Ctrl+Shift+1", "Addon1", "MacCommand1");
+
+  Assert.deepEqual(
+    {
+      hasWithCtrl: shortcutsMap.has("Ctrl+Shift+1"),
+      hasWithCommand: shortcutsMap.has("Command+Shift+1"),
+    },
+    {
+      hasWithCtrl: true,
+      hasWithCommand: true,
+    },
+    "Got the expected results from `has` calls"
+  );
+
+  Assert.deepEqual(
+    {
+      nameWithCtrl: shortcutsMap.getFirstAddonName("Ctrl+Shift+1"),
+      nameWithCommand: shortcutsMap.getFirstAddonName("Command+Shift+1"),
+    },
+    {
+      nameWithCtrl: "Addon1",
+      nameWithCommand: "Addon1",
+    },
+    "Got the expected results from `getFirstAddonName` calls"
+  );
+
+  // Add a duplicate shortcut using Command instead of Ctrl and
+  // verify the expected behaviors.
+  shortcutsMap.recordShortcut("Command+Shift+1", "Addon2", "MacCommand2");
+  Assert.equal(shortcutsMap.size, 1, "Got still one shortcut as expected");
+  shortcutsMap.removeShortcut("Ctrl+Shift+1", "Addon1", "MacCommand1");
+  Assert.equal(shortcutsMap.size, 1, "Got still one shortcut as expected");
+  Assert.deepEqual(
+    {
+      nameWithCtrl: shortcutsMap.getFirstAddonName("Ctrl+Shift+1"),
+      nameWithCommand: shortcutsMap.getFirstAddonName("Command+Shift+1"),
+    },
+    {
+      nameWithCtrl: "Addon2",
+      nameWithCommand: "Addon2",
+    },
+    "Got the expected results from `getFirstAddonName` calls"
+  );
+
+  // Remove the entry added with a shortcut using "Command" by using the
+  // equivalent shortcut using Ctrl.
+  shortcutsMap.removeShortcut("Ctrl+Shift+1", "Addon2", "MacCommand2");
+  Assert.equal(shortcutsMap.size, 0, "Got no shortcut as expected");
+});
--- a/toolkit/components/extensions/test/xpcshell/xpcshell.ini
+++ b/toolkit/components/extensions/test/xpcshell/xpcshell.ini
@@ -30,16 +30,17 @@ prefs =
 #  - xpcshell-remote.ini
 #    For tests which should only run with both remote extensions and remote content.
 #  - xpcshell-content.ini
 #    For tests which rely on content pages, and should run in all configurations.
 #  - xpcshell-e10s.ini
 #    For tests which rely on content pages, and should only run with remote content
 #    but in-process extensions.
 
+[test_ExtensionShortcutKeyMap.js]
 [test_ExtensionStorageSync_migration_kinto.js]
 skip-if = os == 'android' # Not shipped on Android
 [test_MatchPattern.js]
 [test_StorageSyncService.js]
 skip-if = os == 'android' && processor == 'x86_64'
 [test_WebExtensionPolicy.js]
 
 [test_csp_custom_policies.js]
--- a/toolkit/mozapps/extensions/content/shortcuts.js
+++ b/toolkit/mozapps/extensions/content/shortcuts.js
@@ -3,28 +3,28 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 /* import-globals-from aboutaddonsCommon.js */
 
 "use strict";
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   ShortcutUtils: "resource://gre/modules/ShortcutUtils.jsm",
+  ExtensionShortcutKeyMap: "resource://gre/modules/ExtensionShortcuts.jsm",
 });
 
 {
   const FALLBACK_ICON = "chrome://mozapps/skin/extensions/extensionGeneric.svg";
   const COLLAPSE_OPTIONS = {
     limit: 5, // We only want to show 5 when collapsed.
     allowOver: 1, // Avoid collapsing to hide 1 row.
   };
-  const SHORTCUT_KEY_SEPARATOR = "|";
 
   let templatesLoaded = false;
-  let shortcutKeyMap = new Map();
+  let shortcutKeyMap = new ExtensionShortcutKeyMap();
   const templates = {};
 
   function loadTemplates() {
     if (templatesLoaded) {
       return;
     }
     templatesLoaded = true;
 
@@ -280,59 +280,30 @@ XPCOMUtils.defineLazyModuleGetters(this,
 
     return Object.entries(modifierMap)
       .filter(([key, isDown]) => isDown)
       .map(([key]) => key)
       .concat(getStringForEvent(e))
       .join("+");
   }
 
-  function buildDuplicateShortcutsMap(addons) {
-    return Promise.all(
-      addons.map(async addon => {
-        let extension = extensionForAddonId(addon.id);
-        if (extension && extension.shortcuts) {
-          let commands = await extension.shortcuts.allCommands();
-          for (let command of commands) {
-            recordShortcut(command.shortcut, addon.name, command.name);
-          }
-        }
-      })
-    );
+  async function buildDuplicateShortcutsMap(addons) {
+    await shortcutKeyMap.buildForAddonIds(addons.map(addon => addon.id));
   }
 
   function recordShortcut(shortcut, addonName, commandName) {
-    if (!shortcut) {
-      return;
-    }
-    let addons = shortcutKeyMap.get(shortcut);
-    let addonString = `${addonName}${SHORTCUT_KEY_SEPARATOR}${commandName}`;
-    if (addons) {
-      addons.add(addonString);
-    } else {
-      shortcutKeyMap.set(shortcut, new Set([addonString]));
-    }
+    shortcutKeyMap.recordShortcut(shortcut, addonName, commandName);
   }
 
   function removeShortcut(shortcut, addonName, commandName) {
-    let addons = shortcutKeyMap.get(shortcut);
-    let addonString = `${addonName}${SHORTCUT_KEY_SEPARATOR}${commandName}`;
-    if (addons) {
-      addons.delete(addonString);
-      if (addons.size === 0) {
-        shortcutKeyMap.delete(shortcut);
-      }
-    }
+    shortcutKeyMap.removeShortcut(shortcut, addonName, commandName);
   }
 
   function getAddonName(shortcut) {
-    let addons = shortcutKeyMap.get(shortcut);
-    // Get the first addon name with given shortcut.
-    let name = addons.values().next().value;
-    return name.split(SHORTCUT_KEY_SEPARATOR)[0];
+    return shortcutKeyMap.getFirstAddonName(shortcut);
   }
 
   function setDuplicateWarnings() {
     let warningHolder = document.getElementById("duplicate-warning-messages");
     clearWarnings(warningHolder);
     for (let [shortcut, addons] of shortcutKeyMap) {
       if (addons.size > 1) {
         warningHolder.appendChild(createDuplicateWarningBar(shortcut));
--- a/toolkit/mozapps/extensions/test/browser/browser_shortcuts_duplicate_check.js
+++ b/toolkit/mozapps/extensions/test/browser/browser_shortcuts_duplicate_check.js
@@ -144,8 +144,110 @@ add_task(async function testDuplicateSho
       );
     }
   });
 
   await closeView(win);
   await extension.unload();
   await extension2.unload();
 });
+
+add_task(async function testDuplicateShortcutOnMacOSCtrlKey() {
+  if (AppConstants.platform !== "macosx") {
+    ok(
+      true,
+      `Skipping macos specific test on platform ${AppConstants.platform}`
+    );
+    return;
+  }
+
+  const extension = ExtensionTestUtils.loadExtension({
+    useAddonManager: "temporary",
+    manifest: {
+      name: "Extension 1",
+      applications: {
+        gecko: { id: "extension1@mochi.test" },
+      },
+      commands: {
+        commandOne: {
+          // Cover expected mac normalized shortcut on default shortcut.
+          suggested_key: { default: "Ctrl+Shift+1" },
+        },
+        commandTwo: {
+          suggested_key: {
+            default: "Alt+Shift+2",
+            // Cover expected mac normalized shortcut on mac-specific shortcut.
+            mac: "Ctrl+Shift+2",
+          },
+        },
+      },
+    },
+  });
+
+  const extension2 = ExtensionTestUtils.loadExtension({
+    useAddonManager: "temporary",
+    manifest: {
+      name: "Extension 2",
+      applications: {
+        gecko: { id: "extension2@mochi.test" },
+      },
+      commands: {
+        anotherCommand: {},
+      },
+    },
+  });
+
+  await extension.startup();
+  await extension2.startup();
+
+  const win = await loadShortcutsView();
+  const doc = win.document;
+  const errorEl = doc.querySelector("addon-shortcuts .error-message");
+  const errorLabel = errorEl.querySelector(".error-message-label");
+
+  ok(
+    BrowserTestUtils.is_hidden(errorEl),
+    "Expect shortcut error element to be initially hidden"
+  );
+
+  const getShortcutInput = commandName =>
+    doc.querySelector(`input.shortcut-input[name="${commandName}"]`);
+
+  const assertDuplicateShortcutWarning = async msg => {
+    await TestUtils.waitForCondition(
+      () => BrowserTestUtils.is_visible(errorEl),
+      `Wait for the shortcut-duplicate error to be visible on ${msg}`
+    );
+    Assert.deepEqual(
+      document.l10n.getAttributes(errorLabel),
+      {
+        id: "shortcuts-exists",
+        args: { addon: "Extension 1" },
+      },
+      `Got the expected warning message on duplicate shortcut on ${msg}`
+    );
+  };
+
+  const clearWarning = async inputEl => {
+    anotherCommandInput.blur();
+    await TestUtils.waitForCondition(
+      () => BrowserTestUtils.is_hidden(errorEl),
+      "Wait for the shortcut-duplicate error to be hidden"
+    );
+  };
+
+  const anotherCommandInput = getShortcutInput("anotherCommand");
+  anotherCommandInput.focus();
+  EventUtils.synthesizeKey("1", { metaKey: true, shiftKey: true });
+
+  await assertDuplicateShortcutWarning("shortcut conflict with commandOne");
+  await clearWarning(anotherCommandInput);
+
+  anotherCommandInput.focus();
+  EventUtils.synthesizeKey("2", { metaKey: true, shiftKey: true });
+
+  await assertDuplicateShortcutWarning("shortcut conflict with commandTwo");
+  await clearWarning(anotherCommandInput);
+
+  await closeView(win);
+  await extension.unload();
+  await extension2.unload();
+});