Bug 1435717 - Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 25 Jun 2018 18:17:18 +0900
changeset 482229 c0cd065ee5c8832f6962d09805541ecad1d5398e
parent 482228 5540e1b64aa27358f495b8842ba6857b393be203
child 482257 19766d4c54e3c0ef09caf3c9a7fd3f162e4d5ac6
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1435717
milestone63.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 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
dom/events/EventStateManager.cpp
dom/ipc/TabChild.cpp
layout/base/PresShell.cpp
widget/BasicEvents.h
widget/TextEventDispatcher.cpp
widget/tests/mochitest.ini
widget/tests/test_AltGr_key_events_in_web_content_on_windows.html
widget/windows/KeyboardLayout.cpp
--- a/dom/events/EventStateManager.cpp
+++ b/dom/events/EventStateManager.cpp
@@ -1380,18 +1380,17 @@ bool
 EventStateManager::IsRemoteTarget(nsIContent* target)
 {
   return !!TabParent::GetFrom(target);
 }
 
 bool
 EventStateManager::HandleCrossProcessEvent(WidgetEvent* aEvent,
                                            nsEventStatus *aStatus) {
-  if (*aStatus == nsEventStatus_eConsumeNoDefault ||
-      !aEvent->CanBeSentToRemoteProcess()) {
+  if (!aEvent->CanBeSentToRemoteProcess()) {
     return false;
   }
 
   MOZ_ASSERT(!aEvent->HasBeenPostedToRemoteProcess(),
     "Why do we need to post same event to remote processes again?");
 
   // Collect the remote event targets we're going to forward this
   // event to.
--- a/dom/ipc/TabChild.cpp
+++ b/dom/ipc/TabChild.cpp
@@ -2120,16 +2120,24 @@ TabChild::RecvRealKeyEvent(const WidgetK
     // as a default handler.  For example, when an eKeyPress event matches
     // with a content accesskey, and it's executed, peventDefault() of the
     // event won't be called but the status is set to "no default".  Then,
     // the event shouldn't be handled by nsMenuBarListener in the main process.
     if (!localEvent.DefaultPrevented() &&
         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);
   }
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 TabChild::RecvNormalPriorityRealKeyEvent(const WidgetKeyboardEvent& aEvent)
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -7480,17 +7480,17 @@ PresShell::HandleEventInternal(WidgetEve
             // handler from stopping all loads in the document, which
             // would cause <video> loads to stop.
             // XXX We need to claim the Escape key event which will be
             //     dispatched only into chrome is already consumed by
             //     content because we need to prevent its default here
             //     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
             // calling prevent default on the preceding keydown/press events.
             if (!mIsLastChromeOnlyEscapeKeyConsumed &&
                 aEvent->mMessage == eKeyUp) {
               // ESC key released while in DOM fullscreen mode.
               // Fully exit all browser windows and documents from
@@ -7499,17 +7499,17 @@ PresShell::HandleEventInternal(WidgetEve
             }
           }
           nsCOMPtr<nsIDocument> pointerLockedDoc =
             do_QueryReferent(EventStateManager::sPointerLockedDoc);
           if (!mIsLastChromeOnlyEscapeKeyConsumed && pointerLockedDoc) {
             // 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();
             }
           }
         }
         if (keyCode != NS_VK_ESCAPE && keyCode != NS_VK_SHIFT &&
             keyCode != NS_VK_CONTROL && keyCode != NS_VK_ALT &&
--- a/widget/BasicEvents.h
+++ b/widget/BasicEvents.h
@@ -26,16 +26,27 @@ namespace IPC {
 template<typename T>
 struct ParamTraits;
 } // namespace IPC
 
 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
  *
  * BaseEventFlags must be a POD struct for safe to use memcpy (including
  * in ParamTraits<BaseEventFlags>).  So don't make virtual methods, constructor,
  * destructor and operators.
  * This is necessary for VC which is NOT C++0x compiler.
  ******************************************************************************/
@@ -185,28 +196,33 @@ public:
       return;
     }
     mDefaultPrevented = true;
     // Note that even if preventDefault() has already been called by chrome,
     // a call of preventDefault() by content needs to overwrite
     // 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
   {
     return mDefaultPrevented;
   }
   inline bool DefaultPreventedByContent() const
   {
     MOZ_ASSERT(!mDefaultPreventedByContent || DefaultPrevented());
@@ -646,17 +662,21 @@ public:
   /**
    * Helper methods for methods of DOM Event.
    */
   void StopPropagation() { mFlags.StopPropagation(); }
   void StopImmediatePropagation() { mFlags.StopImmediatePropagation(); }
   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
   {
     return mFlags.DefaultPreventedByContent();
   }
   bool IsTrusted() const { return mFlags.IsTrusted(); }
   bool PropagationStopped() const { return mFlags.PropagationStopped(); }
 
--- a/widget/TextEventDispatcher.cpp
+++ b/widget/TextEventDispatcher.cpp
@@ -598,19 +598,21 @@ TextEventDispatcher::DispatchKeyboardEve
   }
 
   WidgetKeyboardEvent keyEvent(true, aMessage, mWidget);
   InitEvent(keyEvent);
   keyEvent.AssignKeyEventData(aKeyboardEvent, false);
 
   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.
   if (keyEvent.mKeyNameIndex != KEY_NAME_INDEX_USE_STRING) {
     MOZ_ASSERT(!aIndexOfKeypress,
       "aIndexOfKeypress must be 0 for non-printable key");
     // If the keyboard event isn't caused by printable key, its charCode should
     // be 0.
--- a/widget/tests/mochitest.ini
+++ b/widget/tests/mochitest.ini
@@ -1,11 +1,13 @@
 [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
 [test_keypress_event_with_alt_on_mac.html]
 skip-if = toolkit != "cocoa"
 [test_picker_no_crash.html]
 skip-if = toolkit != "windows" || e10s # Bug 1267491
 support-files = window_picker_no_crash_child.html
new file mode 100644
--- /dev/null
+++ b/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>
\ No newline at end of file
--- a/widget/windows/KeyboardLayout.cpp
+++ b/widget/windows/KeyboardLayout.cpp
@@ -1983,36 +1983,40 @@ NativeKey::InitKeyEvent(WidgetKeyboardEv
   LayoutDeviceIntPoint point(0, 0);
   mWidget->InitEvent(aKeyEvent, &point);
 
   switch (aKeyEvent.mMessage) {
     case eKeyDown:
       // 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:
       aKeyEvent.mKeyCode = mDOMKeyCode;
       // Unique id for this keydown event and its associated keypress.
       sUniqueKeyEventId++;
       aKeyEvent.mUniqueId = sUniqueKeyEventId;
       break;
     case eKeyUp:
     case eKeyUpOnPlugin:
       aKeyEvent.mKeyCode = mDOMKeyCode;
       // Set defaultPrevented of the key event if the VK_MENU is not a system
       // 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:
       MOZ_ASSERT(!mCharMessageHasGone,
                  "If following char message was consumed by somebody, "
                  "keydown event should be consumed above");
       aKeyEvent.mUniqueId = sUniqueKeyEventId;
       break;