Bug 1475153 - Make TSFTextStore::RecordCompositionStartAction() merge new composition with previous composition if IME commits composition and restart composition to replace the previous commit string r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 12 Jul 2018 22:40:07 +0900
changeset 817378 18f93e25cb951cf3b4fef887790d0b51c1cfa1be
parent 817242 68bf4fbea7a7b56cdbc31df31aaaed092d73161a
push id116048
push usermasayuki@d-toybox.com
push dateThu, 12 Jul 2018 15:55:48 +0000
reviewersm_kato
bugs1475153, 1208043
milestone63.0a1
Bug 1475153 - Make TSFTextStore::RecordCompositionStartAction() merge new composition with previous composition if IME commits composition and restart composition to replace the previous commit string r?m_kato When user removes all composition string of MS Pinyin, MS Wubi, MS ChangJie and MS Quick with Backspace key, IME commits last character temporarily and restart composition to replace the last character with empty string when user tries to remove last one character. This causes flicking composition string because the additional composition selects the character and it may be painted immediately before removed, and also editor will have unnecessary undo transaction. Therefore, as same as bug 1208043, TSFTextStore::RecordCompositionStartAction() should restart last composition in such case. Fortunately, we implemented similar code for bug 1208043, however, unfortunately, we don't have preceding pending compositionstart in this case. Therefore, this patch makes pending compositionend store start offset of composition. Then, we can restart composition only with information stored by pending compositionend action. Additionally, this patch renames the method checking pending actions for self-describing the new meaning. MozReview-Commit-ID: 1RyuacxEbky
widget/windows/TSFTextStore.cpp
widget/windows/TSFTextStore.h
--- a/widget/windows/TSFTextStore.cpp
+++ b/widget/windows/TSFTextStore.cpp
@@ -5036,27 +5036,29 @@ TSFTextStore::InsertTextAtSelectionInter
     compositionStart->mSelectionStart = oldSelection.acpStart;
     compositionStart->mSelectionLength =
       oldSelection.acpEnd - oldSelection.acpStart;
     compositionStart->mAdjustSelection = false;
 
     PendingAction* compositionEnd = mPendingActions.AppendElement();
     compositionEnd->mType = PendingAction::Type::eCompositionEnd;
     compositionEnd->mData = aInsertStr;
+    compositionEnd->mSelectionStart = compositionStart->mSelectionStart;
 
     MOZ_LOG(sTextStoreLog, LogLevel::Debug,
       ("0x%p   TSFTextStore::InsertTextAtSelectionInternal() "
        "appending pending compositionstart and compositionend... "
        "PendingCompositionStart={ mSelectionStart=%d, "
        "mSelectionLength=%d }, PendingCompositionEnd={ mData=\"%s\" "
-       "(Length()=%u) }",
+       "(Length()=%u), mSelectionStart=%d }",
        this, compositionStart->mSelectionStart,
        compositionStart->mSelectionLength,
        GetEscapedUTF8String(compositionEnd->mData).get(),
-       compositionEnd->mData.Length()));
+       compositionEnd->mData.Length(),
+       compositionEnd->mSelectionStart));
   }
 
   contentForTSF.ReplaceSelectedTextWith(aInsertStr);
 
   if (aTextChange) {
     aTextChange->acpStart = oldSelection.acpStart;
     aTextChange->acpOldEnd = oldSelection.acpEnd;
     aTextChange->acpNewEnd = contentForTSF.Selection().EndOffset();
@@ -5141,28 +5143,32 @@ TSFTextStore::RecordCompositionStartActi
       ("0x%p   TSFTextStore::RecordCompositionStartAction() FAILED due to "
        "destroyed during dispatching a keyboard event", this));
     return false;
   }
 
   CompleteLastActionIfStillIncomplete();
 
   // TIP may have inserted text at selection before calling
-  // OnStartComposition().  In this case, we've already created a pair of
-  // pending compositionstart and pending compositionend.  If the pending
-  // compositionstart occurred same range as this composition, it was the
-  // start of this composition.  In such case, we should cancel the pending
-  // compositionend and start composition normally.
+  // OnStartComposition().  In this case, we've already created a pending
+  // compositionend.  If new composition replaces all commit string of the
+  // pending compositionend, we should cancel the pending compositionend and
+  // keep the previous composition normally.
+  // On Windows 7, MS-IME for Korean, MS-IME 2010 for Korean and MS Old Hangul
+  // may start composition with calling InsertTextAtSelection() and
+  // OnStartComposition() with this order (bug 1208043).
+  // On Windows 10, MS Pinyin, MS Wubi, MS ChangJie and MS Quick commits
+  // last character and replace it with empty string with new composition
+  // when user removes last character of composition string with Backspace
+  // key (bug 1462257).
   if (!aPreserveSelection &&
-      WasTextInsertedWithoutCompositionAt(aStart, aLength)) {
+      IsLastPendingActionCompositionEndAt(aStart, aLength)) {
     const PendingAction& pendingCompositionEnd = mPendingActions.LastElement();
-    const PendingAction& pendingCompositionStart =
-      mPendingActions[mPendingActions.Length() - 2];
-    contentForTSF.RestoreCommittedComposition(
-      aComposition, pendingCompositionStart, pendingCompositionEnd);
+    contentForTSF.RestoreCommittedComposition(aComposition,
+                                              pendingCompositionEnd);
     mPendingActions.RemoveLastElement();
     MOZ_LOG(sTextStoreLog, LogLevel::Info,
       ("0x%p   TSFTextStore::RecordCompositionStartAction() "
        "succeeded: restoring the committed string as composing string, "
        "mComposition={ mStart=%ld, mString.Length()=%ld, "
        "mSelectionForTSF={ acpStart=%ld, acpEnd=%ld, style.ase=%s, "
        "style.fInterimChar=%s } }",
        this, mComposition.mStart, mComposition.mString.Length(),
@@ -5234,16 +5240,17 @@ TSFTextStore::RecordCompositionEndAction
   // composition update, we can forget them since composition end will send
   // the latest composition string and it overwrites the composition string
   // even if we dispatch eCompositionChange event before that.  So, let's
   // forget all composition updates now.
   RemoveLastCompositionUpdateActions();
   PendingAction* action = mPendingActions.AppendElement();
   action->mType = PendingAction::Type::eCompositionEnd;
   action->mData = mComposition.mString;
+  action->mSelectionStart = mComposition.mStart;
 
   Content& contentForTSF = ContentForTSFRef();
   if (!contentForTSF.IsInitialized()) {
     MOZ_LOG(sTextStoreLog, LogLevel::Error,
       ("0x%p   TSFTextStore::RecordCompositionEndAction() FAILED due "
        "to ContentForTSFRef() failure", this));
     return E_FAIL;
   }
@@ -7145,34 +7152,31 @@ TSFTextStore::Content::StartComposition(
     mSelection.SetSelection(mComposition.mStart, mComposition.mString.Length(),
                             false, mSelection.GetWritingMode());
   }
 }
 
 void
 TSFTextStore::Content::RestoreCommittedComposition(
                          ITfCompositionView* aCompositionView,
-                         const PendingAction& aPendingCompositionStart,
                          const PendingAction& aCanceledCompositionEnd)
 {
   MOZ_ASSERT(mInitialized);
   MOZ_ASSERT(aCompositionView);
   MOZ_ASSERT(!mComposition.mView);
-  MOZ_ASSERT(aPendingCompositionStart.mType ==
-               PendingAction::Type::eCompositionStart);
   MOZ_ASSERT(aCanceledCompositionEnd.mType ==
                PendingAction::Type::eCompositionEnd);
   MOZ_ASSERT(GetSubstring(
-               static_cast<uint32_t>(aPendingCompositionStart.mSelectionStart),
+               static_cast<uint32_t>(aCanceledCompositionEnd.mSelectionStart),
                static_cast<uint32_t>(aCanceledCompositionEnd.mData.Length())) ==
                aCanceledCompositionEnd.mData);
 
   // Restore the committed string as composing string.
   mComposition.Start(aCompositionView,
-                     aPendingCompositionStart.mSelectionStart,
+                     aCanceledCompositionEnd.mSelectionStart,
                      aCanceledCompositionEnd.mData);
   mLatestCompositionStartOffset = mComposition.mStart;
   mLatestCompositionEndOffset = mComposition.EndOffset();
 }
 
 void
 TSFTextStore::Content::EndComposition(const PendingAction& aCompEnd)
 {
--- a/widget/windows/TSFTextStore.h
+++ b/widget/windows/TSFTextStore.h
@@ -695,18 +695,19 @@ protected:
     {
       eCompositionStart,
       eCompositionUpdate,
       eCompositionEnd,
       eSetSelection,
       eKeyboardEvent,
     };
     Type mType;
+    // For eCompositionStart, eCompositionEnd and eSetSelection
+    LONG mSelectionStart;
     // For eCompositionStart and eSetSelection
-    LONG mSelectionStart;
     LONG mSelectionLength;
     // For eCompositionStart, eCompositionUpdate and eCompositionEnd
     nsString mData;
     // For eCompositionUpdate
     RefPtr<TextRangeArray> mRanges;
     // For eKeyboardEvent
     const MSG* mKeyMsg;
     // For eSetSelection
@@ -733,40 +734,37 @@ protected:
     PendingAction* newAction = mPendingActions.AppendElement();
     newAction->mType = PendingAction::Type::eCompositionUpdate;
     newAction->mRanges = new TextRangeArray();
     newAction->mIncomplete = true;
     return newAction;
   }
 
   /**
-   * WasTextInsertedWithoutCompositionAt() checks if text was inserted without
-   * composition immediately before (e.g., see InsertTextAtSelectionInternal()).
+   * IsLastPendingActionCompositionEndAt() checks whether the previous pending
+   * action is committing composition whose range starts from aStart and its
+   * length is aLength.  In other words, this checks whether new composition
+   * which will replace same range as previous pending commit can be merged
+   * with the previous composition.
    *
    * @param aStart              The inserted offset you expected.
    * @param aLength             The inserted text length you expected.
-   * @return                    true if the last pending actions are
-   *                            eCompositionStart and eCompositionEnd and
-   *                            aStart and aLength match their information.
+   * @return                    true if the last pending action is
+   *                            eCompositionEnd and it inserted the text
+   *                            between aStart and aStart + aLength.
    */
-  bool WasTextInsertedWithoutCompositionAt(LONG aStart, LONG aLength) const
+  bool IsLastPendingActionCompositionEndAt(LONG aStart, LONG aLength) const
   {
-    if (mPendingActions.Length() < 2) {
+    if (mPendingActions.IsEmpty()) {
       return false;
     }
     const PendingAction& pendingLastAction = mPendingActions.LastElement();
-    if (pendingLastAction.mType != PendingAction::Type::eCompositionEnd ||
-        pendingLastAction.mData.Length() != ULONG(aLength)) {
-      return false;
-    }
-    const PendingAction& pendingPreLastAction =
-      mPendingActions[mPendingActions.Length() - 2];
-    return pendingPreLastAction.mType ==
-             PendingAction::Type::eCompositionStart &&
-           pendingPreLastAction.mSelectionStart == aStart;
+    return pendingLastAction.mType == PendingAction::Type::eCompositionEnd &&
+           pendingLastAction.mSelectionStart == aStart &&
+           pendingLastAction.mData.Length() == static_cast<ULONG>(aLength);
   }
 
   bool IsPendingCompositionUpdateIncomplete() const
   {
     if (mPendingActions.IsEmpty()) {
       return false;
     }
     const PendingAction& lastAction = mPendingActions.LastElement();
@@ -887,29 +885,27 @@ protected:
     void ReplaceTextWith(LONG aStart, LONG aLength, const nsAString& aString);
 
     void StartComposition(ITfCompositionView* aCompositionView,
                           const PendingAction& aCompStart,
                           bool aPreserveSelection);
     /**
      * RestoreCommittedComposition() restores the committed string as
      * composing string.  If InsertTextAtSelection() or something is called
-     * before a call of OnStartComposition(), there is a pending
-     * compositionstart and a pending compositionend.  In this case, we
-     * need to cancel the pending compositionend and continue the composition.
+     * before a call of OnStartComposition() or previous composition is
+     * committed and new composition is restarted to clean up the commited
+     * string, there is a pending compositionend.  In this case, we need to
+     * cancel the pending compositionend and continue the composition.
      *
      * @param aCompositionView          The composition view.
-     * @param aPendingCompositionStart  The pending compositionstart which
-     *                                  started the committed composition.
      * @param aCanceledCompositionEnd   The pending compositionend which is
      *                                  canceled for restarting the composition.
      */
     void RestoreCommittedComposition(
                          ITfCompositionView* aCompositionView,
-                         const PendingAction& aPendingCompositionStart,
                          const PendingAction& aCanceledCompositionEnd);
     void EndComposition(const PendingAction& aCompEnd);
 
     const nsString& Text() const
     {
       MOZ_ASSERT(mInitialized);
       return mText;
     }