Bug 1317906 - When a key press causes a call of InsertText(), it shouldn't mark keypress as consumed but instead, should mark InsertText() caused composition. r=m_kato, a=gchang
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 17 Nov 2016 13:35:21 +0900
changeset 367665 ef9b2a0ba51b16e65d42728d8aacd265b926bb9f
parent 367664 7a0a906b9680cd0717436c74d5ed3d33e68745d0
child 367666 6e687af3b7cb22c4f0f5fce6d08e7e266bbe191b
push id1369
push userjlorenzo@mozilla.com
push dateMon, 27 Feb 2017 14:59:41 +0000
treeherdermozilla-release@d75a1dba431f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, gchang
bugs1317906
milestone52.0a2
Bug 1317906 - When a key press causes a call of InsertText(), it shouldn't mark keypress as consumed but instead, should mark InsertText() caused composition. r=m_kato, a=gchang Currently, when InsertText() which is caused by a key press causes committing composition, it consumes keypress event. However, Korean 2-set IME calls InsertText() two times when there is composition and key press causes inserting another character next to the composition. In this case, current design ignores second InsertText() becuase keypress event is already consumed by the first InsertText() call. For solving this issue safely, InsertText() should mark current key event as "dispatched composition event". Then, following InsertText() calls should cause composition events instead of keypress events since following event order is too odd: 1. keydown (currently not dispatched by TextEventDisaptcher) 2. compositionupdate 3. compositionend 4. keypress 5. keyup with the new design this becomes: 1. keydown (currently not dispatched by TextEventDispatcher) 2. compositionupdate 3. compositionend 4. compositionstart 5. compositionupdate 6. compositionend 7. keyup This is similar to Chromium, although, Chromium includes the second InsertText() call into the first composition, we need to fix it later due to risky. MozReview-Commit-ID: GL42cU2WIL0
widget/cocoa/TextInputHandler.h
widget/cocoa/TextInputHandler.mm
--- a/widget/cocoa/TextInputHandler.h
+++ b/widget/cocoa/TextInputHandler.h
@@ -516,16 +516,20 @@ protected:
     // Whether keydown event was consumed by web contents or chrome contents.
     bool mKeyDownHandled;
     // Whether keypress event was dispatched for mKeyEvent.
     bool mKeyPressDispatched;
     // Whether keypress event was consumed by web contents or chrome contents.
     bool mKeyPressHandled;
     // Whether the key event causes other key events via IME or something.
     bool mCausedOtherKeyEvents;
+    // Whether the key event causes composition change or committing
+    // composition.  So, even if InsertText() is called, this may be false
+    // if it dispatches keypress event.
+    bool mCompositionDispatched;
 
     KeyEventState() : mKeyEvent(nullptr)
     {
       Clear();
     }    
 
     explicit KeyEventState(NSEvent* aNativeKeyEvent) : mKeyEvent(nullptr)
     {
@@ -554,21 +558,23 @@ protected:
         mKeyEvent = nullptr;
       }
       mInsertString = nullptr;
       mInsertedString.Truncate();
       mKeyDownHandled = false;
       mKeyPressDispatched = false;
       mKeyPressHandled = false;
       mCausedOtherKeyEvents = false;
+      mCompositionDispatched = false;
     }
 
     bool IsDefaultPrevented() const
     {
-      return mKeyDownHandled || mKeyPressHandled || mCausedOtherKeyEvents;
+      return mKeyDownHandled || mKeyPressHandled || mCausedOtherKeyEvents ||
+             mCompositionDispatched;
     }
 
     bool CanDispatchKeyPressEvent() const
     {
       return !mKeyPressDispatched && !IsDefaultPrevented();
     }
 
     void InitKeyEvent(TextInputHandlerBase* aHandler,
--- a/widget/cocoa/TextInputHandler.mm
+++ b/widget/cocoa/TextInputHandler.mm
@@ -1679,20 +1679,22 @@ TextInputHandler::HandleKeyDownEvent(NSE
          this));
     }
   }
 
   // Note: mWidget might have become null here. Don't count on it from here on.
 
   MOZ_LOG(gLog, LogLevel::Info,
     ("%p TextInputHandler::HandleKeyDownEvent, "
-     "keydown handled=%s, keypress handled=%s, causedOtherKeyEvents=%s",
+     "keydown handled=%s, keypress handled=%s, causedOtherKeyEvents=%s, "
+     "compositionDispatched=%s",
      this, TrueOrFalse(currentKeyEvent->mKeyDownHandled),
      TrueOrFalse(currentKeyEvent->mKeyPressHandled),
-     TrueOrFalse(currentKeyEvent->mCausedOtherKeyEvents)));
+     TrueOrFalse(currentKeyEvent->mCausedOtherKeyEvents),
+     TrueOrFalse(currentKeyEvent->mCompositionDispatched)));
   // Insert empty line to the log for easier to read.
   MOZ_LOG(gLog, LogLevel::Info, (""));
   return currentKeyEvent->IsDefaultPrevented();
 
   NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(false);
 }
 
 void
