Bug 1523635 - part 2: Make IMContextWrapper::OnKeyEvent() trust the result of gtk_im_context_filter_keypress() to decide whether handling event causes async event later r=m_kato a=RyanVM
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 05 Feb 2019 11:59:38 +0000
changeset 515783 380761f22d52cf4ed9a0a6e79294acfe149babfa
parent 515782 f950041314ee23bac0947f7b6206af80a8179b37
child 515784 6f1e1adbccb026ce89384465c7a425916d159a22
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, RyanVM
bugs1523635
milestone66.0
Bug 1523635 - part 2: Make IMContextWrapper::OnKeyEvent() trust the result of gtk_im_context_filter_keypress() to decide whether handling event causes async event later r=m_kato a=RyanVM Unfortunately, we're not sure whether ibus always handles non-dead key events asynchronously or synchoronously. Therefore, for safer fix, this patch makes IMContextWrapper::OnKeyEvent() decide that with the result of gtk_im_context_filter_keypress(). If active IME is ibus and it consumes non- synthesized events on password fields, it adjusts probablyHandledAsynchronously value. So, this patch changes the behavior of only when: - active IME is ibus. - only when a password field or ime-mode:disabled field has focus. - not in dead key sequence. - and the key event is consumed by ibus but nothing happend. This must be enough safe to uplift. Differential Revision: https://phabricator.services.mozilla.com/D18635
widget/gtk/IMContextWrapper.cpp
--- a/widget/gtk/IMContextWrapper.cpp
+++ b/widget/gtk/IMContextWrapper.cpp
@@ -822,16 +822,24 @@ KeyHandlingState IMContextWrapper::OnKey
   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 probablyHandledAsynchronously =
       mIsIMInAsyncKeyHandlingMode && currentContext == mContext;
 
+  // If we're not sure whether the event is handled asynchronously, this is
+  // set to true.
+  bool maybeHandledAsynchronously = false;
+
+  // If aEvent is a synthesized event for async handling, this will be set to
+  // true.
+  bool isHandlingAsyncEvent = false;
+
   // 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
@@ -839,17 +847,17 @@ KeyHandlingState IMContextWrapper::OnKey
   if (probablyHandledAsynchronously) {
     switch (mIMContextID) {
       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);
+        isHandlingAsyncEvent = !!(aEvent->state & IBUS_IGNORED_MASK);
         if (!isHandlingAsyncEvent) {
           // On some environments, IBUS_IGNORED_MASK flag is not set as
           // expected.  In such case, we keep pusing all events into the queue.
           // I.e., that causes eating a lot of memory until it's blurred.
           // Therefore, we need to check whether there is same timestamp event
           // in the queue.  This redundant cost should be low because in most
           // causes, key events in the queue should be 2 or 4.
           isHandlingAsyncEvent =
@@ -859,63 +867,67 @@ KeyHandlingState IMContextWrapper::OnKey
                     ("0x%p   OnKeyEvent(), aEvent->state does not have "
                      "IBUS_IGNORED_MASK but "
                      "same event in the queue.  So, assuming it's a "
                      "synthesized event",
                      this));
           }
         }
 
+        // If it's a synthesized event, let's remove it from the posting
+        // event queue first.  Otherwise the following blocks cannot use
+        // `break`.
+        if (isHandlingAsyncEvent) {
+          MOZ_LOG(gGtkIMLog, LogLevel::Info,
+                  ("0x%p   OnKeyEvent(), aEvent->state has IBUS_IGNORED_MASK "
+                   "or aEvent is in the "
+                   "posting event queue, so, it won't be handled "
+                   "asynchronously anymore. Removing "
+                   "the posted events from the queue",
+                   this));
+          probablyHandledAsynchronously = false;
+          mPostingKeyEvents.RemoveEvent(aEvent);
+        }
+
         // ibus won't send back key press events in a dead key sequcne.
         if (mMaybeInDeadKeySequence && aEvent->type == GDK_KEY_PRESS) {
           probablyHandledAsynchronously = false;
           if (isHandlingAsyncEvent) {
             isUnexpectedAsyncEvent = true;
             break;
           }
           // Some keyboard layouts which have dead keys may send
           // "empty" key event to make us call
           // gtk_im_context_filter_keypress() to commit composed
           // character during a GDK_KEY_PRESS event dispatching.
           if (!gdk_keyval_to_unicode(aEvent->keyval) &&
               !aEvent->hardware_keycode) {
             isUnexpectedAsyncEvent = true;
             break;
           }
-        }
-        // ibus handles key events synchronously if focused editor is
-        // <input type="password"> or |ime-mode: disabled;|.
-        if (mInputContext.mIMEState.mEnabled == IMEState::PASSWORD) {
-          probablyHandledAsynchronously = false;
-          isUnexpectedAsyncEvent = isHandlingAsyncEvent;
           break;
         }
