Bug 1466208 - part 39: Create PresShell::EventHandler::MaybeHandleKeyboardEventBeforeDispatch() r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 08 Mar 2019 12:46:17 +0000
changeset 521271 199af6c43953e40303895707c3d38ca162ef6844
parent 521270 72d3e9ad8d2c4ecd7a6971318ef07e46422304e9
child 521272 63bd1994e17c43e699c23f11ca01266d48e61d1e
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1466208
milestone67.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 1466208 - part 39: Create PresShell::EventHandler::MaybeHandleKeyboardEventBeforeDispatch() r=smaug `PresShell::EventHandler::HandleEventInternal()` may handle `Escape` key before dispatching it in some cases. This requires too many lines for somebody who investigate the method for the other events. Therefore, this patch moves it into the new method. Additionally, this patch creates `WidgetKeyboardEvent::CanTreatAsUserInput()` and `WidgetKeyboardEvent::ShouldInteractionTimeRecorded()` because we should manage similar checks in one place (we already have `WidgetKeyboardEvent::CanUserGestureActivateTarget()`). Finally, their conditions are not enough for what the comment wants to do there since they do not check some modifier keys. Therefore, this patch makes them check all possible modifier keys too. Differential Revision: https://phabricator.services.mozilla.com/D21340
layout/base/PresShell.cpp
layout/base/PresShell.h
widget/TextEvents.h
--- a/layout/base/PresShell.cpp
+++ b/layout/base/PresShell.cpp
@@ -7659,67 +7659,22 @@ nsresult PresShell::EventHandler::Handle
     if (aEvent->IsUserAction()) {
       mPresShell->mHasHandledUserInput = true;
     }
 
     switch (aEvent->mMessage) {
       case eKeyPress:
       case eKeyDown:
       case eKeyUp: {
-        Document* doc = mPresShell->GetCurrentEventContent()
-                            ? mPresShell->mCurrentEventContent->OwnerDoc()
-                            : nullptr;
-        auto keyCode = aEvent->AsKeyboardEvent()->mKeyCode;
-        if (keyCode == NS_VK_ESCAPE) {
-          Document* root = nsContentUtils::GetRootDocument(doc);
-          if (root && root->GetFullscreenElement()) {
-            // Prevent default action on ESC key press when exiting
-            // DOM fullscreen mode. This prevents the browser ESC key
-            // 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(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 (!mPresShell->mIsLastChromeOnlyEscapeKeyConsumed &&
-                aEvent->mMessage == eKeyUp) {
-              // ESC key released while in DOM fullscreen mode.
-              // Fully exit all browser windows and documents from
-              // fullscreen mode.
-              Document::AsyncExitFullscreen(nullptr);
-            }
-          }
-          nsCOMPtr<Document> pointerLockedDoc =
-              do_QueryReferent(EventStateManager::sPointerLockedDoc);
-          if (!mPresShell->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(CrossProcessForwarding::eStop);
-            aEvent->mFlags.mOnlyChromeDispatch = true;
-            if (aEvent->mMessage == eKeyUp) {
-              Document::UnlockPointer();
-            }
-          }
-        }
-        // Allow keys other than ESC and modifiers be marked as a
-        // valid user input for triggering popup, fullscreen, and
-        // pointer lock.
-        isHandlingUserInput =
-            keyCode != NS_VK_ESCAPE && keyCode != NS_VK_SHIFT &&
-            keyCode != NS_VK_CONTROL && keyCode != NS_VK_ALT &&
-            keyCode != NS_VK_WIN && keyCode != NS_VK_META;
+        WidgetKeyboardEvent* keyboardEvent = aEvent->AsKeyboardEvent();
+        MaybeHandleKeyboardEventBeforeDispatch(keyboardEvent);
+        // Not all keyboard events are treated as user input, so that popups
+        // can't be opened, fullscreen mode can't be started, etc at unexpected
+        // time.
+        isHandlingUserInput = keyboardEvent->CanTreatAsUserInput();
         break;
       }
       case eMouseDown:
       case eMouseUp:
       case ePointerDown:
       case ePointerUp:
         isHandlingUserInput = true;
         break;
