Bug 1522227 - Prevent assigning a shortcut that is already assigned r=mstriemer,aswan,flod
authorTrishul <trishul.goel@gmail.com>
Thu, 04 Apr 2019 05:22:28 +0000
changeset 467960 a9aed50dd3ebc0c725736bad607845928cfbd60b
parent 467959 e1d21ee0fa85adcacf0497e8e9b4122631241cd8
child 467961 278767179a87ed8d32d87cf70a44fe70d9ba6ef4
push id112667
push useraiakab@mozilla.com
push dateThu, 04 Apr 2019 16:12:45 +0000
treeherdermozilla-inbound@230bb363f2f3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstriemer, aswan, flod
bugs1522227
milestone68.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 1522227 - Prevent assigning a shortcut that is already assigned r=mstriemer,aswan,flod Prevent assigning a shortcut that is already assigned Differential Revision: https://phabricator.services.mozilla.com/D21327
toolkit/locales/en-US/toolkit/about/aboutAddons.ftl
toolkit/mozapps/extensions/content/shortcuts.css
toolkit/mozapps/extensions/content/shortcuts.js
toolkit/mozapps/extensions/test/browser/browser_manage_shortcuts.js
--- a/toolkit/locales/en-US/toolkit/about/aboutAddons.ftl
+++ b/toolkit/locales/en-US/toolkit/about/aboutAddons.ftl
@@ -319,16 +319,20 @@ shortcuts-browserAction = Activate exten
 shortcuts-pageAction = Activate page action
 shortcuts-sidebarAction = Toggle the sidebar
 
 shortcuts-modifier-mac = Include Ctrl, Alt, or ⌘
 shortcuts-modifier-other = Include Ctrl or Alt
 shortcuts-invalid = Invalid combination
 shortcuts-letter = Type a letter
 shortcuts-system = Can’t override a { -brand-short-name } shortcut
+# String displayed when a keyboard shortcut is already used by another add-on
+# Variables:
+#   $addon (string) - Name of the add-on
+shortcuts-exists = Already in use by { $addon }
 
 shortcuts-card-expand-button =
     { $numberToShow ->
         *[other] Show { $numberToShow } More
     }
 
 shortcuts-card-collapse-button = Show Less
 
