From 36f73c43d826df6654c035299356cc16443973aa Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Mon, 25 Jun 2018 18:17:18 +0900 Subject: [PATCH] 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 | 3 +- dom/ipc/TabChild.cpp | 8 ++ layout/base/PresShell.cpp | 4 +- widget/BasicEvents.h | 24 +++- widget/TextEventDispatcher.cpp | 8 +- widget/tests/mochitest.ini | 2 + ..._key_events_in_web_content_on_windows.html | 106 ++++++++++++++++++ widget/windows/KeyboardLayout.cpp | 10 +- 8 files changed, 153 insertions(+), 12 deletions(-) create mode 100644 widget/tests/test_AltGr_key_events_in_web_content_on_windows.html diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp index 58388e9cfd62..2a15fa7d409d 100644 --- a/dom/events/EventStateManager.cpp +++ b/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; } diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp index 3e0f97cfbefc..0930b060d3f2 100644 --- a/dom/ipc/TabChild.cpp +++ b/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::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); } diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp index 4c523c79d0ec..db43e820eccf 100644 --- a/layout/base/PresShell.cpp +++ b/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(); diff --git a/widget/BasicEvents.h b/widget/BasicEvents.h index 1d55087db86e..dd8455d1e9ab 100644 --- a/widget/BasicEvents.h +++ b/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 { diff --git a/widget/TextEventDispatcher.cpp b/widget/TextEventDispatcher.cpp index 00eb2de4b89b..bb87c70301fd 100644 --- a/widget/TextEventDispatcher.cpp +++ b/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. diff --git a/widget/tests/mochitest.ini b/widget/tests/mochitest.ini index 5e797b13f37b..ff03812ee60f 100644 --- a/widget/tests/mochitest.ini +++ b/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 diff --git a/widget/tests/test_AltGr_key_events_in_web_content_on_windows.html b/widget/tests/test_AltGr_key_events_in_web_content_on_windows.html new file mode 100644 index 000000000000..db8c23ffbf52 --- /dev/null +++ b/widget/tests/test_AltGr_key_events_in_web_content_on_windows.html @@ -0,0 +1,106 @@ + + + + Testing if AltGr keydown and keyup events are fired in web content on Windows + + + + + + +
+ +
+ +
+
+ + + + \ No newline at end of file diff --git a/widget/windows/KeyboardLayout.cpp b/widget/windows/KeyboardLayout.cpp index d4c1b8688eb7..112905ff32ac 100644 --- a/widget/windows/KeyboardLayout.cpp +++ b/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: