Bug 1387357 - IMContextWrapper::DispatchCompositionStart() should stop dispatching eCompositionStart if dispatching preceding eKeyDown event causes changing active IM context r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 04 Sep 2017 20:18:43 +0900
changeset 428376 4249740f06643e7d766db832ea15816518455152
parent 428375 71d59cfac7dfb49889416a4c209c7a57bc707c36
child 428377 9d2fb90da23683a14c4d7b947bc1e397567ca3c7
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1387357
milestone57.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 1387357 - IMContextWrapper::DispatchCompositionStart() should stop dispatching eCompositionStart if dispatching preceding eKeyDown event causes changing active IM context r=m_kato If a keydown event handler moves focus like Ctrl+PageDown handler, IM context may be changed to DISABLED or something. In such case, native IME would stop current composition because focus moving in Gecko causes making IME blurred. However, IMContextWrapper::DispatchCompositionStart() always dispatches eCompositionStart even in such case. So, it should stop dispatching eCompositionStart if IME enabled state is changed during dispatching the preceding keydown event. Note that this patch moves the setter of mComposingContext from OnStartCompositionNative() which is a signal listener of "preedit_start" to DispatchCompositionStart() because if IME starts composition without "preedit_start" signal, DispatchCompositionStart() will be called but OnStartCompositionNative() isn't called. However, this fix needs mComposingContext. MozReview-Commit-ID: F3F6NuCOrkJ
widget/gtk/IMContextWrapper.cpp
widget/gtk/IMContextWrapper.h
--- a/widget/gtk/IMContextWrapper.cpp
+++ b/widget/gtk/IMContextWrapper.cpp
@@ -1041,29 +1041,39 @@ IMContextWrapper::OnStartCompositionCall
     aModule->OnStartCompositionNative(aContext);
 }
 
 void
 IMContextWrapper::OnStartCompositionNative(GtkIMContext* aContext)
 {
     MOZ_LOG(gGtkIMLog, LogLevel::Info,
         ("0x%p OnStartCompositionNative(aContext=0x%p), "
-         "current context=0x%p",
-         this, aContext, GetCurrentContext()));
+         "current context=0x%p, mComposingContext=0x%p",
+         this, aContext, GetCurrentContext(), mComposingContext));
 
     // See bug 472635, we should do nothing if IM context doesn't match.
     if (GetCurrentContext() != aContext) {
         MOZ_LOG(gGtkIMLog, LogLevel::Error,
             ("0x%p   OnStartCompositionNative(), FAILED, "
              "given context doesn't match",
              this));
         return;
     }
 
-    mComposingContext = static_cast<GtkIMContext*>(g_object_ref(aContext));
+    if (mComposingContext && aContext != mComposingContext) {
+        // XXX For now, we should ignore this odd case, just logging.
+        MOZ_LOG(gGtkIMLog, LogLevel::Warning,
+            ("0x%p   OnStartCompositionNative(), Warning, "
+             "there is already a composing context but starting new "
+             "composition with different context",
+             this));
+    }
+
+    // IME may start composition without "preedit_start" signal.  Therefore,
+    // mComposingContext will be initialized in DispatchCompositionStart().
 
     if (!DispatchCompositionStart(aContext)) {
         return;
     }
     mCompositionTargetRange.mOffset = mCompositionStart;
     mCompositionTargetRange.mLength = 0;
 }
 
@@ -1074,30 +1084,39 @@ IMContextWrapper::OnEndCompositionCallba
 {
     aModule->OnEndCompositionNative(aContext);
 }
 
 void
 IMContextWrapper::OnEndCompositionNative(GtkIMContext* aContext)
 {
     MOZ_LOG(gGtkIMLog, LogLevel::Info,
-        ("0x%p OnEndCompositionNative(aContext=0x%p)",
-         this, aContext));
+        ("0x%p OnEndCompositionNative(aContext=0x%p), mComposingContext=0x%p",
+         this, aContext, mComposingContext));
 
     // See bug 472635, we should do nothing if IM context doesn't match.
     // Note that if this is called after focus move, the context may different
     // from any our owning context.
     if (!IsValidContext(aContext)) {
         MOZ_LOG(gGtkIMLog, LogLevel::Error,
             ("0x%p    OnEndCompositionNative(), FAILED, "
              "given context doesn't match with any context",
              this));
         return;
     }
 
