Bug 1435717 - Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too r=smaug

Currently, if an event is consumed in the main process, EventStateManager
does not send it to remote process.  However, this is unexpected behavior
for some WidgetKeyboardEvent dispatchers.  OS sometimes has consumed native
key events before sending applications.  For example, Alt key on Windows
should activate menu bar of focused window but Alt key may be consumed before
focused window receives the event.  In such case, we mark Alt keyboard event
as "consumed before dispatch", and chrome treat it like as its preventDefault()
is called in web content.  (Note that for compatibility with other browsers,
the consumed state is not exposed to web content.  So, Event.defaultPrevented
returns false in web content.)

Therefore, we need to treat "consumed" state and "cross process forwarding"
state separately.  This patch makes calling WidgetEvent::PreventDefault()
always stops cross process forwarding for backward compatibility.  Additionally,
for the special case mentioned above, this patch makes
WidgetEvent::PreventDefaultBeforeDispatch() take additional argument,
|aIfStopCrossProcessForwarding|.  If this is CrossProcessForwarding::eStop,
the event won't be sent to remote process as same as calling PreventDefault().
Otherwise, CrossProcessForwarding::eHold, PreventDefaultBeforeDispatch() call
does not change "cross process forwarding" state.  I.e., if the event's
StopCrossProcessForwarding() and PreventDefault() are not called until
EventStateManager::PostHandleEvent(), the event will be sent to remote process
as usual.

MozReview-Commit-ID: IQGWJvXetxV

--HG--
extra : rebase_source : 4ccdd500e80b8fe29e469ac3b85578e1c07c8358

dom/events/EventStateManager.cpp

@@ -1385,8 +1385,7 @@ EventStateManager::IsRemoteTarget(nsIContent* target)
 bool
 EventStateManager::HandleCrossProcessEvent(WidgetEvent* aEvent,
                                            nsEventStatus *aStatus) {
-  if (*aStatus == nsEventStatus_eConsumeNoDefault ||
-      !aEvent->CanBeSentToRemoteProcess()) {
+  if (!aEvent->CanBeSentToRemoteProcess()) {
     return false;
   }
 

dom/ipc/TabChild.cpp

@@ -2125,6 +2125,14 @@ TabChild::RecvRealKeyEvent(const WidgetKeyboardEvent& aEvent)
         status == nsEventStatus_eConsumeNoDefault) {
       localEvent.PreventDefault();
     }
+    // This is an ugly hack, mNoRemoteProcessDispatch is set to true when the
+    // event's PreventDefault() or StopScrollProcessForwarding() is called.
+    // And then, it'll be checked by ParamTraits<mozilla::WidgetEvent>::Write()
+    // whether the event is being sent to remote process unexpectedly.
+    // However, unfortunately, it cannot check the destination.  Therefore,
+    // we need to clear the flag explicitly here because ParamTraits should
+    // keep checking the flag for avoiding regression.
+    localEvent.mFlags.mNoRemoteProcessDispatch = false;
     SendReplyKeyEvent(localEvent);
   }
 

layout/base/PresShell.cpp

