Bug 1422230 - part 4: TSFTextStore::GetTextExt() should refer composition string in content r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 12 Jan 2018 15:31:04 +0900
changeset 399270 3700de32eedbe6670085841c3a36fe091f7a1543
parent 399269 d02c066d501be954cd60ea4b20b5ed291cd6b938
child 399271 93fa34d5492cfaa900f3eb109cde76ef1c5f5917
push id33256
push userncsoregi@mozilla.com
push dateMon, 15 Jan 2018 17:25:55 +0000
treeherdermozilla-central@a88bb54f50e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1422230
milestone59.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 1422230 - part 4: TSFTextStore::GetTextExt() should refer composition string in content r=m_kato Currently, TSFTextStore::GetTextExt() refers mComposition for doing its own hack. However, this means that it refers composition in TIP. However, query event is computed with content information. So, even if TSFTextStore dispatched eCompositionCommit event, it may not be handled by content yet. In this case, we need information relative to last composition string. So, TSFTextStore::GetTextExt() should refer IsHandlingComposition() and last composition string information stored by mContentForTSF. MozReview-Commit-ID: KMqrDmnUldU
widget/windows/TSFTextStore.cpp
--- a/widget/windows/TSFTextStore.cpp
+++ b/widget/windows/TSFTextStore.cpp
@@ -4275,24 +4275,41 @@ TSFTextStore::GetTextExt(TsViewCookie vc
     MOZ_LOG(sTextStoreLog, LogLevel::Error,
       ("0x%p   TSFTextStore::GetTextExt() FAILED due to "
        "invalid position", this));
     return TS_E_INVALIDPOS;
   }
 
   mWaitingQueryLayout = false;
 
-  // NOTE: TSF (at least on Win 8.1) doesn't return TS_E_NOLAYOUT to the
-  // caller even if we return it.  It's converted to just E_FAIL.
-  // However, this is fixed on Win 10.
+  // When ITextStoreACP::GetTextExt() returns TS_E_NOLAYOUT, TSF returns E_FAIL
+  // to its caller (typically, active TIP).  Then, most TIPs abort current job
+  // or treat such application as non-GUI apps.  E.g., some of them give up
+  // showing candidate window, some others show candidate window at top-left of
+  // the screen.  For avoiding this issue, when there is composition (until
+  // composition is actually committed in remote content), we should not
+  // return TS_E_NOLAYOUT error for TIPs whose some features are broken by
+  // this issue.
+  // Note that ideally, this issue should be avoided by each TIP since this
+  // won't be fixed at least on non-latest Windows.  Actually, Google Japanese
+  // Input (based on Mozc) does it.  When GetTextExt() returns E_FAIL, TIPs
+  // should try to check result of GetRangeFromPoint() because TSF returns
+  // TS_E_NOLAYOUT correctly in this case. See:
+  // https://github.com/google/mozc/blob/6b878e31fb6ac4347dc9dfd8ccc1080fe718479f/src/win32/tip/tip_range_util.cc#L237-L257
 
   bool dontReturnNoLayoutError = false;
 