@@ -2118,28 +2120,30 @@ TextInputHandler::InsertText(NSAttribute
 
   KeyEventState* currentKeyEvent = GetCurrentKeyEvent();
 
   MOZ_LOG(gLog, LogLevel::Info,
     ("%p TextInputHandler::InsertText, aAttrString=\"%s\", "
      "aReplacementRange=%p { location=%llu, length=%llu }, "
      "IsIMEComposing()=%s, IgnoreIMEComposition()=%s, "
      "keyevent=%p, keydownHandled=%s, keypressDispatched=%s, "
-     "causedOtherKeyEvents=%s",
+     "causedOtherKeyEvents=%s, compositionDispatched=%s",
      this, GetCharacters([aAttrString string]), aReplacementRange,
      aReplacementRange ? aReplacementRange->location : 0,
      aReplacementRange ? aReplacementRange->length : 0,
      TrueOrFalse(IsIMEComposing()), TrueOrFalse(IgnoreIMEComposition()),
      currentKeyEvent ? currentKeyEvent->mKeyEvent : nullptr,
      currentKeyEvent ?
        TrueOrFalse(currentKeyEvent->mKeyDownHandled) : "N/A",
      currentKeyEvent ?
        TrueOrFalse(currentKeyEvent->mKeyPressDispatched) : "N/A",
      currentKeyEvent ?
-       TrueOrFalse(currentKeyEvent->mCausedOtherKeyEvents) : "N/A"));
+       TrueOrFalse(currentKeyEvent->mCausedOtherKeyEvents) : "N/A",
+     currentKeyEvent ?
+       TrueOrFalse(currentKeyEvent->mCompositionDispatched) : "N/A"));
 
   if (IgnoreIMEComposition()) {
     return;
   }
 
   InputContext context = mWidget->GetInputContext();
   bool isEditable = (context.mIMEState.mEnabled == IMEState::ENABLED ||
                      context.mIMEState.mEnabled == IMEState::PASSWORD);
@@ -2195,21 +2199,26 @@ TextInputHandler::InsertText(NSAttribute
   bool isReplacingSpecifiedRange =
     isEditable && aReplacementRange &&
     aReplacementRange->location != NSNotFound &&
     !NSEqualRanges(selectedRange, *aReplacementRange);
 
   // If this is not caused by pressing a key, there is a composition or
   // replacing a range which is different from current selection, let's
   // insert the text as committing a composition.
-  if (!currentKeyEvent || IsIMEComposing() || isReplacingSpecifiedRange) {
+  // If InsertText() is called two or more times, we should insert all
+  // text with composition events.
+  // XXX When InsertText() is called multiple times, Chromium dispatches
+  //     only one composition event.  So, we need to store InsertText()
+  //     calls and flush later.
+  if (!currentKeyEvent || currentKeyEvent->mCompositionDispatched ||
+      IsIMEComposing() || isReplacingSpecifiedRange) {
     InsertTextAsCommittingComposition(aAttrString, aReplacementRange);
     if (currentKeyEvent) {
-      currentKeyEvent->mKeyPressHandled = true;
-      currentKeyEvent->mKeyPressDispatched = true;
+      currentKeyEvent->mCompositionDispatched = true;
     }
     return;
   }
 
   // Don't let the same event be fired twice when hitting
   // enter/return! (Bug 420502)
   if (currentKeyEvent && !currentKeyEvent->CanDispatchKeyPressEvent()) {
     return;
@@ -3152,38 +3161,40 @@ IMEInputHandler::SetMarkedText(NSAttribu
   KeyEventState* currentKeyEvent = GetCurrentKeyEvent();
 
   MOZ_LOG(gLog, LogLevel::Info,
     ("%p IMEInputHandler::SetMarkedText, "
      "aAttrString=\"%s\", aSelectedRange={ location=%llu, length=%llu }, "
      "aReplacementRange=%p { location=%llu, length=%llu }, "
      "Destroyed()=%s, IgnoreIMEComposition()=%s, IsIMEComposing()=%s, "
      "mMarkedRange={ location=%llu, length=%llu }, keyevent=%p, "
-     "keydownHandled=%s, keypressDispatched=%s, causedOtherKeyEvents=%s",
+     "keydownHandled=%s, keypressDispatched=%s, causedOtherKeyEvents=%s, "
+     "compositionDispatched=%s",
      this, GetCharacters([aAttrString string]),
      aSelectedRange.location, aSelectedRange.length, aReplacementRange,
      aReplacementRange ? aReplacementRange->location : 0,
      aReplacementRange ? aReplacementRange->length : 0,
      TrueOrFalse(Destroyed()), TrueOrFalse(IgnoreIMEComposition()),
      TrueOrFalse(IsIMEComposing()),
      mMarkedRange.location, mMarkedRange.length,
      currentKeyEvent ? currentKeyEvent->mKeyEvent : nullptr,
      currentKeyEvent ?
        TrueOrFalse(currentKeyEvent->mKeyDownHandled) : "N/A",
      currentKeyEvent ?
        TrueOrFalse(currentKeyEvent->mKeyPressDispatched) : "N/A",
      currentKeyEvent ?
-       TrueOrFalse(currentKeyEvent->mCausedOtherKeyEvents) : "N/A"));
-
-  // If SetMarkedText() is called during composition, that means that
+       TrueOrFalse(currentKeyEvent->mCausedOtherKeyEvents) : "N/A",
+     currentKeyEvent ?
+       TrueOrFalse(currentKeyEvent->mCompositionDispatched) : "N/A"));
+
+  // If SetMarkedText() is called during handling a key press, that means that
   // the key event caused this composition.  So, keypress event shouldn't
-  // be dispatched later, let's consume it here.
+  // be dispatched later, let's mark the key event causing composition event.
   if (currentKeyEvent) {
-    currentKeyEvent->mKeyPressHandled = true;
-    currentKeyEvent->mKeyPressDispatched = true;
+    currentKeyEvent->mCompositionDispatched = true;
   }
 
   if (Destroyed() || IgnoreIMEComposition()) {
     return;
   }
 
   RefPtr<IMEInputHandler> kungFuDeathGrip(this);