+    // If we've not started composition with aContext, we should ignore it.
+    if (aContext != mComposingContext) {
+        MOZ_LOG(gGtkIMLog, LogLevel::Warning,
+            ("0x%p    OnEndCompositionNative(), Warning, "
+             "given context doesn't match with mComposingContext",
+             this));
+        return;
+    }
+
     g_object_unref(mComposingContext);
     mComposingContext = nullptr;
 
     // If we already handled the commit event, we should do nothing here.
     if (IsComposing()) {
         if (!DispatchCompositionCommitEvent(aContext)) {
             // If the widget is destroyed, we should do nothing anymore.
             return;
@@ -1116,33 +1135,46 @@ IMContextWrapper::OnChangeCompositionCal
 {
     aModule->OnChangeCompositionNative(aContext);
 }
 
 void
 IMContextWrapper::OnChangeCompositionNative(GtkIMContext* aContext)
 {
     MOZ_LOG(gGtkIMLog, LogLevel::Info,
-        ("0x%p OnChangeCompositionNative(aContext=0x%p)",
-         this, aContext));
+        ("0x%p OnChangeCompositionNative(aContext=0x%p), "
+         "mComposingContext=0x%p",
+         this, aContext, mComposingContext));
 
     // See bug 472635, we should do nothing if IM context doesn't match.
     // Note that if this is called after focus move, the context may different
     // from any our owning context.
     if (!IsValidContext(aContext)) {
         MOZ_LOG(gGtkIMLog, LogLevel::Error,
             ("0x%p   OnChangeCompositionNative(), FAILED, "
              "given context doesn't match with any context",
              this));
         return;
     }
 
+    if (mComposingContext && aContext != mComposingContext) {
+        // XXX For now, we should ignore this odd case, just logging.
+        MOZ_LOG(gGtkIMLog, LogLevel::Warning,
+            ("0x%p   OnChangeCompositionNative(), Warning, "
+             "given context doesn't match with composing context",
+             this));
+    }
+
     nsAutoString compositionString;
     GetCompositionString(aContext, compositionString);
     if (!IsComposing() && compositionString.IsEmpty()) {
+        MOZ_LOG(gGtkIMLog, LogLevel::Warning,
+            ("0x%p   OnChangeCompositionNative(), Warning, does nothing "
+             "because has not started composition and composing string is "
+             "empty", this));
         mDispatchedCompositionString.Truncate();
         return; // Don't start the composition with empty string.
     }
 
     // Be aware, widget can be gone
     DispatchCompositionChangeEvent(aContext, compositionString);
 }
 
@@ -1262,16 +1294,20 @@ IMContextWrapper::OnCommitCompositionNat
     }
 
     // If we are not in composition and committing with empty string,
     // we need to do nothing because if we continued to handle this
     // signal, we would dispatch compositionstart, text, compositionend
     // events with empty string.  Of course, they are unnecessary events
     // for Web applications and our editor.
     if (!IsComposingOn(aContext) && !commitString[0]) {
+        MOZ_LOG(gGtkIMLog, LogLevel::Warning,
+            ("0x%p   OnCommitCompositionNative(), Warning, does nothing "
+             "because has not started composition and commit string is empty",
+             this));
         return;
     }
 
     // If IME doesn't change their keyevent that generated this commit,
     // don't send it through XIM - just send it as a normal key press
     // event.
     // NOTE: While a key event is being handled, this might be caused on
     // current context.  Otherwise, this may be caused on active context.
@@ -1350,45 +1386,73 @@ IMContextWrapper::DispatchCompositionSta
     if (NS_WARN_IF(!EnsureToCacheSelection())) {
         MOZ_LOG(gGtkIMLog, LogLevel::Error,
             ("0x%p   DispatchCompositionStart(), FAILED, "
              "cannot query the selection offset",
              this));
         return false;
     }
 
