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

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

[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.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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

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.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7ebbddec47d

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db787e9ae7b6

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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

Fix the warnings (uint32_t vs. LONG)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah,

> mComposition={ mStart=%u

I'll modify it to %d, please ignore that.

Comment 8

[Masatoshi Kimura [:emk]]

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?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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();)

Comment 10

[Yohei Yukawa]

(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,

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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...

Comment 12

[Yohei Yukawa]

(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.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Masayuki, can you please provide STR? I am unable to understand the scenario based on the bug description. Thanks!

Comment 14

[Masatoshi Kimura [:emk]]

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."

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/d1aaf77d3988d82623b4b040c001502a807533fb
changeset:  d1aaf77d3988d82623b4b040c001502a807533fb
user:       Masayuki Nakano <[e-mail]>
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

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1aaf77d3988

Comment 18

[Kate Glazko]

Hi Masayuki, can you verify that this is fixed in 42? Are you planning to request an uplift for Aurora 41?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: 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)

Yeah, I'll request approval-aurora after I verify the fix on today's Nightly.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Should I uplift the patch by myself? Or will somebody do it?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/905fc8e8c2bc

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Ryan.