From 6b88580db1035d903cf372d28d7b243961f57639 Mon Sep 17 00:00:00 2001
From: Neil Deakin <neil@mozilla.com>
Date: Sat, 8 Jan 2022 18:07:21 +0000
Subject: [PATCH] Bug 371900, always fire a command event on key elements
 except for those 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
---
 browser/base/content/browser-sets.inc         | 35 ++++++++++++-------
 .../content/contentAreaDownloadsView.xhtml    |  4 ---
 dom/events/GlobalKeyListener.cpp              |  9 +++--
 toolkit/content/editMenuOverlay.js            | 19 +++++-----
 .../content/tests/chrome/window_keys.xhtml    | 10 ++++++
 xpcom/ds/StaticAtoms.py                       |  1 +
 6 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc
index 8bd7ab2ec69ae..91fac883c66ad 100644
--- 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
index f6e88122a717d..bf92e570ee076 100644
--- 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
index 44ec7e794e01a..38e6d2e6c8adf 100644
--- 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
index 8edcc31b1f8a6..57bd52caadf49 100644
--- a/toolkit/content/editMenuOverlay.js
+++ b/toolkit/content/editMenuOverlay.js
@@ -40,9 +40,6 @@ 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(`
@@ -50,14 +47,14 @@ window.addEventListener(
         <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
index e72c790947e6c..d85f8e8c3ea86 100644
--- 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
index 3b3587e5cbdd6..50f0b89309325 100644
--- 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"),