-
-        if (isHandlingAsyncEvent) {
-          MOZ_LOG(gGtkIMLog, LogLevel::Info,
-                  ("0x%p   OnKeyEvent(), aEvent->state has IBUS_IGNORED_MASK "
-                   "or aEvent is in the "
-                   "posting event queue, so, it won't be handled "
-                   "asynchronously anymore. Removing "
-                   "the posted events from the queue",
-                   this));
+        // ibus may handle key events synchronously if focused editor is
+        // <input type="password"> or |ime-mode: disabled;|.  However, in
+        // some environments, not so actually.  Therefore, we need to check
+        // the result of gtk_im_context_filter_keypress() later.
+        if (mInputContext.mIMEState.mEnabled == IMEState::PASSWORD) {
           probablyHandledAsynchronously = false;
-          mPostingKeyEvents.RemoveEvent(aEvent);
+          maybeHandledAsynchronously = !isHandlingAsyncEvent;
           break;
         }
         break;
       }
       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);
+        isHandlingAsyncEvent = !!(aEvent->state & FcitxKeyState_IgnoredMask);
         if (!isHandlingAsyncEvent) {
           // On some environments, FcitxKeyState_IgnoredMask flag *might* be not
           // set as expected. If there were such cases, we'd keep pusing all
           // events into the queue.  I.e., that would cause eating a lot of
           // memory until it'd be blurred.  Therefore, we should check whether
           // there is same timestamp event in the queue.  This redundant cost
           // should be low because in most causes, key events in the queue
           // should be 2 or 4.
@@ -983,16 +995,26 @@ KeyHandlingState IMContextWrapper::OnKey
     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);
+
+  // If we're not sure whether the event is handled by IME asynchronously or
+  // synchronously, we need to trust the result of
+  // gtk_im_context_filter_keypress().  If it consumed and but did nothing,
+  // we can assume that another event will be synthesized.
+  if (!isHandlingAsyncEvent && maybeHandledAsynchronously) {
+    probablyHandledAsynchronously |=
+        isFiltered && !mFallbackToKeyEvent && !mKeyboardEventWasDispatched;
+  }
+
   if (aEvent->type == GDK_KEY_PRESS) {
     if (isFiltered && probablyHandledAsynchronously) {
       sWaitingSynthesizedKeyPressHardwareKeyCode = aEvent->hardware_keycode;
     } else {
       sWaitingSynthesizedKeyPressHardwareKeyCode = 0;
     }
   }
 
@@ -1045,27 +1067,29 @@ KeyHandlingState IMContextWrapper::OnKey
     // If the key event hasn't been handled by active IME nor keyboard
     // layout, we can assume that the dead key sequence has been or was
     // ended.  Note that we should not reset it when the key event is
     // GDK_KEY_RELEASE since it may not be filtered by active keyboard
     // layout even in composition.
     mMaybeInDeadKeySequence = false;
   }
 
-  MOZ_LOG(gGtkIMLog, LogLevel::Debug,
-          ("0x%p   OnKeyEvent(), succeeded, filterThisEvent=%s "
-           "(isFiltered=%s, mFallbackToKeyEvent=%s, "
-           "probablyHandledAsynchronously=%s), mPostingKeyEvents.Length()=%zu, "
-           "mCompositionState=%s, mMaybeInDeadKeySequence=%s, "
-           "mKeyboardEventWasDispatched=%s, mKeyboardEventWasConsumed=%s",
-           this, ToChar(filterThisEvent), ToChar(isFiltered),
-           ToChar(mFallbackToKeyEvent), ToChar(probablyHandledAsynchronously),
-           mPostingKeyEvents.Length(), GetCompositionStateName(),
-           ToChar(mMaybeInDeadKeySequence), ToChar(mKeyboardEventWasDispatched),
-           ToChar(mKeyboardEventWasConsumed)));
+  MOZ_LOG(
+      gGtkIMLog, LogLevel::Debug,
+      ("0x%p   OnKeyEvent(), succeeded, filterThisEvent=%s "
+       "(isFiltered=%s, mFallbackToKeyEvent=%s, "
+       "probablyHandledAsynchronously=%s, maybeHandledAsynchronously=%s), "
+       "mPostingKeyEvents.Length()=%zu, mCompositionState=%s, "
+       "mMaybeInDeadKeySequence=%s, mKeyboardEventWasDispatched=%s, "
+       "mKeyboardEventWasConsumed=%s",
+       this, ToChar(filterThisEvent), ToChar(isFiltered),
+       ToChar(mFallbackToKeyEvent), ToChar(probablyHandledAsynchronously),
+       ToChar(maybeHandledAsynchronously), mPostingKeyEvents.Length(),
+       GetCompositionStateName(), ToChar(mMaybeInDeadKeySequence),
+       ToChar(mKeyboardEventWasDispatched), ToChar(mKeyboardEventWasConsumed)));
 
   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.