1435730 - [TSF][TS_E_NOLAYOUT] Candidate window of Japanist 10 positioned incorrectly and flicked (not aware of TS_E_NOLAYOUT issue)

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Japanist 10 is a new Japanese TIP.  However, this is not aware of TS_E_NOLAYOUT issue. Therefore, when we return TS_E_NOLAYOUT from ITextStoreACP::GetTextExt(), it shows candidate window at wrong point or reposition it when we notify it of layout change. So, we need to add some hack for it too.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1435730 - part 1: Make TSFTextStore::GetTextExt() not return TS_E_NOLAYOUT error to Japanist 10 when the range is in composition string

Similar to ATOK, Japanist 10 requests all or part of composition string.
If we return TS_E_NOLAYOUT in this case, you'll see candidate window at
top-left of the screen.

For avoiding this issue, we should not return TS_E_NOLAYOUT to Japanist 10
when the query range is in composition string.

Review commit: https://reviewboard.mozilla.org/r/218322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218322/

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1435730 - part 2: TSFTextStore::GetTextExt() shouldn't adjust acpStart to start of composition string if ContentCacheInParent perhaps has some character rects in composition string

The renaming problem is, when I try to convert 2nd or later clause of
composition string with Japanist 10, it shows candidate window below the
start of composition string first, then, it moves candidate window to
below the selected clause.  This is caused by our bug of the hack in
TSFTextStore::GetTextExt().

First, we compute wrong minimum modified
offset of mContentForTSF.  It stores last composition string when it's
initialized.  Then, when a part of composition string is modified, it
sets minimum modified offset with the last composition string.  However,
we don't update it when we receive notifications from content which means
all dispatched composition events are handled in content and
ContentCacheInParent stores character rects at least in this time.  So,
this patch adds TSFTextStore::Content::OnCompositionEventsHandled() to
update the last composition string.

Next, TSFTextStore::GetTextExt() always adjusts acpStart to start of
composition string when acpStart is larger than composition start.
However, this causes this remaining problem.  If ContentCacheInParent
stores character rects of even older composition string, we should use
it as far as possible.  This must not be problem in most cases since
most Chinese characters and Japanese Kana characters have same width.

This touches share code of the hack between any TIPs.  However, this must
not be risky because this patch just reduces amount of adjusting acpStart
offset in safe range.

Review commit: https://reviewboard.mozilla.org/r/218324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218324/

Comment 4

[Makoto Kato [:m_kato] (PTO until May 6)]

Comment on attachment 8948912 [details]
Bug 1435730 - part 1: Make TSFTextStore::GetTextExt() not return TS_E_NOLAYOUT error to Japanist 10 when the range is in composition string

https://reviewboard.mozilla.org/r/218322/#review224092

Comment 5

[Makoto Kato [:m_kato] (PTO until May 6)]

Comment on attachment 8948913 [details]
Bug 1435730 - part 2: TSFTextStore::GetTextExt() shouldn't adjust acpStart to start of composition string if ContentCacheInParent perhaps has some character rects in composition string

https://reviewboard.mozilla.org/r/218324/#review224102

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39ecbb5103c1
part 1: Make TSFTextStore::GetTextExt() not return TS_E_NOLAYOUT error to Japanist 10 when the range is in composition string r=m_kato
https://hg.mozilla.org/integration/autoland/rev/5785602f6ff4
part 2: TSFTextStore::GetTextExt() shouldn't adjust acpStart to start of composition string if ContentCacheInParent perhaps has some character rects in composition string r=m_kato

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/39ecbb5103c1
https://hg.mozilla.org/mozilla-central/rev/5785602f6ff4

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

We have not had this bug report from actual Japanist 10 users yet. I think that number of Japanist 10 users in our users is really small. Additionally, the part 2 patch may affect to other IMEs. I've tested Nightly with various Japanese IMEs for 2 days, but don't have enough time to test Chinese IMEs. So, perhaps, we shouldn't uplift these patches since they'll be fixed by the next ESR.