Bug 1520123: Notify users when there is a duplicate shortcut r=mstriemer,fluent-reviewers,flod,Gijs
☠☠ backed out by c1683d4d191a ☠ ☠
authorTrishul <trishul.goel@gmail.com>
Wed, 14 Aug 2019 22:33:06 +0000
changeset 488192 5974ae0842115acc3819232a089825997da3c53f
parent 488191 b5d442a2f457cc5f9879224879be1775769944e8
child 488193 eb56d88d1b0758babf0ba4ac55fe261481833f09
push id36437
push userncsoregi@mozilla.com
push dateThu, 15 Aug 2019 19:33:18 +0000
treeherdermozilla-central@44aac6fc3352 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstriemer, fluent-reviewers, flod, Gijs
bugs1520123
milestone70.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 1520123: Notify users when there is a duplicate shortcut r=mstriemer,fluent-reviewers,flod,Gijs Notify users when there is a duplicate shortcut Differential Revision: https://phabricator.services.mozilla.com/D29125
toolkit/locales/en-US/toolkit/about/aboutAddons.ftl
toolkit/mozapps/extensions/content/shortcuts.css
toolkit/mozapps/extensions/content/shortcuts.html
toolkit/mozapps/extensions/content/shortcuts.js
toolkit/mozapps/extensions/test/browser/browser.ini
toolkit/mozapps/extensions/test/browser/browser_shortcuts_duplicate_check.js
toolkit/themes/shared/in-content/common.inc.css
--- a/toolkit/locales/en-US/toolkit/about/aboutAddons.ftl
+++ b/toolkit/locales/en-US/toolkit/about/aboutAddons.ftl
@@ -323,16 +323,25 @@ 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 in warning label when there is a duplicate shortcut
+shortcuts-duplicate = Duplicate shortcut
+
+# String displayed when a keyboard shortcut is already assigned to more than one add-on
+# Variables:
+#   $shortcut (string) - Shortcut string for the add-on
+shortcuts-duplicate-warning-message = { $shortcut } is being used as a shortcut in more than one case. Duplicate shortcuts may cause unexpected behavior.
+
 # 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
--- a/toolkit/mozapps/extensions/content/shortcuts.css
+++ b/toolkit/mozapps/extensions/content/shortcuts.css
@@ -1,15 +1,14 @@
 .body {
   margin-inline-start: 28px;
+  width: 664px;
 }
 
 .shortcut.card {
-  /* Preferences content is 664px and the cards have 16px of left/right padding. */
-  width: 632px;
   margin-bottom: 16px;
 }
 
 .shortcut.card:first-of-type {
   margin-top: 8px;
 }
 
 .shortcut.card:hover {
@@ -48,53 +47,77 @@
   display: flex;
   justify-content: center;
 }
 
 .expand-button {
   margin: 8px 0 0;
 }
 
+.expand-button[warning]:not(:focus) {
+  outline: 2px solid var(--yellow-60);
+  outline-offset: -1px;
+  -moz-outline-radius: 3px;
+  box-shadow: 0 0 0 4px var(--yellow-60-a30);
+}
+
 .shortcut-input {
   font-size: 12px;
   padding: 4px 8px;
 }
 
 .extension-heading {
   display: flex;
 }
 
 .error-message {
   --error-background: var(--red-60);
+  --warning-background: var(--yellow-50);
+  --warning-text-color: var(--yellow-90);
   color: white;
   display: flex;
   flex-direction: column;
   position: absolute;
   visibility: hidden;
 }
 
 .error-message-icon {
   margin-left: 10px;
   width: 14px;
   height: 8px;
   fill: var(--error-background);
   stroke: var(--error-background);
   -moz-context-properties: fill, stroke;
 }
 
+.error-message[type="warning"] > .error-message-icon {
+  fill: var(--warning-background);
+  stroke: var(--warning-background);
+}
+
 .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[type="warning"] > .error-message-label {
+  background-color: var(--warning-background);
+  color: var(--warning-text-color);
+}
+
 .error-message-arrow {
   background-color: var(--error-background);
   content: "";
   max-height: 8px;
   width: 8px;
   transform: translate(4px, -6px) rotate(45deg);
   position: absolute;
 }
