Closed Bug 1187566 Opened 5 years ago Closed 5 years ago

[TSF] TSFTextStore::Content should compute mMinTextModifiedOffset only with the latest composition string and original composition string

Categories

(Core :: Widget: Win32, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 + fixed
firefox42 + fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(2 files, 3 obsolete files)

7.80 KB, patch
emk
: review+
Details | Diff | Splinter Review
7.96 KB, patch
Details | Diff | Splinter Review
[Tracking Requested - why for this release]:

This bug is what Google Japanese Input's candidate window sometimes positioned at bottom-left of the window when you convert second or later clause.

We should fix this in Aurora (first TSF release) too since this is much ugly for Google Japanese Input users. Fortunately, the patch isn't risky.

Additionally I see this symptom on Win10. So, Google Japanese Input still has TS_E_NOLAYOUT problem in some cases. We should enable the hack for that on Win10 too.
When Google Japanese Input modifies composition string, all composition string is removed first and insert new composition string. Therefore, current TSFTextStore::Content::mMinTextModifiedOffset becomes 0. Therefore the hack for Google Japanese Input in GetTextExt() won't work when you convert second or later clause. I.e., this patch makes mMinTextModifiedOffset the first character offset of the converting clause.

And also, the hack prevents similar problem on Win10. So, I guess that Google Japanese Input must still have a bug which is not related to TS_E_NOLAYOUT. Let's enable the hack on Win10 for the convenience of Google Japanese Input users.
Attachment #8638875 - Flags: review?(VYV03354)
Ah,

> mComposition={ mStart=%u

I'll modify it to %d, please ignore that.
Comment on attachment 8638878 [details] [diff] [review]
TSFTextStore::Content should compute mMinTextModified Offset only with the latest composition string and original composition string, and also the hack should be enabled on Win10

Review of attachment 8638878 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/TSFTextStore.cpp
@@ +5322,5 @@
> +      } else if (mMinTextModifiedOffset >=
> +                   static_cast<uint32_t>(mComposition.mStart) &&
> +                 mMinTextModifiedOffset <
> +                   static_cast<uint32_t>(mComposition.EndOffset())) {
> +        // The previous change in the composition string must be canceled.

I don't understand this comment. Who cancels the change? Did the TIP cancel the change? Or do we cancel the change by setting mMinTextModifiedOffset here?
Thank you, Kimura-san!

(In reply to Masatoshi Kimura [:emk] from comment #8)
> Comment on attachment 8638878 [details] [diff] [review]
> TSFTextStore::Content should compute mMinTextModified Offset only with the
> latest composition string and original composition string, and also the hack
> should be enabled on Win10
> 
> Review of attachment 8638878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/TSFTextStore.cpp
> @@ +5322,5 @@
> > +      } else if (mMinTextModifiedOffset >=
> > +                   static_cast<uint32_t>(mComposition.mStart) &&
> > +                 mMinTextModifiedOffset <
> > +                   static_cast<uint32_t>(mComposition.EndOffset())) {
> > +        // The previous change in the composition string must be canceled.
> 
> I don't understand this comment. Who cancels the change? Did the TIP cancel
> the change? Or do we cancel the change by setting mMinTextModifiedOffset
> here?

In that case, TIP changes the composition string twice or more and the mMinTextModifiedOffset is in the original composition string but the new composition string becomes the original composition string (because of mComposition.mString == mLastCompositionString here). Therefore, at least the changes in the composition string is canceled (but no sure about the later characters, therefore, the new mMinTextModifiedOffset is mComposition.EndOffset();)
Flags: needinfo?(VYV03354)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 7/27 - 8/2) from comment #0)
> [Tracking Requested - why for this release]:
> 
> This bug is what Google Japanese Input's candidate window sometimes
> positioned at bottom-left of the window when you convert second or later
> clause.

Can you include the version number of Google Japanese Input so that future readers can reproduce the issue correctly?

The latest publicly available version of Google Japanese Input for Windows is 2.16.2007.100 (dev channel version) as of writing this, and my understanding is that that version can deal with the TS_E_NOLAYOUT bug in TSF runtime w/o any hack in the application side.
https://tools.google.com/dlpage/japaneseinput/eula.html?hl=ja&extra=dev

Thanks,
Flags: needinfo?(masayuki)
(In reply to Yohei Yukawa from comment #10)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 7/27 - 8/2)
> from comment #0)
> > [Tracking Requested - why for this release]:
> > 
> > This bug is what Google Japanese Input's candidate window sometimes
> > positioned at bottom-left of the window when you convert second or later
> > clause.
> 
> Can you include the version number of Google Japanese Input so that future
> readers can reproduce the issue correctly?

My version is 1.13.1641.0. I just installed stable version and don't stop auto updater.

> The latest publicly available version of Google Japanese Input for Windows
> is 2.16.2007.100 (dev channel version) as of writing this, and my
> understanding is that that version can deal with the TS_E_NOLAYOUT bug in
> TSF runtime w/o any hack in the application side.
> https://tools.google.com/dlpage/japaneseinput/eula.html?hl=ja&extra=dev

Thanks. I'll try that, but our main target is stable version users, of course...
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 7/27 - 8/2) from comment #11)
> Thanks. I'll try that, but our main target is stable version users, of
> course...

I understood you point, but I also think that information would be really important and helpful for Mozc team like you are gathering information from nightly users for Firefox.
Masayuki, can you please provide STR? I am unable to understand the scenario based on the bug description. Thanks!
Flags: needinfo?(masayuki)
Comment on attachment 8638878 [details] [diff] [review]
TSFTextStore::Content should compute mMinTextModified Offset only with the latest composition string and original composition string, and also the hack should be enabled on Win10

Review of attachment 8638878 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/TSFTextStore.cpp
@@ +5312,5 @@
>          firstDifferentOffset =
>            mComposition.mStart +
>              FirstDifferentCharOffset(mComposition.mString,
>                                       mLastCompositionString);
> +        // The previous change to the composition string must be canceled.

OK, I prefer "The previous change to the composition string is canceled."
Attachment #8638878 - Flags: review?(VYV03354) → review+
Flags: needinfo?(VYV03354)
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/d1aaf77d3988d82623b4b040c001502a807533fb
changeset:  d1aaf77d3988d82623b4b040c001502a807533fb
user:       Masayuki Nakano <masayuki@d-toybox.com>
date:       Mon Aug 03 15:15:30 2015 +0900
description:
Bug 1187566 TSFTextStore::Content should compute mMinTextModified Offset only with the latest composition string and original composition string, and also the hack should be enabled on Win10 r=emk
(In reply to Ritu Kothari (:ritu) from comment #13)
> Masayuki, can you please provide STR? I am unable to understand the scenario
> based on the bug description. Thanks!

Absolutely. Sorry for the poor information in the request message.

STR:
1. Install Google Japanese Input for Windows (this is a major third party IME in Japan, https://www.google.co.jp/ime/)
2. Launch Firefox and select Google Japanese Input in the language bar.
3. Type "watasinonamaehanakanodesu", then, composition string must be separated to two clauses: "私の名前は" and "中野です"
4. Select the second clause by right arrow key.
5. Press spacebar a lot of times to convert the second clause.

Expected Result:

The candidate window which lists converted string of the second clause should be positioned bottom of the first character of the second clause.

Actual Result:

The candidate window is positioned at the expected position in most times. However, sometimes, it's moved to botton-left of the focused window.
Flags: needinfo?(masayuki) → needinfo?(rkothari)
https://hg.mozilla.org/mozilla-central/rev/d1aaf77d3988
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Hi Masayuki, can you verify that this is fixed in 42? Are you planning to request an uplift for Aurora 41?
Flags: needinfo?(masayuki)
Comment on attachment 8643024 [details] [diff] [review]
nsTextStore::Content should compute mMinTextModified Offset only with the latest composition string and original compos ition string, and also the hack should be enabled on Win10 (For Aurora)

Approval Request Comment
[Feature/regressing bug #]:This is necessary for enabling TSF mode in release builds (TSF is the new IME framework on Windows)
[User impact if declined]: Google Japanese Input which is a very major third party IME in Japan has some bugs of deciding candidate window. We implemented hack for that in bug 1061604, however, it doesn't work well in some cases. And it's caused by a bug of TSF which is fixed on Win10 but current stable release still has this bug even on Win10. So, user thinks this is a new regression of us and the behavior is ugly. (candidate window is moved from caret position to the botton-left of the window randomly)
[Describe test coverage new/current, TreeHerder]: Landed on m-c. I tested manually on the latest Nightly. And I also tested on debug build of Aurora.
[Risks and why]: Low because the changed code is referred only from a few IMEs which need similar hack. (I don't see the regression with such IMEs)
[String/UUID change made/needed]: Nothing.
Attachment #8643024 - Flags: approval-mozilla-aurora?
Comment on attachment 8643024 [details] [diff] [review]
nsTextStore::Content should compute mMinTextModified Offset only with the latest composition string and original compos ition string, and also the hack should be enabled on Win10 (For Aurora)

Let's uplift to Aurora. This fix has been in Nightly for a few days so should be stable. We are supporting TSF in FF41 so uplifting to Aurora41 makes sense.
Flags: needinfo?(rkothari)
Attachment #8643024 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Should I uplift the patch by myself? Or will somebody do it?
Flags: needinfo?(rkothari)
Thank you, Ryan.
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.