@@ -7911,37 +7866,81 @@ bool PresShell::EventHandler::PrepareToD
   // of not cancelable.
   if (mouseEvent->IsShift()) {
     aEvent->mFlags.mOnlyChromeDispatch = true;
     aEvent->mFlags.mRetargetToNonNativeAnonymous = true;
   }
   return true;
 }
 
+void PresShell::EventHandler::MaybeHandleKeyboardEventBeforeDispatch(
+    WidgetKeyboardEvent* aKeyboardEvent) {
+  MOZ_ASSERT(aKeyboardEvent);
+
+  if (aKeyboardEvent->mKeyCode != NS_VK_ESCAPE) {
+    return;
+  }
+
+  // If we're in fullscreen mode, exit from it forcibly when Escape key is
+  // pressed.
+  Document* doc = mPresShell->GetCurrentEventContent()
+                      ? mPresShell->mCurrentEventContent->OwnerDoc()
+                      : nullptr;
+  Document* root = nsContentUtils::GetRootDocument(doc);
+  if (root && root->GetFullscreenElement()) {
+    // Prevent default action on ESC key press when exiting
+    // DOM fullscreen mode. This prevents the browser ESC key
+    // 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.
+    aKeyboardEvent->PreventDefaultBeforeDispatch(CrossProcessForwarding::eStop);
+    aKeyboardEvent->mFlags.mOnlyChromeDispatch = true;
+
+    // The event listeners in chrome can prevent this ESC behavior by
+    // calling prevent default on the preceding keydown/press events.
+    if (!mPresShell->mIsLastChromeOnlyEscapeKeyConsumed &&
+        aKeyboardEvent->mMessage == eKeyUp) {
+      // ESC key released while in DOM fullscreen mode.
+      // Fully exit all browser windows and documents from
+      // fullscreen mode.
+      Document::AsyncExitFullscreen(nullptr);
+    }
+  }
+
+  nsCOMPtr<Document> pointerLockedDoc =
+      do_QueryReferent(EventStateManager::sPointerLockedDoc);
+  if (!mPresShell->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.
+    aKeyboardEvent->PreventDefaultBeforeDispatch(CrossProcessForwarding::eStop);
+    aKeyboardEvent->mFlags.mOnlyChromeDispatch = true;
+    if (aKeyboardEvent->mMessage == eKeyUp) {
+      Document::UnlockPointer();
+    }
+  }
+}
+
 void PresShell::EventHandler::RecordEventPreparationPerformance(
     const WidgetEvent* aEvent) {
   MOZ_ASSERT(aEvent);
 
   switch (aEvent->mMessage) {
     case eKeyPress:
     case eKeyDown:
     case eKeyUp:
-      switch (aEvent->AsKeyboardEvent()->mKeyCode) {
-        case NS_VK_ESCAPE:
-        case NS_VK_SHIFT:
-        case NS_VK_CONTROL:
-        case NS_VK_ALT:
-        case NS_VK_WIN:
-        case NS_VK_META:
-          break;
-        default:
-          GetPresContext()->RecordInteractionTime(
-              nsPresContext::InteractionType::eKeyInteraction,
-              aEvent->mTimeStamp);
-          break;
+      if (aEvent->AsKeyboardEvent()->ShouldInteractionTimeRecorded()) {
+        GetPresContext()->RecordInteractionTime(
+            nsPresContext::InteractionType::eKeyInteraction,
+            aEvent->mTimeStamp);
       }
       Telemetry::AccumulateTimeDelta(Telemetry::INPUT_EVENT_QUEUED_KEYBOARD_MS,
                                      aEvent->mTimeStamp);
       return;
 
     case eMouseDown:
     case eMouseUp:
       Telemetry::AccumulateTimeDelta(Telemetry::INPUT_EVENT_QUEUED_CLICK_MS,
--- a/layout/base/PresShell.h
+++ b/layout/base/PresShell.h
@@ -1081,16 +1081,26 @@ class PresShell final : public nsIPresSh
      * RecordEventHandlingResponsePerformance() records event handling response
      * performance with telemetry.
      *
      * @param aEvent            The handled event.
      */
     void RecordEventHandlingResponsePerformance(const WidgetEvent* aEvent);
 
     /**
+     * MaybeHandleKeyboardEventBeforeDispatch() may handle aKeyboardEvent
+     * if it should do something before dispatched into the DOM.
+     *
+     * @param aKeyboardEvent    The handling keyboard event.
+     */
+    MOZ_CAN_RUN_SCRIPT
+    void MaybeHandleKeyboardEventBeforeDispatch(
+        WidgetKeyboardEvent* aKeyboardEvent);
+
+    /**
      * PrepareToDispatchContextMenuEvent() prepares to dispatch aEvent into
      * the DOM.
      *
      * @param aEvent            Must be eContextMenu event.
      * @return                  true if it can be dispatched into the DOM.
      *                          Otherwise, false.
      */
     bool PrepareToDispatchContextMenuEvent(WidgetEvent* aEvent);
--- a/widget/TextEvents.h
+++ b/widget/TextEvents.h
@@ -311,16 +311,62 @@ class WidgetKeyboardEvent : public Widge
                                               (IsAlt() && !IsAltGraph()) ||
                                               IsMeta() || IsOS();
     const bool isEnterOrSpaceKey =
         mKeyNameIndex == KEY_NAME_INDEX_Enter || mKeyCode == NS_VK_SPACE;
     return (PseudoCharCode() || isEnterOrSpaceKey) &&
            !isCombiningWithOperationKeys;
   }
 
+  /**
+   * CanTreatAsUserInput() returns true if the key is pressed for perhaps
+   * doing something on the web app or our UI.  This means that when this
+   * returns false, e.g., when user presses a modifier key, user is probably
+   * displeased by opening popup, entering fullscreen mode, etc.  Therefore,
+   * only when this returns true, such reactions should be allowed.
+   */
+  bool CanTreatAsUserInput() const {
+    if (!IsTrusted()) {
+      return false;
+    }
+    switch (mKeyNameIndex) {
+      case KEY_NAME_INDEX_Escape:
+      // modifier keys:
+      case KEY_NAME_INDEX_Alt:
+      case KEY_NAME_INDEX_AltGraph:
+      case KEY_NAME_INDEX_CapsLock:
+      case KEY_NAME_INDEX_Control:
+      case KEY_NAME_INDEX_Fn:
+      case KEY_NAME_INDEX_FnLock:
+      case KEY_NAME_INDEX_Meta:
+      case KEY_NAME_INDEX_NumLock:
+      case KEY_NAME_INDEX_ScrollLock:
+      case KEY_NAME_INDEX_Shift:
+      case KEY_NAME_INDEX_Symbol:
+      case KEY_NAME_INDEX_SymbolLock:
+      // legacy modifier keys:
+      case KEY_NAME_INDEX_Hyper:
+      case KEY_NAME_INDEX_Super:
+      // obsolete modifier key:
+      case KEY_NAME_INDEX_OS:
+        return false;
+      default:
+        return true;
+    }
+  }
+
+  /**
+   * ShouldInteractionTimeRecorded() returns true if the handling time of
+   * the event should be recorded with the telemetry.
+   */
+  bool ShouldInteractionTimeRecorded() const {
+    // Let's record only when we can treat the instance is a user input.
+    return CanTreatAsUserInput();
+  }
+
   // OS translated Unicode chars which are used for accesskey and accelkey
   // handling. The handlers will try from first character to last character.
   nsTArray<AlternativeCharCode> mAlternativeCharCodes;
   // DOM KeyboardEvent.key only when mKeyNameIndex is KEY_NAME_INDEX_USE_STRING.
   nsString mKeyValue;
   // DOM KeyboardEvent.code only when mCodeNameIndex is
   // CODE_NAME_INDEX_USE_STRING.
   nsString mCodeValue;