@@ -7485,7 +7485,7 @@ PresShell::HandleEventInternal(WidgetEvent* aEvent,
             //     for some reasons (not sure) but we need to detect
             //     if a chrome event handler will call PreventDefault()
             //     again and check it later.
-            aEvent->PreventDefaultBeforeDispatch();
+            aEvent->PreventDefaultBeforeDispatch(CrossProcessForwarding::eStop);
             aEvent->mFlags.mOnlyChromeDispatch = true;
 
             // The event listeners in chrome can prevent this ESC behavior by
@@ -7504,7 +7504,7 @@ PresShell::HandleEventInternal(WidgetEvent* aEvent,
             // XXX See above comment to understand the reason why this needs
             //     to claim that the Escape key event is consumed by content
             //     even though it will be dispatched only into chrome.
-            aEvent->PreventDefaultBeforeDispatch();
+            aEvent->PreventDefaultBeforeDispatch(CrossProcessForwarding::eStop);
             aEvent->mFlags.mOnlyChromeDispatch = true;
             if (aEvent->mMessage == eKeyUp) {
               nsIDocument::UnlockPointer();

widget/BasicEvents.h

@@ -31,6 +31,17 @@ namespace mozilla {
 
 class EventTargetChainItem;
 
+enum class CrossProcessForwarding
+{
+  // eStop prevents the event to be sent to remote process.
+  eStop,
+  // eAllow keeps current state of the event whether it's sent to remote
+  // process.  In other words, eAllow does NOT mean that making the event
+  // sent to remote process when IsCrossProcessForwardingStopped() returns
+  // true.
+  eAllow,
+};
+
 /******************************************************************************
  * mozilla::BaseEventFlags
  *
@@ -190,18 +201,23 @@ public:
     // mDefaultPreventedByContent to true because in such case, defaultPrevented
     // must be true when web apps check it after they call preventDefault().
     if (aCalledByDefaultHandler) {
+      StopCrossProcessForwarding();
       mDefaultPreventedByChrome = true;
     } else {
       mDefaultPreventedByContent = true;
     }
   }
   // This should be used only before dispatching events into the DOM tree.
-  inline void PreventDefaultBeforeDispatch()
+  inline void
+  PreventDefaultBeforeDispatch(CrossProcessForwarding aCrossProcessForwarding)
   {
     if (!mCancelable) {
       return;
     }
     mDefaultPrevented = true;
+    if (aCrossProcessForwarding == CrossProcessForwarding::eStop) {
+      StopCrossProcessForwarding();
+    }
   }
   inline bool DefaultPrevented() const
   {
@@ -651,7 +667,11 @@ public:
   void PreventDefault(bool aCalledByDefaultHandler = true,
                       nsIPrincipal* aPrincipal = nullptr);
 
-  void PreventDefaultBeforeDispatch() { mFlags.PreventDefaultBeforeDispatch(); }
+  void
+  PreventDefaultBeforeDispatch(CrossProcessForwarding aCrossProcessForwarding)
+  {
+    mFlags.PreventDefaultBeforeDispatch(aCrossProcessForwarding);
+  }
   bool DefaultPrevented() const { return mFlags.DefaultPrevented(); }
   bool DefaultPreventedByContent() const
   {

widget/TextEventDispatcher.cpp

@@ -603,9 +603,11 @@ TextEventDispatcher::DispatchKeyboardEventInternal(
 
   if (aStatus == nsEventStatus_eConsumeNoDefault) {
     // If the key event should be dispatched as consumed event, marking it here.
-    // This is useful to prevent double action.  E.g., when the key was already
-    // handled by system, our chrome shouldn't handle it.
-    keyEvent.PreventDefaultBeforeDispatch();
+    // This is useful to prevent double action.  This is intended to the system
+    // has already consumed the event but we need to dispatch the event for
+    // compatibility with older version and other browsers.  So, we should not
+    // stop cross process forwarding of them.
+    keyEvent.PreventDefaultBeforeDispatch(CrossProcessForwarding::eAllow);
   }
 
   // Corrects each member for the specific key event type.

widget/tests/mochitest.ini

@@ -1,6 +1,8 @@
 [DEFAULT]
 support-files = utils.js
 
+[test_AltGr_key_events_in_web_content_on_windows.html]
+skip-if = toolkit != 'windows' || headless # headless: Bug 1410525
 [test_assign_event_data.html]
 subsuite = clipboard
 skip-if = toolkit == "cocoa" || (toolkit == 'android' && debug) # Mac: Bug 933303, Android bug 1285414

widget/tests/test_AltGr_key_events_in_web_content_on_windows.html

@@ -0,0 +1,106 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <title>Testing if AltGr keydown and keyup events are fired in web content on Windows</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/NativeKeyCodes.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css">
+</head>
+<body>
+<div id="display">
+  <input id="input">
+</div>
+<div id="content" style="display: none">
+</div>
+<pre id="test">
+</pre>
+
+<script class="testbody" type="application/javascript">
+SimpleTest.waitForExplicitFinish();
+
+function checkEvent(aEvent, aExpectedEvents, aDescription) {
+  if (!aExpectedEvents.length) {
+    ok(false, `${aDescription}: no more expected events ` +
+              `(type: ${aEvent.type}, code: ${aEvent.code}, key: ${aEvent.key}, keyCode: ${aEvent.keyCode}`);
+  }
+  let expectedEvent = aExpectedEvents.shift();
+  for (let property in expectedEvent) {
+    is(aEvent[property], expectedEvent[property], `${aDescription}: ${property}`);
+  }
+}
+
+async function runAltGrKeyTest() {
+  return new Promise(resolve => {
+    let target = document.getElementById("input");
+    target.focus();
+
+    let events = [
+      { type: "keydown", code: "ControlLeft", key: "Control", keyCode: KeyboardEvent.DOM_VK_CONTROL },
+      { type: "keydown", code: "AltRight", key: "AltGraph", keyCode: KeyboardEvent.DOM_VK_ALT },
+      { type: "keyup", code: "ControlLeft", key: "Control", keyCode: KeyboardEvent.DOM_VK_CONTROL },
+      { type: "keyup", code: "AltRight", key: "AltGraph", keyCode: KeyboardEvent.DOM_VK_ALT },
+    ];
+    function handleEvent(aEvent) {
+      checkEvent(aEvent, events, "runAltGrKeyTest");
+      if (aEvent.type === "keyup" && aEvent.code === "AltRight") {
+        is(events.length, 0, "runAltGrKeyTest: all expected events are fired");
+        SimpleTest.executeSoon(() => {
+          target.removeEventListener("keydown", handleEvent);
+          target.removeEventListener("keypress", handleEvent);
+          target.removeEventListener("keyup", handleEvent);
+          resolve();
+        });
+      }
+    }
+    target.addEventListener("keydown", handleEvent);
+    target.addEventListener("keypress", handleEvent);
+    target.addEventListener("keyup", handleEvent);
+
+    synthesizeNativeKey(KEYBOARD_LAYOUT_SPANISH, WIN_VK_RMENU, {},
+                        "", "");
+  });
+}
+
+async function runEmulatingAltGrKeyTest() {
+  return new Promise(resolve => {
+    let target = document.getElementById("input");
+    target.focus();
+
+    let events = [
+      { type: "keydown", code: "ControlLeft", key: "Control", keyCode: KeyboardEvent.DOM_VK_CONTROL },
+      { type: "keydown", code: "AltLeft", key: "Alt", keyCode: KeyboardEvent.DOM_VK_ALT },
+      { type: "keyup", code: "AltLeft", key: "Alt", keyCode: KeyboardEvent.DOM_VK_ALT },
+      { type: "keyup", code: "ControlLeft", key: "Control", keyCode: KeyboardEvent.DOM_VK_CONTROL },
+    ];
+    function handleEvent(aEvent) {
+      checkEvent(aEvent, events, "runEmulatingAltGrKeyTest");
+      if (aEvent.type === "keyup" && aEvent.code === "ControlLeft") {
+        is(events.length, 0, "runAltGrKeyTest: all expected events are fired");
+        SimpleTest.executeSoon(() => {
+          target.removeEventListener("keydown", handleEvent);
+          target.removeEventListener("keypress", handleEvent);
+          target.removeEventListener("keyup", handleEvent);
+          resolve();
+        });
+      }
+    }
+    target.addEventListener("keydown", handleEvent);
+    target.addEventListener("keypress", handleEvent);
+    target.addEventListener("keyup", handleEvent);
+
+    synthesizeNativeKey(KEYBOARD_LAYOUT_SPANISH, WIN_VK_LMENU, { ctrlKey: true },
+                        "", "");
+  });
+}
+
+async function runTests() {
+  await runAltGrKeyTest();
+  await runEmulatingAltGrKeyTest();
+  SimpleTest.finish();
+}
+
+SimpleTest.waitForFocus(runTests);
+</script>
+</body>
+</html>

widget/windows/KeyboardLayout.cpp

@@ -1988,8 +1988,10 @@ NativeKey::InitKeyEvent(WidgetKeyboardEvent& aKeyEvent,
       // If it was followed by a char message but it was consumed by somebody,
       // we should mark it as consumed because somebody must have handled it
       // and we should prevent to do "double action" for the key operation.
+      // However, for compatibility with older version and other browsers,
+      // we should dispatch the events even in the web content.
       if (mCharMessageHasGone) {
-        aKeyEvent.PreventDefaultBeforeDispatch();
+        aKeyEvent.PreventDefaultBeforeDispatch(CrossProcessForwarding::eAllow);
       }
       MOZ_FALLTHROUGH;
     case eKeyDownOnPlugin:
@@ -2005,9 +2007,11 @@ NativeKey::InitKeyEvent(WidgetKeyboardEvent& aKeyEvent,
       // key release, so that the menu bar does not trigger.  This helps avoid
       // triggering the menu bar for ALT key accelerators used in assistive
       // technologies such as Window-Eyes and ZoomText or for switching open
-      // state of IME.
+      // state of IME.  On the other hand, we should dispatch the events even
+      // in the web content for compatibility with older version and other
+      // browsers.
       if (mOriginalVirtualKeyCode == VK_MENU && mMsg.message != WM_SYSKEYUP) {
-        aKeyEvent.PreventDefaultBeforeDispatch();
+        aKeyEvent.PreventDefaultBeforeDispatch(CrossProcessForwarding::eAllow);
       }
       break;
     case eKeyPress: