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
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 17 Nov 2016 13:35:21 +0900
changeset 441153 b9a918aca10c791c8cfedfa2d759b3505a44c666
parent 441152 79ec544204fc005bef2bfff1b51fbf0be4bcab3f
child 441154 ec7fb4f14d3ec23ded7eea40ff49ebbcbec6bde1
push id36375
push userbmo:rchien@mozilla.com
push dateFri, 18 Nov 2016 16:08:18 +0000
reviewersm_kato
bugs1317906
milestone53.0a1
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 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);