+
+/* The margin between message bars. */
+message-bar-stack > * {
+  margin-bottom: 8px;
+}
--- a/toolkit/mozapps/extensions/content/shortcuts.html
+++ b/toolkit/mozapps/extensions/content/shortcuts.html
@@ -6,20 +6,24 @@
 <html>
   <head>
     <link rel="stylesheet" href="chrome://global/skin/in-content/common.css" type="text/css"/>
     <link rel="stylesheet" href="chrome://mozapps/content/extensions/shortcuts.css"  type="text/css"/>
 
     <link rel="localization" href="branding/brand.ftl"/>
     <link rel="localization" href="toolkit/about/aboutAddons.ftl"/>
 
+    <script src="chrome://mozapps/content/extensions/message-bar.js"></script>
     <script src="chrome://mozapps/content/extensions/shortcuts.js"></script>
   </head>
   <body id="body">
     <div class="body">
+      <message-bar-stack id="duplicate-warning-messages" reverse max-message-bar-count="5">
+      </message-bar-stack>
+
       <div class="error-message">
         <img class="error-message-icon" src="chrome://global/skin/arrow/panelarrow-vertical.svg"/>
         <div class="error-message-label"></div>
       </div>
 
       <div id="addon-shortcuts"></div>
 
       <template id="card-template">
