Bug 371900, always fire a command event on key elements except for th…

…ose that are marked not to, r=masayuki The edit-related commands are special because they are handled by ShortcutKeyDefinitions.cpp yet we have duplicate keys because we want the menu disabled state to update properly, so we don't fire command events on those. Differential Revision: https://phabricator.services.mozilla.com/D135157
dothq · Jan 8, 2022 · 6b88580 · 6b88580
1 parent d25b03e
commit 6b88580
Show file tree

Hide file tree

Showing 6 changed files with 49 additions and 29 deletions.
diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc
@@ -170,7 +170,8 @@
     <key id="key_toggleMute" data-l10n-id="mute-toggle-shortcut" command="cmd_toggleMute" modifiers="control"/>
     <key id="key_undo"
          data-l10n-id="text-action-undo-shortcut"
-         modifiers="accel"/>
+         modifiers="accel"
+         internal="true"/>
     <key id="key_redo"
 #ifdef XP_UNIX
          data-l10n-id="text-action-undo-shortcut"
@@ -179,18 +180,21 @@
          data-l10n-id="text-action-redo-shortcut"
          modifiers="accel"
 #endif
-         />
+         internal="true"/>
     <key id="key_cut"
          data-l10n-id="text-action-cut-shortcut"
-         modifiers="accel"/>
+         modifiers="accel"
+         internal="true"/>
     <key id="key_copy"
          data-l10n-id="text-action-copy-shortcut"
-         modifiers="accel"/>
+         modifiers="accel"
+         internal="true"/>
     <key id="key_paste"
          data-l10n-id="text-action-paste-shortcut"
-         modifiers="accel"/>
+         modifiers="accel"
+         internal="true"/>
     <key id="key_delete" keycode="VK_DELETE" command="cmd_delete" reserved="false"/>
-    <key id="key_selectAll" data-l10n-id="text-action-select-all-shortcut" modifiers="accel"/>
+    <key id="key_selectAll" data-l10n-id="text-action-select-all-shortcut" modifiers="accel" internal="true"/>
 
     <key keycode="VK_BACK" command="cmd_handleBackspace" reserved="false"/>
     <key keycode="VK_BACK" command="cmd_handleShiftBackspace" modifiers="shift" reserved="false"/>
@@ -310,7 +314,9 @@
 # On OS X, dark voodoo magic invokes the quit code for this key.
 # So we're not adding the attribute on OSX because of backwards/add-on compat.
 # See bug 1369909 for background on this.
-#ifndef XP_MACOSX
+#ifdef XP_MACOSX
+         internal="true"
+#else
          command="cmd_quitApplication"
 #endif
          reserved="true"/>
@@ -361,20 +367,25 @@
     <key id="key_minimizeWindow"
          command="minimizeWindow"
          data-l10n-id="window-minimize-shortcut"
-         modifiers="accel"/>
+         modifiers="accel"
+         internal="true"/>
     <key id="key_openHelpMac"
          oncommand="openHelpLink('firefox-osxkey');"
          data-l10n-id="help-shortcut"
-         modifiers="accel"/>
+         modifiers="accel"
+         internal="true"/>
     <!-- These are used to build the Application menu -->
     <key id="key_preferencesCmdMac"
          data-l10n-id="preferences-shortcut"
-         modifiers="accel"/>
+         modifiers="accel"
+         internal="true"/>
     <key id="key_hideThisAppCmdMac"
          data-l10n-id="hide-app-shortcut"
-         modifiers="accel"/>
+         modifiers="accel"
+         internal="true"/>
     <key id="key_hideOtherAppsCmdMac"
          data-l10n-id="hide-other-apps-shortcut"
-         modifiers="accel,alt"/>
+         modifiers="accel,alt"
+         internal="true"/>
 #endif
   </keyset>
diff --git a/browser/components/downloads/content/contentAreaDownloadsView.xhtml b/browser/components/downloads/content/contentAreaDownloadsView.xhtml
@@ -15,10 +15,6 @@
 %editMenuDTD;
 ]>
 
-<!-- @CSP: We have to whitelist the 'oncommand' handler for all the cmd_* fields within
-   - editMenuOverlay.js until Bug 371900 is fixed using
-   -    sha512-4o5Uf4E4EG+90Mb820FH2YFDf4IuX4bfUwQC7reK1ZhgcXWJBKMK2330XIELaFJJ8HiPffS9mP60MPjuXMIrHA==
-   -->
 <window id="contentAreaDownloadsView"
         data-l10n-id="downloads-window"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