-  if (mComposition.IsComposing() && mComposition.mStart < acpEnd &&
+  if (IsHandlingComposition() && mContentForTSF.HasOrHadComposition() &&
       mContentForTSF.IsLayoutChangedAt(acpEnd)) {
+    MOZ_ASSERT(!mComposition.IsComposing() ||
+               mComposition.mStart ==
+                 mContentForTSF.LatestCompositionStartOffset());
+    MOZ_ASSERT(!mComposition.IsComposing() ||
+               mComposition.EndOffset() ==
+                 mContentForTSF.LatestCompositionEndOffset());
     const Selection& selectionForTSF = SelectionForTSFRef();
     // The bug of Microsoft Office IME 2010 for Japanese is similar to
     // MS-IME for Win 8.1 and Win 10.  Newer version of MS Office IME is not
     // released yet.  So, we can hack it without prefs  because there must be
     // no developers who want to disable this hack for tests.
     const bool kIsMSOfficeJapaneseIME2010 =
       TSFStaticSink::IsMSOfficeJapaneseIME2010Active();
     // MS IME for Japanese doesn't support asynchronous handling at deciding
@@ -4345,40 +4362,39 @@ TSFTextStore::GetTextExt(TsViewCookie vc
     //     refers native caret rect on windows whose window class is one of
     //     Mozilla window classes and we stop creating native caret for ATOK
     //     because creating native caret causes ATOK refers caret position
     //     when GetTextExt() returns TS_E_NOLAYOUT.
     else if (TSFPrefs::DoNotReturnNoLayoutErrorToATOKOfCompositionString() &&
              TSFStaticSink::IsATOKActive() &&
              (!TSFStaticSink::IsATOKReferringNativeCaretActive() ||
               !TSFPrefs::NeedToCreateNativeCaretForLegacyATOK()) &&
-             mComposition.mStart == acpStart &&
-             mComposition.EndOffset() == acpEnd) {
+             mContentForTSF.LatestCompositionStartOffset() == acpStart &&
+             mContentForTSF.LatestCompositionEndOffset() == acpEnd) {
       dontReturnNoLayoutError = true;
     }
     // Free ChangJie 2010 doesn't handle ITfContextView::GetTextExt() properly.
     // Prehaps, it's due to the bug of TSF.  We need to check if this is
     // necessary on Windows 10 before disabling this on Windows 10.
     else if (TSFPrefs::DoNotReturnNoLayoutErrorToFreeChangJie() &&
              TSFStaticSink::IsFreeChangJieActive()) {
-      acpEnd = mComposition.mStart;
+      acpEnd = mContentForTSF.LatestCompositionStartOffset();
       acpStart = std::min(acpStart, acpEnd);
       dontReturnNoLayoutError = true;
     }
     // Some Chinese TIPs of Microsoft doesn't show candidate window in e10s
     // mode on Win8 or later.
-    else if (
-      IsWin8OrLater() &&
-      ((TSFPrefs::DoNotReturnNoLayoutErrorToMSTraditionalTIP() &&
-        (TSFStaticSink::IsMSChangJieActive() ||
-         TSFStaticSink::IsMSQuickActive())) ||
-       (TSFPrefs::DoNotReturnNoLayoutErrorToMSSimplifiedTIP() &&
-         (TSFStaticSink::IsMSPinyinActive() ||
-          TSFStaticSink::IsMSWubiActive())))) {
-      acpEnd = mComposition.mStart;
+    else if (IsWin8OrLater() &&
+             ((TSFPrefs::DoNotReturnNoLayoutErrorToMSTraditionalTIP() &&
+               (TSFStaticSink::IsMSChangJieActive() ||
+                TSFStaticSink::IsMSQuickActive())) ||
+             (TSFPrefs::DoNotReturnNoLayoutErrorToMSSimplifiedTIP() &&
+               (TSFStaticSink::IsMSPinyinActive() ||
+                TSFStaticSink::IsMSWubiActive())))) {
+      acpEnd = mContentForTSF.LatestCompositionStartOffset();
       acpStart = std::min(acpStart, acpEnd);
       dontReturnNoLayoutError = true;
     }
 
     // If we hack the queried range for active TIP, that means we should not
     // return TS_E_NOLAYOUT even if hacked offset is still modified.  So, as
     // far as possible, we should adjust the offset.
     if (dontReturnNoLayoutError) {
@@ -4396,18 +4412,18 @@ TSFTextStore::GetTextExt(TsViewCookie vc
       // useful because it may return caret rect or old character's rect which
       // the user still see.  That must be useful information for TIP.
       int32_t firstModifiedOffset =
         static_cast<int32_t>(mContentForTSF.MinOffsetOfLayoutChanged());
       LONG lastUnmodifiedOffset = std::max(firstModifiedOffset - 1, 0);
       if (mContentForTSF.IsLayoutChangedAt(acpStart)) {
         // If TSF queries text rect in composition string, we should return
         // rect at start of the composition even if its layout is changed.
-        if (acpStart >= mComposition.mStart) {
-          acpStart = mComposition.mStart;
+        if (acpStart >= mContentForTSF.LatestCompositionStartOffset()) {
+          acpStart = mContentForTSF.LatestCompositionStartOffset();
         }
         // Otherwise, use first character's rect.  Even if there is no
         // characters, the query event will return caret rect instead.
         else {
           acpStart = lastUnmodifiedOffset;
         }
         MOZ_ASSERT(acpStart <= acpEnd);
       }
@@ -4448,16 +4464,22 @@ TSFTextStore::GetTextExt(TsViewCookie vc
   int64_t startOffset = acpStart;
   if (mComposition.IsComposing()) {
     // If there is a composition, TSF must want character rects related to
     // the composition.  Therefore, we should use insertion point relative
     // query because the composition might be at different position from
     // the position where TSFTextStore believes it at.
     options.mRelativeToInsertionPoint = true;
     startOffset -= mComposition.mStart;
+  } else if (IsHandlingComposition() && mContentForTSF.HasOrHadComposition()) {
+    // If there was a composition and it hasn't been committed in the content
+    // yet, ContentCacheInParent is still open for relative offset query from
+    // the latest composition.
+    options.mRelativeToInsertionPoint = true;
+    startOffset -= mContentForTSF.LatestCompositionStartOffset();
   } else if (!CanAccessActualContentDirectly()) {
     // If TSF/TIP cannot access actual content directly, there may be pending
     // text and/or selection changes which have not been notified TSF yet.
     // Therefore, we should use relative to insertion point query since
     // TSF/TIP computes the offset from the cached selection.
     options.mRelativeToInsertionPoint = true;
     startOffset -= mSelectionForTSF.StartOffset();
   }