Bug 1305355 part.2 TSFTextStore shouldn't append any ranges when composition string is empty r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 26 Sep 2016 17:12:34 +0900
changeset 315394 fa9da7230277a57e27c6602c85723dddad457d88
parent 315393 f4cedc85d48d549e33bd1bec7093606f028f97d0
child 315395 fb4fd0ff521501151f43eb1dc07294bbc88f8130
push id30748
push usercbook@mozilla.com
push dateWed, 28 Sep 2016 13:53:19 +0000
treeherdermozilla-central@8c84b7618840 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1305355
milestone52.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 1305355 part.2 TSFTextStore shouldn't append any ranges when composition string is empty r=m_kato TSFTextStore shouldn't append any ranges to dispatch empty composition event since empty clause information may make TextComposition confused. Additionally, it doesn't need to append caret range because CompositionTransaction always sets caret at start of composition when the composition string is empty. MozReview-Commit-ID: Iu8IWwEOxaf
widget/windows/TSFTextStore.cpp
--- a/widget/windows/TSFTextStore.cpp
+++ b/widget/windows/TSFTextStore.cpp
@@ -2490,21 +2490,25 @@ TSFTextStore::RestartComposition(ITfComp
   }
   contentForTSF.ReplaceTextWith(mComposition.mStart,
                                 mComposition.mString.Length(),
                                 commitString);
   // Record a compositionupdate action for commit the part of composing string.
   PendingAction* action = LastOrNewPendingCompositionUpdate();
   action->mData = mComposition.mString;
   action->mRanges->Clear();
-  TextRange caretRange;
-  caretRange.mStartOffset = caretRange.mEndOffset =
-    uint32_t(oldComposition.mStart + commitString.Length());
-  caretRange.mRangeType = TextRangeType::eCaret;
-  action->mRanges->AppendElement(caretRange);
+  // Note that we shouldn't append ranges when composition string
+  // is empty because it may cause TextComposition confused.
+  if (!action->mData.IsEmpty()) {
+    TextRange caretRange;
+    caretRange.mStartOffset = caretRange.mEndOffset =
+      uint32_t(oldComposition.mStart + commitString.Length());
+    caretRange.mRangeType = TextRangeType::eCaret;
+    action->mRanges->AppendElement(caretRange);
+  }
   action->mIncomplete = false;
 
   // Record compositionend action.
   RecordCompositionEndAction();
 
   // Record compositionstart action only with the new start since this method
   // hasn't restored composing string yet.
   RecordCompositionStartAction(aCompositionView, newStart, 0, false);
@@ -2637,138 +2641,145 @@ TSFTextStore::RecordCompositionUpdateAct
 
   PendingAction* action = LastOrNewPendingCompositionUpdate();
   action->mData = mComposition.mString;
   // The ranges might already have been initialized, however, if this is
   // called again, that means we need to overwrite the ranges with current
   // information.
   action->mRanges->Clear();
 
-  TextRange newRange;
-  // No matter if we have display attribute info or not,
-  // we always pass in at least one range to eCompositionChange
-  newRange.mStartOffset = 0;
-  newRange.mEndOffset = action->mData.Length();
-  newRange.mRangeType = TextRangeType::eRawClause;
-  action->mRanges->AppendElement(newRange);
-
-  RefPtr<ITfRange> range;
-  while (S_OK == enumRanges->Next(1, getter_AddRefs(range), nullptr) && range) {
-
-    LONG rangeStart = 0, rangeLength = 0;
-    if (FAILED(GetRangeExtent(range, &rangeStart, &rangeLength))) {
-      continue;
-    }
-    // The range may include out of composition string.  We should ignore
-    // outside of the composition string.
-    LONG start = std::min(std::max(rangeStart, mComposition.mStart),
-                          mComposition.EndOffset());
-    LONG end = std::max(std::min(rangeStart + rangeLength,
-                                 mComposition.EndOffset()),
-                        mComposition.mStart);
-    LONG length = end - start;
-    if (length < 0) {
-      MOZ_LOG(sTextStoreLog, LogLevel::Error,
-        ("0x%p   TSFTextStore::RecordCompositionUpdateAction() "
-         "ignores invalid range (%d-%d)",
-         this, rangeStart - mComposition.mStart,
-         rangeStart - mComposition.mStart + rangeLength));
-      continue;
-    }
-    if (!length) {
-      MOZ_LOG(sTextStoreLog, LogLevel::Debug,
-        ("0x%p   TSFTextStore::RecordCompositionUpdateAction() "
-         "ignores a range due to outside of the composition or empty "
-         "(%d-%d)",
-         this, rangeStart - mComposition.mStart,
-         rangeStart - mComposition.mStart + rangeLength));
-      continue;
-    }
-
+  // Note that we shouldn't append ranges when composition string
+  // is empty because it may cause TextComposition confused.
+  if (!action->mData.IsEmpty()) {
     TextRange newRange;
-    newRange.mStartOffset = uint32_t(start - mComposition.mStart);
-    // The end of the last range in the array is
-    // always kept at the end of composition
-    newRange.mEndOffset = mComposition.mString.Length();
-
-    TF_DISPLAYATTRIBUTE attr;
-    hr = GetDisplayAttribute(attrPropetry, range, &attr);
-    if (FAILED(hr)) {
-      newRange.mRangeType = TextRangeType::eRawClause;
-    } else {
-      newRange.mRangeType = GetGeckoSelectionValue(attr);
-      if (GetColor(attr.crText, newRange.mRangeStyle.mForegroundColor)) {
-        newRange.mRangeStyle.mDefinedStyles |=
-                               TextRangeStyle::DEFINED_FOREGROUND_COLOR;
+    // No matter if we have display attribute info or not,
+    // we always pass in at least one range to eCompositionChange
+    newRange.mStartOffset = 0;
+    newRange.mEndOffset = action->mData.Length();
+    newRange.mRangeType = TextRangeType::eRawClause;
+    action->mRanges->AppendElement(newRange);
+
+    RefPtr<ITfRange> range;
+    while (enumRanges->Next(1, getter_AddRefs(range), nullptr) == S_OK) {
+      if (NS_WARN_IF(!range)) {
+        break;
+      }
+
+      LONG rangeStart = 0, rangeLength = 0;
+      if (FAILED(GetRangeExtent(range, &rangeStart, &rangeLength))) {
+        continue;
+      }
+      // The range may include out of composition string.  We should ignore
+      // outside of the composition string.
+      LONG start = std::min(std::max(rangeStart, mComposition.mStart),
+                            mComposition.EndOffset());
+      LONG end = std::max(std::min(rangeStart + rangeLength,
+                                   mComposition.EndOffset()),
+                          mComposition.mStart);
+      LONG length = end - start;
+      if (length < 0) {
+        MOZ_LOG(sTextStoreLog, LogLevel::Error,
+          ("0x%p   TSFTextStore::RecordCompositionUpdateAction() "
+           "ignores invalid range (%d-%d)",
+           this, rangeStart - mComposition.mStart,
+           rangeStart - mComposition.mStart + rangeLength));
+        continue;
       }
-      if (GetColor(attr.crBk, newRange.mRangeStyle.mBackgroundColor)) {
-        newRange.mRangeStyle.mDefinedStyles |=
-                               TextRangeStyle::DEFINED_BACKGROUND_COLOR;
+      if (!length) {
+        MOZ_LOG(sTextStoreLog, LogLevel::Debug,
+          ("0x%p   TSFTextStore::RecordCompositionUpdateAction() "
+           "ignores a range due to outside of the composition or empty "
+           "(%d-%d)",
+           this, rangeStart - mComposition.mStart,
+           rangeStart - mComposition.mStart + rangeLength));
+        continue;
       }
-      if (GetColor(attr.crLine, newRange.mRangeStyle.mUnderlineColor)) {
-        newRange.mRangeStyle.mDefinedStyles |=
-                               TextRangeStyle::DEFINED_UNDERLINE_COLOR;
+
+      TextRange newRange;
+      newRange.mStartOffset = uint32_t(start - mComposition.mStart);
+      // The end of the last range in the array is
+      // always kept at the end of composition
+      newRange.mEndOffset = mComposition.mString.Length();
+
+      TF_DISPLAYATTRIBUTE attr;
+      hr = GetDisplayAttribute(attrPropetry, range, &attr);
+      if (FAILED(hr)) {
+        newRange.mRangeType = TextRangeType::eRawClause;
+      } else {
+        newRange.mRangeType = GetGeckoSelectionValue(attr);
+        if (GetColor(attr.crText, newRange.mRangeStyle.mForegroundColor)) {
+          newRange.mRangeStyle.mDefinedStyles |=
+                                 TextRangeStyle::DEFINED_FOREGROUND_COLOR;
+        }
+        if (GetColor(attr.crBk, newRange.mRangeStyle.mBackgroundColor)) {
+          newRange.mRangeStyle.mDefinedStyles |=
+                                 TextRangeStyle::DEFINED_BACKGROUND_COLOR;
+        }
+        if (GetColor(attr.crLine, newRange.mRangeStyle.mUnderlineColor)) {
+          newRange.mRangeStyle.mDefinedStyles |=
+                                 TextRangeStyle::DEFINED_UNDERLINE_COLOR;
+        }
+        if (GetLineStyle(attr.lsStyle, newRange.mRangeStyle.mLineStyle)) {
+          newRange.mRangeStyle.mDefinedStyles |=
+                                 TextRangeStyle::DEFINED_LINESTYLE;
+          newRange.mRangeStyle.mIsBoldLine = attr.fBoldLine != 0;
+        }
       }
-      if (GetLineStyle(attr.lsStyle, newRange.mRangeStyle.mLineStyle)) {
-        newRange.mRangeStyle.mDefinedStyles |=
-                               TextRangeStyle::DEFINED_LINESTYLE;
-        newRange.mRangeStyle.mIsBoldLine = attr.fBoldLine != 0;
+
+      TextRange& lastRange = action->mRanges->LastElement();
+      if (lastRange.mStartOffset == newRange.mStartOffset) {
+        // Replace range if last range is the same as this one
+        // So that ranges don't overlap and confuse the editor
+        lastRange = newRange;
+      } else {
+        lastRange.mEndOffset = newRange.mStartOffset;
+        action->mRanges->AppendElement(newRange);
       }
     }
 
-    TextRange& lastRange = action->mRanges->LastElement();
-    if (lastRange.mStartOffset == newRange.mStartOffset) {
-      // Replace range if last range is the same as this one
-      // So that ranges don't overlap and confuse the editor
-      lastRange = newRange;
-    } else {
-      lastRange.mEndOffset = newRange.mStartOffset;
-      action->mRanges->AppendElement(newRange);
+    // We need to hack for Korean Input System which is Korean standard TIP.
+    // It sets no change style to IME selection (the selection is always only
+    // one).  So, the composition string looks like normal (or committed)
+    // string.  At this time, current selection range is same as the
+    // composition string range.  Other applications set a wide caret which
+    // covers the composition string,  however, Gecko doesn't support the wide
+    // caret drawing now (Gecko doesn't support XOR drawing), unfortunately.
+    // For now, we should change the range style to undefined.
+    if (!selectionForTSF.IsCollapsed() && action->mRanges->Length() == 1) {
+      TextRange& range = action->mRanges->ElementAt(0);
+      LONG start = selectionForTSF.MinOffset();
+      LONG end = selectionForTSF.MaxOffset();
+      if ((LONG)range.mStartOffset == start - mComposition.mStart &&
+          (LONG)range.mEndOffset == end - mComposition.mStart &&
+          range.mRangeStyle.IsNoChangeStyle()) {
+        range.mRangeStyle.Clear();
+        // The looks of selected type is better than others.
+        range.mRangeType = TextRangeType::eSelectedRawClause;
+      }
     }
-  }
-
-  // We need to hack for Korean Input System which is Korean standard TIP.
-  // It sets no change style to IME selection (the selection is always only
-  // one).  So, the composition string looks like normal (or committed) string.
-  // At this time, current selection range is same as the composition string
-  // range.  Other applications set a wide caret which covers the composition
-  // string,  however, Gecko doesn't support the wide caret drawing now (Gecko
-  // doesn't support XOR drawing), unfortunately.  For now, we should change
-  // the range style to undefined.
-  if (!selectionForTSF.IsCollapsed() && action->mRanges->Length() == 1) {
-    TextRange& range = action->mRanges->ElementAt(0);
-    LONG start = selectionForTSF.MinOffset();
-    LONG end = selectionForTSF.MaxOffset();
-    if ((LONG)range.mStartOffset == start - mComposition.mStart &&
-        (LONG)range.mEndOffset == end - mComposition.mStart &&
-        range.mRangeStyle.IsNoChangeStyle()) {
-      range.mRangeStyle.Clear();
-      // The looks of selected type is better than others.
-      range.mRangeType = TextRangeType::eSelectedRawClause;
+
+    // The caret position has to be collapsed.
+    uint32_t caretPosition =
+      static_cast<uint32_t>(selectionForTSF.MaxOffset() - mComposition.mStart);
+
+    // If caret is in the target clause and it doesn't have specific style,
+    // the target clause will be painted as normal selection range.  Since
+    // caret shouldn't be in selection range on Windows, we shouldn't append
+    // caret range in such case.
+    const TextRange* targetClause = action->mRanges->GetTargetClause();
+    if (!targetClause || targetClause->mRangeStyle.IsDefined() ||
+        caretPosition < targetClause->mStartOffset ||
+        caretPosition > targetClause->mEndOffset) {
+      TextRange caretRange;
+      caretRange.mStartOffset = caretRange.mEndOffset = caretPosition;
+      caretRange.mRangeType = TextRangeType::eCaret;
+      action->mRanges->AppendElement(caretRange);
     }
   }
 
-  // The caret position has to be collapsed.
-  uint32_t caretPosition =
-    static_cast<uint32_t>(selectionForTSF.MaxOffset() - mComposition.mStart);
-
-  // If caret is in the target clause and it doesn't have specific style,
-  // the target clause will be painted as normal selection range.  Since caret
-  // shouldn't be in selection range on Windows, we shouldn't append caret
-  // range in such case.
-  const TextRange* targetClause = action->mRanges->GetTargetClause();
-  if (!targetClause || targetClause->mRangeStyle.IsDefined() ||
-      caretPosition < targetClause->mStartOffset ||
-      caretPosition > targetClause->mEndOffset) {
-    TextRange caretRange;
-    caretRange.mStartOffset = caretRange.mEndOffset = caretPosition;
-    caretRange.mRangeType = TextRangeType::eCaret;
-    action->mRanges->AppendElement(caretRange);
-  }
-
   action->mIncomplete = false;
 
   MOZ_LOG(sTextStoreLog, LogLevel::Info,
     ("0x%p   TSFTextStore::RecordCompositionUpdateAction() "
      "succeeded", this));
 
   return S_OK;
 }