diff --git a/dom/events/GlobalKeyListener.cpp b/dom/events/GlobalKeyListener.cpp
@@ -584,8 +584,13 @@ bool XULKeySetGlobalKeyListener::IsExecutableElement(
     return false;
   }
 
-  aElement->GetAttr(nsGkAtoms::oncommand, value);
-  return !value.IsEmpty();
+  // Internal keys are defined as <key> elements so that the menu label
+  // and disabled state can be updated properly, but the command is executed
+  // by some other means. This will typically be because the key is defined
+  // as a shortcut defined in ShortcutKeyDefinitions.cpp instead, or on Mac,
+  // some special system defined keys.
+  return !aElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::internal,
+                                nsGkAtoms::_true, eCaseMatters);
 }
 
 already_AddRefed<dom::EventTarget> XULKeySetGlobalKeyListener::GetHandlerTarget(

diff --git a/toolkit/content/editMenuOverlay.js b/toolkit/content/editMenuOverlay.js
@@ -40,24 +40,21 @@ function goUpdatePasteMenuItems() {
 window.addEventListener(
   "DOMContentLoaded",
   () => {
-    // Bug 371900: Remove useless oncommand attribute once bug 371900 is fixed
-    // If you remove/update the oncommand attribute for any of the cmd_*, please
-    // also remove/update the sha512 hash in the CSP within about:downloads
     let container =
       document.querySelector("commandset") || document.documentElement;
     let fragment = MozXULElement.parseXULToFragment(`
       <commandset id="editMenuCommands">
         <commandset id="editMenuCommandSetAll" commandupdater="true" events="focus,select" />
         <commandset id="editMenuCommandSetUndo" commandupdater="true" events="undo" />
         <commandset id="editMenuCommandSetPaste" commandupdater="true" events="clipboard" />
-        <command id="cmd_undo" oncommand=";" />
-        <command id="cmd_redo" oncommand=";" />
-        <command id="cmd_cut" oncommand=";" />
-        <command id="cmd_copy" oncommand=";" />
-        <command id="cmd_paste" oncommand=";" />
-        <command id="cmd_delete" oncommand=";" />
-        <command id="cmd_selectAll" oncommand=";" />
-        <command id="cmd_switchTextDirection" oncommand=";" />
+        <command id="cmd_undo" internal="true"/>
+        <command id="cmd_redo" internal="true" />
+        <command id="cmd_cut" internal="true" />
+        <command id="cmd_copy" internal="true" />
+        <command id="cmd_paste" internal="true" />
+        <command id="cmd_delete" internal="true" />
+        <command id="cmd_selectAll" internal="true" />
+        <command id="cmd_switchTextDirection" />
       </commandset>
     `);
 

diff --git a/toolkit/content/tests/chrome/window_keys.xhtml b/toolkit/content/tests/chrome/window_keys.xhtml
@@ -55,12 +55,21 @@ var keysToTest = [
   ["k-g-co", "G", { ctrlKey: true, osKey: true } ],
   ["scommand", "Y", { } ],
   ["", "U", { } ],
+  ["k-z-c", "Z", { ctrlKey: true } ],
 ];
 
 function runTest()
 {
+  let nonInlineKeyFired = false;
+  document.getElementById("k-z-c").addEventListener("command", event => {
+    nonInlineKeyFired = true;
+    checkKey(event);
+  });
+
   iterateKeys(true, "normal");
 
+  ok(nonInlineKeyFired, "non-inline command listener fired");
+
   var keyset = document.getElementById("keyset");
   keyset.setAttribute("disabled", "true");
   iterateKeys(false, "disabled");
@@ -174,6 +183,7 @@ SimpleTest.waitForFocus(runTest);
   <key id="k-g-co" key="g" modifiers="control os" oncommand="checkKey(event)"/>
   <key id="k-y" key="y" command="scommand"/>
   <key id="k-u" key="u" command="scommand-disabled"/>
+  <key id="k-z-c" key="z" modifiers="control"/>
 </keyset>
 
 <keyset id="keyset2">

diff --git a/xpcom/ds/StaticAtoms.py b/xpcom/ds/StaticAtoms.py
@@ -554,6 +554,7 @@
     Atom("insertion", "insertion"),
     Atom("integer", "integer"),
     Atom("integrity", "integrity"),
+    Atom("internal", "internal"),
     Atom("internals", "internals"),
     Atom("intersection", "intersection"),
     Atom("intersectionobserverlist", "intersectionobserverlist"),