--- a/toolkit/mozapps/extensions/content/shortcuts.css
+++ b/toolkit/mozapps/extensions/content/shortcuts.css
@@ -77,17 +77,20 @@
   stroke: var(--error-background);
   -moz-context-properties: fill, stroke;
 }
 
 .error-message-label {
   background-color: var(--error-background);
   border-radius: 2px;
   margin: 0;
+  margin-inline-end: 8px;
+  max-width: 300px;
   padding: 5px 10px;
+  word-wrap: break-word;
 }
 
 .error-message-arrow {
   background-color: var(--error-background);
   content: "";
   max-height: 8px;
   width: 8px;
   transform: translate(4px, -6px) rotate(45deg);
--- a/toolkit/mozapps/extensions/content/shortcuts.js
+++ b/toolkit/mozapps/extensions/content/shortcuts.js
@@ -15,16 +15,17 @@ XPCOMUtils.defineLazyModuleGetters(this,
 
 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.
 };
 
 let templatesLoaded = false;
+let shortcutKeyMap = new Map();
 const templates = {};
 
 function loadTemplates() {
   if (templatesLoaded) return;
   templatesLoaded = true;
 
   templates.card = document.getElementById("card-template");
   templates.row = document.getElementById("shortcut-row-template");
@@ -150,24 +151,24 @@ function getShortcutValue(shortcut) {
     return key;
   }
 
   return null;
 }
 
 let error;
 
-function setError(input, messageId) {
+function setError(input, messageId, args) {
   if (!error) error = document.querySelector(".error-message");
 
   let {x, y, height} = input.getBoundingClientRect();
   error.style.top = `${y + window.scrollY + height - 5}px`;
   error.style.left = `${x}px`;
   document.l10n.setAttributes(
-    error.querySelector(".error-message-label"), messageId);
+    error.querySelector(".error-message-label"), messageId, args);
   error.style.visibility = "visible";
 }
 
 function inputBlurred(e) {
   if (!error) error = document.querySelector(".error-message");
 
   error.style.visibility = "hidden";
   e.target.value = getShortcutValue(e.target.getAttribute("shortcut"));
@@ -197,16 +198,41 @@ function getShortcutForEvent(e) {
 
   return Object.entries(modifierMap)
     .filter(([key, isDown]) => isDown)
     .map(([key]) => key)
     .concat(getStringForEvent(e))
     .join("+");
 }
 
+function recordShortcut(shortcut, addonName) {
+  let addons = shortcutKeyMap.get(shortcut);
+  if (addons) {
+    addons.add(addonName);
+  } else {
+    shortcutKeyMap.set(shortcut, new Set([addonName]));
+  }
+}
+
+function removeShortcut(shortcut, addonName) {
+  let addons = shortcutKeyMap.get(shortcut);
+  if (addons) {
+    addons.delete(addonName);
+    if (addons.size === 0) {
+      shortcutKeyMap.delete(shortcut);
+    }
+  }
+}
+
+function getAddonName(shortcut) {
+  let addons = shortcutKeyMap.get(shortcut);
+  // Get the first addon name with given shortcut.
+  return addons.values().next().value;
+}
+
 function onShortcutChange(e) {
   let input = e.target;
 
   if (e.key == "Escape") {
     input.blur();
     return;
   }
 
@@ -236,20 +262,32 @@ function onShortcutChange(e) {
     case ShortcutUtils.IS_VALID:
       // Show an error if this is already a system shortcut.
       let chromeWindow = window.windowRoot.ownerGlobal;
       if (ShortcutUtils.isSystem(chromeWindow, shortcutString)) {
         setError(input, "shortcuts-system");
         break;
       }
 
-      // Update the shortcut if it isn't reserved.
       let addonId = input.closest(".card").getAttribute("addon-id");
       let extension = extensionForAddonId(addonId);
 
+      // Check if shortcut is already assigned.
+      if (shortcutKeyMap.has(shortcutString)) {
+        setError(input, "shortcuts-exists", {addon: getAddonName(shortcutString)});
+        break;
+      } else {
+        // Update the shortcut if it isn't reserved or assigned.
+        let oldShortcut = input.getAttribute("shortcut");
+        let addonName = input.closest(".card").getAttribute("addon-name");
+
+        removeShortcut(oldShortcut, addonName);
+        recordShortcut(shortcutString, addonName);
+      }
+
       // This is async, but we're not awaiting it to keep the handler sync.
       extension.shortcuts.updateCommand({
         name: input.getAttribute("name"),
         shortcut: shortcutString,
       });
       input.setAttribute("shortcut", shortcutString);
       input.blur();
       break;
@@ -290,16 +328,17 @@ async function renderAddons(addons) {
     // Skip this extension if it isn't a webextension.
     if (!extension) continue;
 
     if (extension.shortcuts) {
       let card = document.importNode(
         templates.card.content, true).firstElementChild;
       let icon = AddonManager.getPreferredIconURL(addon, 24, window);
       card.setAttribute("addon-id", addon.id);
+      card.setAttribute("addon-name", addon.name);
       card.querySelector(".addon-icon").src = icon || FALLBACK_ICON;
       card.querySelector(".addon-name").textContent = addon.name;
 
       let commands = await extension.shortcuts.allCommands();
 
       // Sort the commands so the ones with shortcuts are at the top.
       commands.sort((a, b) => {
         // Boolean compare the shortcuts to see if they're both set or unset.
@@ -339,16 +378,17 @@ async function renderAddons(addons) {
         input.addEventListener("blur", inputBlurred);
         input.addEventListener("focus", clearValue);
 
         if (willHideCommands && i == limit) {
           firstHiddenInput = input;
         }
 
         card.appendChild(row);
+        recordShortcut(command.shortcut, addon.name);
       }
 
       // Add an expand button, if needed.
       if (willHideCommands) {
         let row = document.importNode(templates.expandRow.content, true);
         let button = row.querySelector(".expand-button");
         let numberToShow = commands.length - limit;
         let setLabel = (type) => {
--- a/toolkit/mozapps/extensions/test/browser/browser_manage_shortcuts.js
+++ b/toolkit/mozapps/extensions/test/browser/browser_manage_shortcuts.js
@@ -25,24 +25,24 @@ async function loadShortcutsView() {
 function closeView() {
   return close_manager(gManagerWindow);
 }
 
 add_task(async function testUpdatingCommands() {
   let commands = {
     commandZero: {},
     commandOne: {
-      suggested_key: {default: "Shift+Alt+4"},
+      suggested_key: {default: "Shift+Alt+7"},
     },
     commandTwo: {
       description: "Command Two!",
       suggested_key: {default: "Alt+4"},
     },
     _execute_browser_action: {
-      suggested_key: {default: "Shift+Alt+5"},
+      suggested_key: {default: "Shift+Alt+9"},
     },
   };
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       commands,
       browser_action: {default_popup: "popup.html"},
     },
     background() {
@@ -77,52 +77,54 @@ add_task(async function testUpdatingComm
 
   async function checkShortcut(name, key, modifiers) {
     EventUtils.synthesizeKey(key, modifiers);
     let message = await extension.awaitMessage("oncommand");
     is(message, name, `Expected onCommand listener to fire with the correct name: ${name}`);
   }
 
   // Check that the original shortcuts work.
-  await checkShortcut("commandOne", "4", {shiftKey: true, altKey: true});
+  await checkShortcut("commandOne", "7", {shiftKey: true, altKey: true});
   await checkShortcut("commandTwo", "4", {altKey: true});
 
   let doc = await loadShortcutsView();
 
   let card = doc.querySelector(`.card[addon-id="${extension.id}"]`);
   ok(card, `There is a card for the extension`);
 
   let inputs = card.querySelectorAll(".shortcut-input");
   is(inputs.length, Object.keys(commands).length, "There is an input for each command");
 
   let nameOrder = Array.from(inputs).map(input => input.getAttribute("name"));
   Assert.deepEqual(
     nameOrder,
     ["commandOne", "commandTwo", "_execute_browser_action", "commandZero"],
     "commandZero should be last since it is unset");
 
+  let count = 1;
   for (let input of inputs) {
     // Change the shortcut.
     input.focus();
-    EventUtils.synthesizeKey("7", {shiftKey: true, altKey: true});
+    EventUtils.synthesizeKey("8", {shiftKey: true, altKey: true});
+    count++;
 
     // Wait for the shortcut attribute to change.
     await BrowserTestUtils.waitForCondition(
-      () => input.getAttribute("shortcut") == "Alt+Shift+7");
+      () => input.getAttribute("shortcut") == "Alt+Shift+8");
 
     // Check that the change worked (but skip if browserAction).
     if (input.getAttribute("name") != "_execute_browser_action") {
-      await checkShortcut(input.getAttribute("name"), "7", {shiftKey: true, altKey: true});
+      await checkShortcut(input.getAttribute("name"), "8", {shiftKey: true, altKey: true});
     }
 
     // Change it again so it doesn't conflict with the next command.
     input.focus();
-    EventUtils.synthesizeKey("9", {shiftKey: true, altKey: true});
+    EventUtils.synthesizeKey(count.toString(), {shiftKey: true, altKey: true});
     await BrowserTestUtils.waitForCondition(
-      () => input.getAttribute("shortcut") == "Alt+Shift+9");
+      () => input.getAttribute("shortcut") == `Alt+Shift+${count}`);
   }
 
   // Check that errors can be shown.
   let input = inputs[0];
   let error = doc.querySelector(".error-message");
   let label = error.querySelector(".error-message-label");
   is(error.style.visibility, "hidden", "The error is initially hidden");
 
@@ -134,16 +136,22 @@ add_task(async function testUpdatingComm
   is(error.style.visibility, "visible", "The error is shown");
 
   // Escape should clear the focus and hide the error.
   is(doc.activeElement, input, "The input is focused");
   EventUtils.synthesizeKey("Escape", {});
   ok(doc.activeElement != input, "The input is no longer focused");
   is(error.style.visibility, "hidden", "The error is hidden");
 
+  // Check if assigning already assigned shortcut is prevented.
+  input.focus();
+  EventUtils.synthesizeKey("2", {shiftKey: true, altKey: true});
+  is(label.dataset.l10nId, "shortcuts-exists", `The message is set`);
+  is(error.style.visibility, "visible", "The error is shown");
+
   // Check the label uses the description first, and has a default for the special cases.
   function checkLabel(name, value) {
     let input = doc.querySelector(`.shortcut-input[name="${name}"]`);
     let label = input.previousElementSibling;
     if (label.dataset.l10nId) {
       is(label.dataset.l10nId, value, "The l10n-id is set");
     } else {
       is(label.textContent, value, "The textContent is set");