Bug 1505147 - nsWindow::OnKeyPressEvent() shouldn't dispatch eKeyDown event when IMContextWrapper::OnKeyEvent() has already dispatched it for the event r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 26 Nov 2018 03:26:39 +0000
changeset 507172 f17b7ba6d0aa737d4f69a0fc3206da8c539225e5
parent 507171 a38ee229fe1b7ab29d3bce205b8c8611e4a8297e
child 507173 8e88421b280c2afda62f4ba704ce29701c30549f
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1505147
milestone65.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 1505147 - nsWindow::OnKeyPressEvent() shouldn't dispatch eKeyDown event when IMContextWrapper::OnKeyEvent() has already dispatched it for the event r=m_kato Currently, IMContextWrapper::OnKeyEvent() assumes that IME won't synthesize keyboard event asynchronously again in some cases. For example, one of the cases is that user inputs text with a dead key sequence. However, IME may synthesize key event asynchronously only in a few cases even in a dead key sequence. Unfortunately, for not losing a chance to dispatch eKeyDown/eKeyUp event, we need to keep dispatching eKeyDown or eKeyUp event when we receive original event in dead key sequence. However, according to this bug, we need to stop dispatching eKeyDown and eKeyUp events when we receive unexpected async synthesized key event. If IMContextWrapper::OnKeyEvent() needs to return whether it (has already) dispatched an eKeyDown or eKeyUp and whether it was consumed, then, nsWindow can stop dispatching redundant eKeyDown and eKeyUp events. So, this patch makes IMContextWrapper::OnKeyEvent() return KeyHandlingState enum class instead of just a bool value to notify the caller of detail of the event status. And also makes each caller of nsWindow not dispatch eKeyDown nor eKeyUp event when it returns KeyHandlingState::eNotHandledButDispatched or KeyHandlingState::eNotHandledButConsumed. Differential Revision: https://phabricator.services.mozilla.com/D12517
widget/gtk/IMContextWrapper.cpp
widget/gtk/IMContextWrapper.h
widget/gtk/nsWindow.cpp
--- a/widget/gtk/IMContextWrapper.cpp
+++ b/widget/gtk/IMContextWrapper.cpp
@@ -348,16 +348,17 @@ IMContextWrapper::IMContextWrapper(nsWin
     , mComposingContext(nullptr)
     , mCompositionStart(UINT32_MAX)
     , mProcessingKeyEvent(nullptr)
     , mCompositionState(eCompositionState_NotComposing)
     , mIMContextID(IMContextID::eUnknown)
     , mIsIMFocused(false)
     , mFallbackToKeyEvent(false)
     , mKeyboardEventWasDispatched(false)
+    , mKeyboardEventWasConsumed(false)
     , mIsDeletingSurrounding(false)
     , mLayoutChanged(false)
     , mSetCursorPositionOnKeyEvent(true)
     , mPendingResettingIMContext(false)
     , mRetrieveSurroundingSignalReceived(false)
     , mMaybeInDeadKeySequence(false)
     , mIsIMInAsyncKeyHandlingMode(false)
 {
@@ -807,26 +808,26 @@ IMContextWrapper::OnBlurWindow(nsWindow*
 
     if (!mIsIMFocused || mLastFocusedWindow != aWindow) {
         return;
     }
 
     Blur();
 }
 
-bool
+KeyHandlingState
 IMContextWrapper::OnKeyEvent(nsWindow* aCaller,
                              GdkEventKey* aEvent,
                              bool aKeyboardEventWasDispatched /* = false */)
 {
     MOZ_ASSERT(aEvent, "aEvent must be non-null");
 
     if (!mInputContext.mIMEState.MaybeEditable() ||
         MOZ_UNLIKELY(IsDestroyed())) {
-        return false;
+        return KeyHandlingState::eNotHandled;
     }
 
     MOZ_LOG(gGtkIMLog, LogLevel::Info,
         ("0x%p OnKeyEvent(aCaller=0x%p, "
          "aEvent(0x%p): { type=%s, keyval=%s, unicode=0x%X, state=%s, "
          "time=%u, hardware_keycode=%u, group=%u }, "
          "aKeyboardEventWasDispatched=%s)",
          this, aCaller, aEvent, GetEventType(aEvent),
@@ -844,27 +845,27 @@ IMContextWrapper::OnKeyEvent(nsWindow* a
          GetIMContextIDName(mIMContextID),
          ToChar(mIsIMInAsyncKeyHandlingMode)));
 
     if (aCaller != mLastFocusedWindow) {
         MOZ_LOG(gGtkIMLog, LogLevel::Error,
             ("0x%p   OnKeyEvent(), FAILED, the caller isn't focused "
              "window, mLastFocusedWindow=0x%p",
              this, mLastFocusedWindow));
-        return false;
+        return KeyHandlingState::eNotHandled;
     }
 
     // Even if old IM context has composition, key event should be sent to
     // current context since the user expects so.
     GtkIMContext* currentContext = GetCurrentContext();
     if (MOZ_UNLIKELY(!currentContext)) {
         MOZ_LOG(gGtkIMLog, LogLevel::Error,
             ("0x%p   OnKeyEvent(), FAILED, there are no context",
              this));
-        return false;
+        return KeyHandlingState::eNotHandled;
     }
 
     if (mSetCursorPositionOnKeyEvent) {
         SetCursorPosition(currentContext);
         mSetCursorPositionOnKeyEvent = false;
     }
 
     // Let's support dead key event even if active keyboard layout also
@@ -874,87 +875,113 @@ IMContextWrapper::OnKeyEvent(nsWindow* a
     mMaybeInDeadKeySequence |= isDeadKey;
 
     // If current context is mSimpleContext, both ibus and fcitx handles key
     // events synchronously.  So, only when current context is mContext which
     // is GtkIMMulticontext, the key event may be handled by IME asynchronously.
     bool maybeHandledAsynchronously =
         mIsIMInAsyncKeyHandlingMode && currentContext == mContext;
 
+    // If we've decided that the event won't be synthesized asyncrhonously
+    // by IME, but actually IME did it, this is set to true.
+    bool isUnexpectedAsyncEvent = false;
+
     // If IM is ibus or fcitx and it handles key events asynchronously,
     // they mark aEvent->state as "handled by me" when they post key event
     // to another process.  Unfortunately, we need to check this hacky
     // flag because it's difficult to store all pending key events by
     // an array or a hashtable.
     if (maybeHandledAsynchronously) {
         switch (mIMContextID) {
-            case IMContextID::eIBus:
+            case IMContextID::eIBus: {
+                // See src/ibustypes.h
+                static const guint IBUS_IGNORED_MASK = 1 << 25;
+                // If IBUS_IGNORED_MASK was set to aEvent->state, the event
+                // has already been handled by another process and it wasn't
+                // used by IME.
+                bool isHandlingAsyncEvent =
+                    !!(aEvent->state & IBUS_IGNORED_MASK);
+
                 // ibus won't send back key press events in a dead key sequcne.
                 if (mMaybeInDeadKeySequence && aEvent->type == GDK_KEY_PRESS) {
                     maybeHandledAsynchronously = false;
+                    isUnexpectedAsyncEvent = isHandlingAsyncEvent;
                     break;
                 }
                 // ibus handles key events synchronously if focused editor is
                 // <input type="password"> or |ime-mode: disabled;|.
                 if (mInputContext.mIMEState.mEnabled == IMEState::PASSWORD) {
                     maybeHandledAsynchronously = false;
+                    isUnexpectedAsyncEvent = isHandlingAsyncEvent;
                     break;
                 }
-                // See src/ibustypes.h
-                static const guint IBUS_IGNORED_MASK = 1 << 25;
-                // If IBUS_IGNORED_MASK was set to aEvent->state, the event
-                // has already been handled by another process and it wasn't
-                // used by IME.
-                if (aEvent->state & IBUS_IGNORED_MASK) {
+
+                if (isHandlingAsyncEvent) {
                     MOZ_LOG(gGtkIMLog, LogLevel::Info,
                         ("0x%p   OnKeyEvent(), aEvent->state has "
                          "IBUS_IGNORED_MASK, so, it won't be handled "
                          "asynchronously anymore. Removing posted events from "
                          "the queue",
                          this));
                     maybeHandledAsynchronously = false;
                     mPostingKeyEvents.RemoveEvent(aEvent);
                     break;
                 }
                 break;
-            case IMContextID::eFcitx:
+            }
+            case IMContextID::eFcitx: {
+                // See src/lib/fcitx-utils/keysym.h
+                static const guint FcitxKeyState_IgnoredMask = 1 << 25;
+                // If FcitxKeyState_IgnoredMask was set to aEvent->state,
+                // the event has already been handled by another process and
+                // it wasn't used by IME.
+                bool isHandlingAsyncEvent =
+                    !!(aEvent->state & FcitxKeyState_IgnoredMask);
+
                 // fcitx won't send back key press events in a dead key sequcne.
                 if (mMaybeInDeadKeySequence && aEvent->type == GDK_KEY_PRESS) {
                     maybeHandledAsynchronously = false;
+                    isUnexpectedAsyncEvent = isHandlingAsyncEvent;
                     break;
                 }
 
                 // fcitx handles key events asynchronously even if focused
                 // editor cannot use IME actually.
 
-                // See src/lib/fcitx-utils/keysym.h
-                static const guint FcitxKeyState_IgnoredMask = 1 << 25;
-                // If FcitxKeyState_IgnoredMask was set to aEvent->state,
-                // the event has already been handled by another process and
-                // it wasn't used by IME.
-                if (aEvent->state & FcitxKeyState_IgnoredMask) {
+                if (isHandlingAsyncEvent) {
                     MOZ_LOG(gGtkIMLog, LogLevel::Info,
                         ("0x%p   OnKeyEvent(), aEvent->state has "
                          "FcitxKeyState_IgnoredMask, so, it won't be handled "
                          "asynchronously anymore. Removing posted events from "
                          "the queue",
                          this));
                     maybeHandledAsynchronously = false;
                     mPostingKeyEvents.RemoveEvent(aEvent);
                     break;
                 }
                 break;
+            }
             default:
                 MOZ_ASSERT_UNREACHABLE("IME may handle key event "
                     "asyncrhonously, but not yet confirmed if it comes agian "
                     "actually");
         }
     }
 
-    mKeyboardEventWasDispatched = aKeyboardEventWasDispatched;
+    if (!isUnexpectedAsyncEvent) {
+        mKeyboardEventWasDispatched = aKeyboardEventWasDispatched;
+        mKeyboardEventWasConsumed = false;
+    } else {
+        // If we didn't expect this event, we've alreday dispatched eKeyDown
+        // event or eKeyUp event for that.
+        mKeyboardEventWasDispatched = true;
+        // And in this case, we need to assume that another key event hasn't
+        // been receivied and mKeyboardEventWasConsumed keeps storing the
+        // dispatched eKeyDown or eKeyUp event's state.
+    }
     mFallbackToKeyEvent = false;
     mProcessingKeyEvent = aEvent;
     gboolean isFiltered =
         gtk_im_context_filter_keypress(currentContext, aEvent);
 
     // The caller of this shouldn't handle aEvent anymore if we've dispatched
     // composition events or modified content with other events.
     bool filterThisEvent = isFiltered && !mFallbackToKeyEvent;
@@ -1015,17 +1042,32 @@ IMContextWrapper::OnKeyEvent(nsWindow* a
         ("0x%p   OnKeyEvent(), succeeded, filterThisEvent=%s "
          "(isFiltered=%s, mFallbackToKeyEvent=%s, "
          "maybeHandledAsynchronously=%s), mCompositionState=%s, "
          "mMaybeInDeadKeySequence=%s",
          this, ToChar(filterThisEvent), ToChar(isFiltered),
          ToChar(mFallbackToKeyEvent), ToChar(maybeHandledAsynchronously),
          GetCompositionStateName(), ToChar(mMaybeInDeadKeySequence)));
 
-    return filterThisEvent;
+    if (filterThisEvent) {
+        return KeyHandlingState::eHandled;
+    }
+    // If another call of this method has already dispatched eKeyDown event,
+    // we should return KeyHandlingState::eNotHandledButEventDispatched because
+    // the caller should've stopped handling the event if preceding eKeyDown
+    // event was consumed.
+    if (aKeyboardEventWasDispatched) {
+        return KeyHandlingState::eNotHandledButEventDispatched;
+    }
+    if (!mKeyboardEventWasDispatched) {
+        return KeyHandlingState::eNotHandled;
+    }
+    return mKeyboardEventWasConsumed ?
+         KeyHandlingState::eNotHandledButEventConsumed :
+         KeyHandlingState::eNotHandledButEventDispatched;
 }
 
 void
 IMContextWrapper::OnFocusChangeInGecko(bool aFocus)
 {
     MOZ_LOG(gGtkIMLog, LogLevel::Info,
         ("0x%p OnFocusChangeInGecko(aFocus=%s), "
          "mCompositionState=%s, mIsIMFocused=%s",
@@ -1825,16 +1867,17 @@ IMContextWrapper::OnCommitCompositionNat
 
             // First, dispatch eKeyDown event.
             keyDownEvent.mKeyValue = utf16CommitString;
             nsEventStatus status = nsEventStatus_eIgnore;
             bool dispatched =
                 dispatcher->DispatchKeyboardEvent(eKeyDown, keyDownEvent,
                                                   status, mProcessingKeyEvent);
             if (!dispatched || status == nsEventStatus_eConsumeNoDefault) {
+                mKeyboardEventWasConsumed = true;
                 MOZ_LOG(gGtkIMLog, LogLevel::Info,
                     ("0x%p   OnCommitCompositionNative(), "
                      "doesn't dispatch eKeyPress event because the preceding "
                      "eKeyDown event was not dispatched or was consumed",
                      this));
                 return;
             }
             if (mLastFocusedWindow != keyDownEvent.mWidget ||
@@ -1947,20 +1990,20 @@ IMContextWrapper::MaybeDispatchKeyEventA
         // when we're not in a dead key composition, we should mark the
         // eKeyDown and eKeyUp event as "processed by IME" since we should
         // expose raw keyCode and key value to web apps the key event is a
         // part of a dead key sequence.
         // FYI: We should ignore if default of preceding keydown or keyup
         //      event is prevented since even on the other browsers, web
         //      applications cannot cancel the following composition event.
         //      Spec bug: https://github.com/w3c/uievents/issues/180
-        bool isCancelled;
-        lastFocusedWindow->DispatchKeyDownOrKeyUpEvent(sourceEvent,
-                                                       !mMaybeInDeadKeySequence,
-                                                       &isCancelled);
+        lastFocusedWindow->DispatchKeyDownOrKeyUpEvent(
+                               sourceEvent,
+                               !mMaybeInDeadKeySequence,
+                               &mKeyboardEventWasConsumed);
         MOZ_LOG(gGtkIMLog, LogLevel::Info,
             ("0x%p   MaybeDispatchKeyEventAsProcessedByIME(), keydown or keyup "
              "event is dispatched",
              this));
 
         if (!mProcessingKeyEvent) {
             MOZ_LOG(gGtkIMLog, LogLevel::Info,
                 ("0x%p   MaybeDispatchKeyEventAsProcessedByIME(), removing first "
@@ -2016,19 +2059,19 @@ IMContextWrapper::MaybeDispatchKeyEventA
             // physical key information during composition.
             fakeKeyDownEvent.mCodeNameIndex = CODE_NAME_INDEX_UNKNOWN;
 
             MOZ_LOG(gGtkIMLog, LogLevel::Info,
                 ("0x%p MaybeDispatchKeyEventAsProcessedByIME("
                  "aFollowingEvent=%s), dispatch fake eKeyDown event",
                  this, ToChar(aFollowingEvent)));
 
-            bool isCancelled;
-            lastFocusedWindow->DispatchKeyDownOrKeyUpEvent(fakeKeyDownEvent,
-                                                           &isCancelled);
+            lastFocusedWindow->DispatchKeyDownOrKeyUpEvent(
+                                   fakeKeyDownEvent,
+                                   &mKeyboardEventWasConsumed);
             MOZ_LOG(gGtkIMLog, LogLevel::Info,
                 ("0x%p   MaybeDispatchKeyEventAsProcessedByIME(), "
                  "fake keydown event is dispatched",
                  this));
         }
     }
 
     if (lastFocusedWindow->IsDestroyed() ||
--- a/widget/gtk/IMContextWrapper.h
+++ b/widget/gtk/IMContextWrapper.h
@@ -20,16 +20,32 @@
 #include "mozilla/TextEventDispatcherListener.h"
 #include "WritingModes.h"
 
 class nsWindow;
 
 namespace mozilla {
 namespace widget {
 
+/**
+ * KeyHandlingState is result of IMContextWrapper::OnKeyEvent().
+ */
+enum class KeyHandlingState {
+    // The native key event has not been handled by IMContextWrapper.
+    eNotHandled,
+    // The native key event was handled by IMContextWrapper.
+    eHandled,
+    // The native key event has not been handled by IMContextWrapper,
+    // but eKeyDown or eKeyUp event has been dispatched.
+    eNotHandledButEventDispatched,
+    // The native key event has not been handled by IMContextWrapper,
+    // but eKeyDown or eKeyUp event has been dispatched and consumed.
+    eNotHandledButEventConsumed,
+};
+
 class IMContextWrapper final : public TextEventDispatcherListener
 {
 public:
     // TextEventDispatcherListener implementation
     NS_DECL_ISUPPORTS
 
     NS_IMETHOD NotifyIME(TextEventDispatcher* aTextEventDispatcher,
                          const IMENotification& aNotification) override;
@@ -83,18 +99,18 @@ public:
      * @param aEvent                        A native key press or release
      *                                      event.
      * @param aKeyboardEventWasDispatched   true if eKeyDown or eKeyUp event
      *                                      for aEvent has already been
      *                                      dispatched.  In this case,
      *                                      this class doesn't dispatch
      *                                      keyboard event anymore.
      */
-    bool OnKeyEvent(nsWindow* aWindow, GdkEventKey* aEvent,
-                    bool aKeyboardEventWasDispatched = false);
+    KeyHandlingState OnKeyEvent(nsWindow* aWindow, GdkEventKey* aEvent,
+                                bool aKeyboardEventWasDispatched = false);
 
     // IME related nsIWidget methods.
     nsresult EndIMEComposition(nsWindow* aCaller);
     void SetInputContext(nsWindow* aCaller,
                          const InputContext* aContext,
                          const InputContextAction* aAction);
     InputContext GetInputContext();
     void OnUpdateComposition();
@@ -426,16 +442,19 @@ protected:
     // mKeyboardEventWasDispatched is used by OnKeyEvent() and
     // MaybeDispatchKeyEventAsProcessedByIME().
     // MaybeDispatchKeyEventAsProcessedByIME() dispatches an eKeyDown or
     // eKeyUp event event if the composition is caused by a native
     // key press event.  If this is true, a keyboard event has been dispatched
     // for the native event.  If so, MaybeDispatchKeyEventAsProcessedByIME()
     // won't dispatch keyboard event anymore.
     bool mKeyboardEventWasDispatched;
+    // Whether the keyboard event which as dispatched at setting
+    // mKeyboardEventWasDispatched to true was consumed or not.
+    bool mKeyboardEventWasConsumed;
     // mIsDeletingSurrounding is true while OnDeleteSurroundingNative() is
     // trying to delete the surrounding text.
     bool mIsDeletingSurrounding;
     // mLayoutChanged is true after OnLayoutChange() is called.  This is reset
     // when eCompositionChange is being dispatched.
     bool mLayoutChanged;
     // mSetCursorPositionOnKeyEvent true when caret rect or position is updated
     // with no composition.  If true, we update candidate window position
--- a/widget/gtk/nsWindow.cpp
+++ b/widget/gtk/nsWindow.cpp
@@ -3083,19 +3083,21 @@ nsWindow::OnKeyPressEvent(GdkEventKey *a
     LOGFOCUS(("OnKeyPressEvent [%p]\n", (void *)this));
 
     // if we are in the middle of composing text, XIM gets to see it
     // before mozilla does.
     // FYI: Don't dispatch keydown event before notifying IME of the event
     //      because IME may send a key event synchronously and consume the
     //      original event.
     bool IMEWasEnabled = false;
+    KeyHandlingState handlingState = KeyHandlingState::eNotHandled;
     if (mIMContext) {
         IMEWasEnabled = mIMContext->IsEnabled();
-        if (mIMContext->OnKeyEvent(this, aEvent)) {
+        handlingState = mIMContext->OnKeyEvent(this, aEvent);
+        if (handlingState == KeyHandlingState::eHandled) {
             return TRUE;
         }
     }
 
     // work around for annoying things.
     if (IsCtrlAltTab(aEvent)) {
         return TRUE;
     }
@@ -3104,28 +3106,32 @@ nsWindow::OnKeyPressEvent(GdkEventKey *a
 
     // Dispatch keydown event always.  At auto repeating, we should send
     // KEYDOWN -> KEYPRESS -> KEYDOWN -> KEYPRESS ... -> KEYUP
     // However, old distributions (e.g., Ubuntu 9.10) sent native key
     // release event, so, on such platform, the DOM events will be:
     // KEYDOWN -> KEYPRESS -> KEYUP -> KEYDOWN -> KEYPRESS -> KEYUP...
 
     bool isKeyDownCancelled = false;
-    if (DispatchKeyDownOrKeyUpEvent(aEvent, false, &isKeyDownCancelled) &&
-        (MOZ_UNLIKELY(mIsDestroyed) || isKeyDownCancelled)) {
-        return TRUE;
+    if (handlingState == KeyHandlingState::eNotHandled) {
+        if (DispatchKeyDownOrKeyUpEvent(aEvent, false, &isKeyDownCancelled) &&
+            (MOZ_UNLIKELY(mIsDestroyed) || isKeyDownCancelled)) {
+            return TRUE;
+        }
+        handlingState = KeyHandlingState::eNotHandledButEventDispatched;
     }
 
     // If a keydown event handler causes to enable IME, i.e., it moves
     // focus from IME unusable content to IME usable editor, we should
     // send the native key event to IME for the first input on the editor.
     if (!IMEWasEnabled && mIMContext && mIMContext->IsEnabled()) {
         // Notice our keydown event was already dispatched.  This prevents
         // unnecessary DOM keydown event in the editor.
-        if (mIMContext->OnKeyEvent(this, aEvent, true)) {
+        handlingState =  mIMContext->OnKeyEvent(this, aEvent, true);
+        if (handlingState == KeyHandlingState::eHandled) {
             return TRUE;
         }
     }
 
     // Look for specialized app-command keys
     switch (aEvent->keyval) {
         case GDK_Back:
             return DispatchCommandEvent(nsGkAtoms::Back);
@@ -3241,18 +3247,21 @@ nsWindow::MaybeDispatchContextMenuEvent(
     return true;
 }
 
 gboolean
 nsWindow::OnKeyReleaseEvent(GdkEventKey* aEvent)
 {
     LOGFOCUS(("OnKeyReleaseEvent [%p]\n", (void *)this));
 
-    if (mIMContext && mIMContext->OnKeyEvent(this, aEvent)) {
-        return TRUE;
+    if (mIMContext) {
+        KeyHandlingState handlingState = mIMContext->OnKeyEvent(this, aEvent);
+        if (handlingState != KeyHandlingState::eNotHandled) {
+            return TRUE;
+        }
     }
 
     bool isCancelled = false;
     if (NS_WARN_IF(!DispatchKeyDownOrKeyUpEvent(aEvent, false, &isCancelled))) {
         return FALSE;
     }
 
     return TRUE;