Bug 1191862 - part 3: Make `GlobalKeyListener` not reserve key combination which is mapped to an edit command or a navigation command by native key bindings r=NeilDeakin,smaug

Users may map reserved shortcut keys of Firefox/Thunderbird as an editing command or a navigation command. Therefore if and only if an editable element has focus and a reserved key combination is mapped to an editing command or a navigation command by the system settings, we should allow to dispatch it into the content and work it as what user expects. With this change, keyboard only users may loose some shortcut keys to leave from a web content which blocks keyboard focus in it. However, there may be another reserved shortcut keys to escape from such web apps only with keyboard because it's hard to think that all reserved shortcut keys conflict with users' settings. Differential Revision: https://phabricator.services.mozilla.com/D138009
2022-02-15 08:00:06 +00:00 · 2022-02-15 08:00:06 +00:00 · 8afd3c5af5
--- a/browser/base/content/test/permissions/browser_reservedkey.js
+++ b/browser/base/content/test/permissions/browser_reservedkey.js
@ -6,8 +6,6 @@ add_task(async function test_reserved_shortcuts() {
  key1.setAttribute("key", "O");
  key1.setAttribute("reserved", "true");
  key1.setAttribute("count", "0");
-  // We need to have the attribute "oncommand" for the "command" listener to fire
-  key1.setAttribute("oncommand", "//");
  key1.addEventListener("command", () => {
    let attribute = key1.getAttribute("count");
    key1.setAttribute("count", Number(attribute) + 1);
@ -19,8 +17,6 @@ add_task(async function test_reserved_shortcuts() {
  key2.setAttribute("key", "P");
  key2.setAttribute("reserved", "false");
  key2.setAttribute("count", "0");
-  // We need to have the attribute "oncommand" for the "command" listener to fire
-  key2.setAttribute("oncommand", "//");
  key2.addEventListener("command", () => {
    let attribute = key2.getAttribute("count");
    key2.setAttribute("count", Number(attribute) + 1);
@ -31,8 +27,6 @@ add_task(async function test_reserved_shortcuts() {
  key3.setAttribute("modifiers", "shift");
  key3.setAttribute("key", "Q");
  key3.setAttribute("count", "0");
-  // We need to have the attribute "oncommand" for the "command" listener to fire
-  key3.setAttribute("oncommand", "//");
  key3.addEventListener("command", () => {
    let attribute = key3.getAttribute("count");
    key3.setAttribute("count", Number(attribute) + 1);
@ -226,3 +220,98 @@ add_task(async function test_backspace_delete() {

  BrowserTestUtils.removeTab(tab);
 });
+
+// TODO: Make this to run on Windows too to have automated tests also there.
+if (
+  navigator.platform.includes("Mac") ||
+  navigator.platform.includes("Linux")
+) {
+  add_task(
+    async function test_reserved_shortcuts_conflict_with_user_settings() {
+      await new Promise(resolve => {
+        SpecialPowers.pushPrefEnv(
+          { set: [["test.events.async.enabled", true]] },
+          resolve
+        );
+      });
+
+      const keyset = document.createXULElement("keyset");
+      const key = document.createXULElement("key");
+      key.setAttribute("id", "conflict_with_known_native_key_binding");
+      if (navigator.platform.includes("Mac")) {
+        // Select to end of the paragraph
+        key.setAttribute("modifiers", "ctrl,shift");
+        key.setAttribute("key", "E");
+      } else {
+        // Select All
+        key.setAttribute("modifiers", "ctrl");
+        key.setAttribute("key", "a");
+      }
+      key.setAttribute("reserved", "true");
+      key.setAttribute("count", "0");
+      key.addEventListener("command", () => {
+        const attribute = key.getAttribute("count");
+        key.setAttribute("count", Number(attribute) + 1);
+      });
+
+      keyset.appendChild(key);
+      const container = document.createXULElement("box");
+      container.appendChild(keyset);
+      document.documentElement.appendChild(container);
+
+      const pageUrl =
+        "data:text/html,<body onload='document.body.firstChild.focus(); getSelection().collapse(document.body.firstChild, 0)'><div contenteditable>Test</div></body>";
+      const tab = await BrowserTestUtils.openNewForegroundTab(
+        gBrowser,
+        pageUrl
+      );
+
+      await SpecialPowers.spawn(
+        tab.linkedBrowser,
+        [key.getAttribute("key")],
+        async function(aExpectedKeyValue) {
+          content.promiseTestResult = new Promise(resolve => {
+            content.addEventListener("keyup", event => {
+              if (event.key.toLowerCase() == aExpectedKeyValue.toLowerCase()) {
+                resolve(
+                  content
+                    .getSelection()
+                    .getRangeAt(0)
+                    .toString()
+                );
+              }
+            });
+          });
+        }
+      );
+
+      EventUtils.synthesizeKey(key.getAttribute("key"), {
+        ctrlKey: key.getAttribute("modifiers").includes("ctrl"),
+        shiftKey: key.getAttribute("modifiers").includes("shift"),
+      });
+
+      const selectedText = await SpecialPowers.spawn(
+        tab.linkedBrowser,
+        [],
+        async function() {
+          return content.promiseTestResult;
+        }
+      );
+      is(
+        selectedText,
+        "Test",
+        "The shortcut key should select all text in the editor"
+      );
+
+      is(
+        key.getAttribute("count"),
+        "0",
+        "The reserved shortcut key should be consumed by the focused editor instead"
+      );
+
+      document.documentElement.removeChild(container);
+
+      BrowserTestUtils.removeTab(tab);
+    }
+  );
+}
--- a/dom/events/GlobalKeyListener.cpp
+++ b/dom/events/GlobalKeyListener.cpp
@ -13,6 +13,7 @@
 #include "mozilla/EventStateManager.h"
 #include "mozilla/HTMLEditor.h"
 #include "mozilla/KeyEventHandler.h"
+#include "mozilla/NativeKeyBindingsType.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/ShortcutKeys.h"
 #include "mozilla/StaticPtr.h"
@ -21,6 +22,7 @@
 #include "mozilla/dom/Event.h"
 #include "mozilla/dom/EventBinding.h"
 #include "mozilla/dom/KeyboardEvent.h"
+#include "mozilla/widget/IMEData.h"
 #include "nsAtom.h"
 #include "nsCOMPtr.h"
 #include "nsContentUtils.h"
@ -29,6 +31,7 @@
 #include "nsIContent.h"
 #include "nsIContentInlines.h"
 #include "nsIDocShell.h"
+#include "nsIWidget.h"
 #include "nsNetUtil.h"
 #include "nsPIDOMWindow.h"

@ -396,11 +399,26 @@ bool GlobalKeyListener::IsReservedKey(WidgetKeyboardEvent* aKeyEvent,
    return false;
  }

-  if (reserved == ReservedKey_True) {
-    return true;
+  if (reserved != ReservedKey_True &&
+      !nsContentUtils::ShouldBlockReservedKeys(aKeyEvent)) {
+    return false;
  }

-  return nsContentUtils::ShouldBlockReservedKeys(aKeyEvent);
+  // Okay, the key handler is reserved, but if the key combination is mapped to
+  // an edit command or a selection navigation command, we should not treat it
+  // as reserved since user wants to do the mapped thing(s) in editor.
+  if (MOZ_UNLIKELY(!aKeyEvent->IsTrusted() || !aKeyEvent->mWidget)) {
+    return true;
+  }
+  widget::InputContext inputContext = aKeyEvent->mWidget->GetInputContext();
+  if (!inputContext.mIMEState.IsEditable()) {
+    return true;
+  }
+  return MOZ_UNLIKELY(!aKeyEvent->IsEditCommandsInitialized(
+             inputContext.GetNativeKeyBindingsType())) ||
+         aKeyEvent
+             ->EditCommandsConstRef(inputContext.GetNativeKeyBindingsType())
+             .IsEmpty();
 }

 bool GlobalKeyListener::HasHandlerForEvent(dom::KeyboardEvent* aEvent,
--- a/dom/events/IMEStateManager.cpp
+++ b/dom/events/IMEStateManager.cpp
@ -1350,6 +1350,8 @@ static bool IsNextFocusableElementTextControl(const Element* aInputContent) {

 static void GetInputType(const IMEState& aState, const nsIContent& aContent,
                         nsAString& aInputType) {
+  // NOTE: If you change here, you may need to update
+  //       widget::InputContext::GatNativeKeyBindings too.
  if (aContent.IsHTMLElement(nsGkAtoms::input)) {
    const HTMLInputElement* inputElement =
        HTMLInputElement::FromNode(&aContent);
--- a/dom/ipc/BrowserParent.cpp
+++ b/dom/ipc/BrowserParent.cpp
@ -2011,16 +2011,24 @@ void BrowserParent::SendRealKeyEvent(WidgetKeyboardEvent& aEvent) {
  //       you also need to update
  //       TextEventDispatcher::DispatchKeyboardEventInternal().
  if (aEvent.mMessage == eKeyPress) {
-    // XXX Should we do this only when input context indicates an editor having
-    //     focus and the key event won't cause inputting text?
-    Maybe<WritingMode> writingMode;
-    if (aEvent.mWidget) {
-      if (RefPtr<widget::TextEventDispatcher> dispatcher =
-              aEvent.mWidget->GetTextEventDispatcher()) {
-        writingMode = dispatcher->MaybeQueryWritingModeAtSelection();
+    // If current input context is editable, the edit commands are initialized
+    // by TextEventDispatcher::DispatchKeyboardEventInternal().  Otherwise,
+    // we need to do it here (they are not necessary for the parent process,
+    // therefore, we need to do it here for saving the runtime cost).
+    if (!aEvent.AreAllEditCommandsInitialized()) {
+      // XXX Is it good thing that the keypress event will be handled in an
+      //     editor even though the user pressed the key combination before the
+      //     focus change has not been completed in the parent process yet or
+      //     focus change will happen?  If no, we can stop doing this.
+      Maybe<WritingMode> writingMode;
+      if (aEvent.mWidget) {
+        if (RefPtr<widget::TextEventDispatcher> dispatcher =
+                aEvent.mWidget->GetTextEventDispatcher()) {
+          writingMode = dispatcher->MaybeQueryWritingModeAtSelection();
+        }
      }
+      aEvent.InitAllEditCommands(writingMode);
    }
-    aEvent.InitAllEditCommands(writingMode);
  } else {
    aEvent.PreventNativeKeyBindings();
  }
--- a/widget/IMEData.h
+++ b/widget/IMEData.h
@ -8,6 +8,7 @@

 #include "mozilla/CheckedInt.h"
 #include "mozilla/EventForwards.h"
+#include "mozilla/NativeKeyBindingsType.h"
 #include "mozilla/ToString.h"

 #include "nsPoint.h"
@ -420,6 +421,17 @@ struct InputContext final {
    return mHTMLInputType.LowerCaseEqualsLiteral("password");
  }

+  NativeKeyBindingsType GetNativeKeyBindingsType() const {
+    MOZ_DIAGNOSTIC_ASSERT(mIMEState.IsEditable());
+    // See GetInputType in IMEStateManager.cpp
+    if (mHTMLInputType.IsEmpty()) {
+      return NativeKeyBindingsType::RichTextEditor;
+    }
+    return mHTMLInputType.EqualsLiteral("textarea")
+               ? NativeKeyBindingsType::MultiLineEditor
+               : NativeKeyBindingsType::SingleLineEditor;
+  }
+
  // https://html.spec.whatwg.org/dev/interaction.html#autocapitalization
  bool IsAutocapitalizeSupported() const {
    return !mHTMLInputType.EqualsLiteral("password") &&
--- a/widget/TextEventDispatcher.cpp
+++ b/widget/TextEventDispatcher.cpp
@ -5,6 +5,7 @@

 #include "TextEventDispatcher.h"

+#include "IMEData.h"
 #include "PuppetWidget.h"
 #include "TextEvents.h"

@ -694,6 +695,14 @@ bool TextEventDispatcher::DispatchKeyboardEventInternal(
    keyEvent.mFlags.mOnlySystemGroupDispatchInContent = true;
  }

+  // If an editable element has focus and we're in the parent process, we should
+  // retrieve native key bindings right now because even if it matches with a
+  // reserved shortcut key, it should be handled by the editor.
+  if (XRE_IsParentProcess() && mHasFocus &&
+      (aMessage == eKeyDown || aMessage == eKeyPress)) {
+    keyEvent.InitAllEditCommands(mWritingMode);
+  }
+
  DispatchInputEvent(mWidget, keyEvent, aStatus);
  return true;
 }
--- a/widget/TextEventDispatcher.h
+++ b/widget/TextEventDispatcher.h
@ -542,11 +542,11 @@ class TextEventDispatcher final {
   *                        Then, WillDispatchKeyboardEvent() is always called.
   * @return                true if an event is dispatched.  Otherwise, false.
   */
-  bool DispatchKeyboardEventInternal(EventMessage aMessage,
-                                     const WidgetKeyboardEvent& aKeyboardEvent,
-                                     nsEventStatus& aStatus, void* aData,
-                                     uint32_t aIndexOfKeypress = 0,
-                                     bool aNeedsCallback = false);
+  // TODO: Mark this as MOZ_CAN_RUN_SCRIPT instead.
+  MOZ_CAN_RUN_SCRIPT_BOUNDARY bool DispatchKeyboardEventInternal(
+      EventMessage aMessage, const WidgetKeyboardEvent& aKeyboardEvent,
+      nsEventStatus& aStatus, void* aData, uint32_t aIndexOfKeypress = 0,
+      bool aNeedsCallback = false);

  /**
   * ClearNotificationRequests() clears mIMENotificationRequests.