--- a/toolkit/mozapps/extensions/content/shortcuts.js
+++ b/toolkit/mozapps/extensions/content/shortcuts.js
@@ -15,16 +15,17 @@ XPCOMUtils.defineLazyModuleGetters(this,
   ShortcutUtils: "resource://gre/modules/ShortcutUtils.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();
 const templates = {};
 
 function loadTemplates() {
   if (templatesLoaded) {
     return;
@@ -214,24 +215,33 @@ function getShortcutValue(shortcut) {
     return key;
   }
 
   return null;
 }
 
 let error;
 
-function setError(input, messageId, args) {
+function setError(...args) {
+  setInputMessage("error", ...args);
+}
+
+function setWarning(...args) {
+  setInputMessage("warning", ...args);
+}
+
+function setInputMessage(type, 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`;
+  error.setAttribute("type", type);
   document.l10n.setAttributes(
     error.querySelector(".error-message-label"),
     messageId,
     args
   );
   error.style.visibility = "visible";
 }
 
@@ -239,18 +249,23 @@ function inputBlurred(e) {
   if (!error) {
     error = document.querySelector(".error-message");
   }
 
   error.style.visibility = "hidden";
   e.target.value = getShortcutValue(e.target.getAttribute("shortcut"));
 }
 
-function clearValue(e) {
+function onFocus(e) {
   e.target.value = "";
+
+  let warning = e.target.getAttribute("warning");
+  if (warning) {
+    setWarning(e.target, warning);
+  }
 }
 
 function getShortcutForEvent(e) {
   let modifierMap;
 
   if (AppConstants.platform == "macosx") {
     modifierMap = {
       MacCtrl: e.ctrlKey,
@@ -268,39 +283,114 @@ function getShortcutForEvent(e) {
 
   return Object.entries(modifierMap)
     .filter(([key, isDown]) => isDown)
     .map(([key]) => key)
     .concat(getStringForEvent(e))
     .join("+");
 }
 
-function recordShortcut(shortcut, addonName) {
+function buildDuplicateShortcutsMap(addons) {
+  return Promise.all(
+    addons.map(async addon => {
+      let extension = extensionForAddonId(addon.id);
+      if (extension.shortcuts) {
+        let commands = await extension.shortcuts.allCommands();
+        for (let command of commands) {
+          recordShortcut(command.shortcut, addon.name, command.name);
+        }
+      }
+    })
+  );
+}
+
+function recordShortcut(shortcut, addonName, commandName) {
+  if (!shortcut) {
+    return;
+  }
   let addons = shortcutKeyMap.get(shortcut);
+  let addonString = `${addonName}${SHORTCUT_KEY_SEPARATOR}${commandName}`;
   if (addons) {
-    addons.add(addonName);
+    addons.add(addonString);
   } else {
-    shortcutKeyMap.set(shortcut, new Set([addonName]));
+    shortcutKeyMap.set(shortcut, new Set([addonString]));
   }
 }
 
-function removeShortcut(shortcut, addonName) {
+function removeShortcut(shortcut, addonName, commandName) {
   let addons = shortcutKeyMap.get(shortcut);
+  let addonString = `${addonName}${SHORTCUT_KEY_SEPARATOR}${commandName}`;
   if (addons) {
-    addons.delete(addonName);
+    addons.delete(addonString);
     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;
+  let name = addons.values().next().value;
+  return name.split(SHORTCUT_KEY_SEPARATOR)[0];
+}
+
+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));
+      markDuplicates(shortcut);
+    }
+  }
+}
+
+function clearWarnings(warningHolder) {
+  warningHolder.textContent = "";
+  let inputs = document.querySelectorAll(".shortcut-input[warning]");
+  for (let input of inputs) {
+    input.removeAttribute("warning");
+    let row = input.closest(".shortcut-row");
+    if (row.hasAttribute("hide-before-expand")) {
+      row
+        .closest(".card")
+        .querySelector(".expand-button")
+        .removeAttribute("warning");
+    }
+  }
+}
+
+function createDuplicateWarningBar(shortcut) {
+  let messagebar = document.createElement("message-bar");
+  messagebar.setAttribute("type", "warning");
+
+  let message = document.createElement("span");
+  document.l10n.setAttributes(message, "shortcuts-duplicate-warning-message", {
+    shortcut,
+  });
+
+  messagebar.append(message);
+  return messagebar;
+}
+
+function markDuplicates(shortcut) {
+  let inputs = document.querySelectorAll(
+    `.shortcut-input[shortcut="${shortcut}"]`
+  );
+  for (let input of inputs) {
+    input.setAttribute("warning", "shortcuts-duplicate");
+    let row = input.closest(".shortcut-row");
+    if (row.hasAttribute("hide-before-expand")) {
+      row
+        .closest(".card")
+        .querySelector(".expand-button")
+        .setAttribute("warning", "shortcuts-duplicate");
+    }
+  }
 }
 
 function onShortcutChange(e) {
   let input = e.target;
 
   if (e.key == "Escape") {
     input.blur();
     return;
@@ -345,28 +435,30 @@ function onShortcutChange(e) {
         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");
+        let commandName = input.getAttribute("name");
 
-        removeShortcut(oldShortcut, addonName);
-        recordShortcut(shortcutString, addonName);
+        removeShortcut(oldShortcut, addonName, commandName);
+        recordShortcut(shortcutString, addonName, commandName);
       }
 
       // 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();
+      setDuplicateWarnings();
       break;
     case ShortcutUtils.MODIFIER_REQUIRED:
       if (AppConstants.platform == "macosx") {
         setError(input, "shortcuts-modifier-mac");
       } else {
         setError(input, "shortcuts-modifier-other");
       }
       break;
@@ -390,16 +482,27 @@ function renderNoShortcutAddons(addons) 
   }
 
   return fragment;
 }
 
 async function renderAddons(addons) {
   let frag = document.createDocumentFragment();
   let noShortcutAddons = [];
+
+  await buildDuplicateShortcutsMap(addons);
+
+  let isDuplicate = command => {
+    if (command.shortcut) {
+      let dupes = shortcutKeyMap.get(command.shortcut);
+      return dupes.size > 1;
+    }
+    return false;
+  };
+
   for (let addon of addons) {
     let extension = extensionForAddonId(addon.id);
 
     // Skip this extension if it isn't a webextension.
     if (!extension) {
       continue;
     }
 
@@ -411,16 +514,25 @@ async function renderAddons(addons) {
       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) => {
+        if (isDuplicate(a) && isDuplicate(b)) {
+          return 0;
+        }
+        if (isDuplicate(a)) {
+          return -1;
+        }
+        if (isDuplicate(b)) {
+          return 1;
+        }
         // Boolean compare the shortcuts to see if they're both set or unset.
         if (!a.shortcut == !b.shortcut) {
           return 0;
         }
         if (a.shortcut) {
           return -1;
         }
         return 1;
@@ -449,24 +561,23 @@ async function renderAddons(addons) {
         }
         let input = row.querySelector(".shortcut-input");
         input.value = getShortcutValue(command.shortcut);
         input.setAttribute("name", command.name);
         input.setAttribute("shortcut", command.shortcut);
         input.addEventListener("keydown", onShortcutChange);
         input.addEventListener("keyup", onShortcutChange);
         input.addEventListener("blur", inputBlurred);
-        input.addEventListener("focus", clearValue);
+        input.addEventListener("focus", onFocus);
 
         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 => {
@@ -518,9 +629,10 @@ async function render() {
     frag = await renderAddons(addons);
   } else {
     frag = document.importNode(templates.noAddons.content, true);
   }
 
   let container = document.getElementById("addon-shortcuts");
   container.textContent = "";
   container.appendChild(frag);
+  setDuplicateWarnings();
 }
--- a/toolkit/mozapps/extensions/test/browser/browser.ini
+++ b/toolkit/mozapps/extensions/test/browser/browser.ini
@@ -86,16 +86,17 @@ skip-if = (os == 'win' && processor == '
 [browser_installssl.js]
 skip-if = verify
 [browser_interaction_telemetry.js]
 [browser_manage_shortcuts.js]
 [browser_manage_shortcuts_hidden.js]
 [browser_pluginprefs.js]
 [browser_recentupdates.js]
 [browser_reinstall.js]
+[browser_shortcuts_duplicate_check.js]
 [browser_task_next_test.js]
 [browser_updateid.js]
 [browser_updatessl.js]
 [browser_webapi.js]
 [browser_webapi_access.js]
 skip-if = fission
 [browser_webapi_addon_listener.js]
 fail-if = fission
new file mode 100644
--- /dev/null
+++ b/toolkit/mozapps/extensions/test/browser/browser_shortcuts_duplicate_check.js
@@ -0,0 +1,156 @@
+"use strict";
+
+const { PromiseTestUtils } = ChromeUtils.import(
+  "resource://testing-common/PromiseTestUtils.jsm"
+);
+PromiseTestUtils.whitelistRejectionsGlobally(/Message manager disconnected/);
+
+let gManagerWindow;
+
+async function loadShortcutsView() {
+  gManagerWindow = await open_manager(null);
+  let categoryUtilities = new CategoryUtilities(gManagerWindow);
+  await categoryUtilities.openType("extension");
+
+  // There should be a manage shortcuts link.
+  let doc = gManagerWindow.document;
+  let shortcutsLink = doc.getElementById("manage-shortcuts");
+
+  // Open the shortcuts view.
+  shortcutsLink.click();
+  await wait_for_view_load(gManagerWindow);
+
+  return doc.getElementById("shortcuts-view").contentDocument;
+}
+
+function closeView() {
+  return close_manager(gManagerWindow);
+}
+
+add_task(async function testDuplicateShortcutsWarnings() {
+  let duplicateCommands = {
+    commandOne: {
+      suggested_key: { default: "Shift+Alt+1" },
+    },
+    commandTwo: {
+      description: "Command Two!",
+      suggested_key: { default: "Shift+Alt+2" },
+    },
+  };
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      commands: duplicateCommands,
+      name: "Extension 1",
+    },
+    background() {
+      browser.test.sendMessage("ready");
+    },
+    useAddonManager: "temporary",
+  });
+
+  await extension.startup();
+  await extension.awaitMessage("ready");
+
+  let extension2 = ExtensionTestUtils.loadExtension({
+    manifest: {
+      commands: {
+        ...duplicateCommands,
+        commandThree: {
+          description: "Command Three!",
+          suggested_key: { default: "Shift+Alt+3" },
+        },
+      },
+      name: "Extension 2",
+    },
+    background() {
+      browser.test.sendMessage("ready");
+    },
+    useAddonManager: "temporary",
+  });
+
+  await extension2.startup();
+  await extension2.awaitMessage("ready");
+
+  let doc = await loadShortcutsView();
+
+  let warningBars = doc.querySelectorAll("message-bar");
+  // Ensure warning messages are shown for each duplicate shorctut.
+  is(
+    warningBars.length,
+    Object.keys(duplicateCommands).length,
+    "There is a warning message bar for each duplicate shortcut"
+  );
+
+  // Ensure warning messages are correct with correct shortcuts.
+  let count = 1;
+  for (let warning of warningBars) {
+    let warningMsg = warning.querySelector("span");
+    let l10nAttrs = doc.l10n.getAttributes(warningMsg);
+    is(
+      l10nAttrs.id,
+      "shortcuts-duplicate-warning-message",
+      "Warning message is shown"
+    );
+    Assert.deepEqual(
+      l10nAttrs.args,
+      { shortcut: `Shift+Alt+${count}` },
+      "Warning message shortcut is correct"
+    );
+    count++;
+  }
+
+  ["Shift+Alt+1", "Shift+Alt+2"].forEach((shortcut, index) => {
+    // Ensure warning messages are correct with correct shortcuts.
+    let warning = warningBars[index];
+    let warningMsg = warning.querySelector("span");
+    let l10nAttrs = doc.l10n.getAttributes(warningMsg);
+    is(
+      l10nAttrs.id,
+      "shortcuts-duplicate-warning-message",
+      "Warning message is shown"
+    );
+    Assert.deepEqual(
+      l10nAttrs.args,
+      { shortcut },
+      "Warning message shortcut is correct"
+    );
+
+    // Check if all inputs have warning style.
+    let inputs = doc.querySelectorAll(`input[shortcut="${shortcut}"]`);
+    for (let input of inputs) {
+      // Check if warning error message is shown on focus.
+      input.focus();
+      let error = doc.querySelector(".error-message");
+      let label = error.querySelector(".error-message-label");
+      is(error.style.visibility, "visible", "The error element is shown");
+      is(
+        error.getAttribute("type"),
+        "warning",
+        "Duplicate shortcut has warning class"
+      );
+      is(
+        label.dataset.l10nId,
+        "shortcuts-duplicate",
+        "Correct error message is shown"
+      );
+
+      // On keypress events wrning class should be removed.
+      EventUtils.synthesizeKey("A");
+      ok(
+        !error.classList.contains("warning"),
+        "Error element should not have warning class"
+      );
+
+      input.blur();
+      is(
+        error.style.visibility,
+        "hidden",
+        "The error element is hidden on blur"
+      );
+    }
+  });
+
+  await closeView();
+  await extension.unload();
+  await extension2.unload();
+});
--- a/toolkit/themes/shared/in-content/common.inc.css
+++ b/toolkit/themes/shared/in-content/common.inc.css
@@ -84,16 +84,17 @@
   --red-50-a30: rgba(255, 0, 57, 0.3);
   --red-60: #d70022;
   --red-70: #a4000f;
   --red-80: #5a0002;
   --red-90: #3e0200;
   --yellow-10: #ffff98;
   --yellow-50: #ffe900;
   --yellow-60: #d7b600;
+  --yellow-60-a30: rgba(215, 182, 0, 0.3);
   --yellow-70: #a47f00;
   --yellow-80: #715100;
   --yellow-90: #3e2800;
 
   --shadow-10: 0 1px 4px var(--grey-90-a10);
   --shadow-30: 0 4px 16px var(--grey-90-a10);
 
   --card-padding: 16px;
@@ -955,16 +956,21 @@ xul|treechildren::-moz-tree-image(select
   background-color: var(--yellow-50);
   color: var(--yellow-90);
 }
 
 .message-bar-warning > .message-bar-icon {
   list-style-image: url("chrome://browser/skin/warning.svg");
 }
 
+input[type="text"][warning]:enabled:not(:focus) {
+  border-color: var(--yellow-60);
+  box-shadow: 0 0 0 1px var(--yellow-60), 0 0 0 4px var(--yellow-60-a30);
+}
+
 .card {
   background: var(--in-content-box-background);
   /* Needed for high-contrast where the border will appear. */
   border: 1px solid transparent;
   border-radius: 4px;
   box-shadow: var(--card-shadow);
   margin: 0 0 8px;
   /* Remove the border from the overall padding. */