+    mComposingContext = static_cast<GtkIMContext*>(g_object_ref(aContext));
+    MOZ_ASSERT(mComposingContext);
+
     // Keep the last focused window alive
     RefPtr<nsWindow> lastFocusedWindow(mLastFocusedWindow);
 
     // XXX The composition start point might be changed by composition events
     //     even though we strongly hope it doesn't happen.
     //     Every composition event should have the start offset for the result
     //     because it may high cost if we query the offset every time.
     mCompositionStart = mSelection.mOffset;
     mDispatchedCompositionString.Truncate();
 
     if (mProcessingKeyEvent && !mKeyDownEventWasSent &&
         mProcessingKeyEvent->type == GDK_KEY_PRESS) {
+        // A keydown event handler may change focus with the following keydown
+        // event.  In such case, we need to cancel this composition.  So, we
+        // need to store IM context now because mComposingContext may be
+        // overwritten with different context if calling this method
+        // recursively.
+        // Note that we don't need to grab the context here because |context|
+        // will be used only for checking if it's same as mComposingContext.
+        GtkIMContext* context = mComposingContext;
+
         // If this composition is started by a native keydown event, we need to
         // dispatch our keydown event here (before composition start).
         bool isCancelled;
         mLastFocusedWindow->DispatchKeyDownEvent(mProcessingKeyEvent,
                                                  &isCancelled);
         MOZ_LOG(gGtkIMLog, LogLevel::Debug,
-            ("0x%p   DispatchCompositionStart(), FAILED, keydown event "
-             "is dispatched",
+            ("0x%p   DispatchCompositionStart(), preceding keydown event is "
+             "dispatched",
              this));
         if (lastFocusedWindow->IsDestroyed() ||
             lastFocusedWindow != mLastFocusedWindow) {
-            MOZ_LOG(gGtkIMLog, LogLevel::Error,
-                ("0x%p   DispatchCompositionStart(), FAILED, the focused "
+            MOZ_LOG(gGtkIMLog, LogLevel::Warning,
+                ("0x%p   DispatchCompositionStart(), Warning, the focused "
                  "widget was destroyed/changed by keydown event",
                  this));
             return false;
         }
+
+        // If the dispatched keydown event caused moving focus and that also
+        // caused changing active context, we need to cancel composition here.
+        if (GetCurrentContext() != context) {
+            MOZ_LOG(gGtkIMLog, LogLevel::Warning,
+                ("0x%p   DispatchCompositionStart(), Warning, the preceding "
+                 "keydown event causes changing active IM context",
+                 this));
+            if (mComposingContext == context) {
+                // Only when the context is still composing, we should call
+                // ResetIME() here.  Otherwise, it should've already been
+                // cleaned up.
+                ResetIME();
+            }
+            return false;
+        }
     }
 
     RefPtr<TextEventDispatcher> dispatcher = GetTextEventDispatcher();
     nsresult rv = dispatcher->BeginNativeInputTransaction();
     if (NS_WARN_IF(NS_FAILED(rv))) {
         MOZ_LOG(gGtkIMLog, LogLevel::Error,
             ("0x%p   DispatchCompositionStart(), FAILED, "
              "due to BeginNativeInputTransaction() failure",
--- a/widget/gtk/IMContextWrapper.h
+++ b/widget/gtk/IMContextWrapper.h
@@ -358,18 +358,21 @@ protected:
     void Focus();
 
     // Steals focus from the instance of this class.
     void Blur();
 
     // Initializes the instance.
     void Init();
 
-    // Reset the current composition of IME.  All native composition events
-    // during this processing are ignored.
+    /**
+     * Reset the active context, i.e., if there is mComposingContext, reset it.
+     * Otherwise, reset current context.  Note that all native composition
+     * events during calling this will be ignored.
+     */
     void ResetIME();
 
     // Gets the current composition string by the native APIs.
     void GetCompositionString(GtkIMContext* aContext,
                               nsAString& aCompositionString);
 
     /**
      * Generates our text range array from current composition string.