1468917 - When the ATOK2016 candidate window pops up, it flickers if browser in maximized size mode

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Alice0775 White]

Reproducible: always

Steps to Reproduce:
1. Maximize browser
2. Open data:text/html,<input> or any input field and select it
3. IME on
4. Type someting
5. Press Conversion key until candidate windou popup


Actual Results:
When the ATOK2016 candidate window pops up.
It appears at corner of screen for a moment. And then it moves to the correct position.

Expected Results:
Not flicker


Disabling e10s seems to fix the problem.

Other Browser does not have such annoying problem.

Comment 1

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

Hmm, cannot reproduce.

1. Is it recent regression?
2. Do you use InsiderPreview build in Fast ring?
3. Did you change intl.tsf.*?
4. Do you enable ATOKインサイト (設定 -> 入力・変換 -> 変換補助 -> 一時文書学習候補を表示する) and enable a11y feature of Firefox?
5. Do you see composition string when candidate window is not positioned correctly?

Sounds like the symptom is, when you need the candidate window, remote process hasn't notified the main process of content information such as character rectangles of composition string, caret position, etc. So, this could occur when the child process too busy or you typed really fast like machine. Anyway, if so, composition string shouldn't be shown in the editor since if remote process received a composition event at least once, remote process should've already sent layout information to the main process.

Comment 2

[Alice0775 White]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #1)
> Hmm, cannot reproduce.
> 
> 1. Is it recent regression?

No. at least it is replroduced on Firefox45 if e10s forcibly enabled

> 2. Do you use InsiderPreview build in Fast ring?

No. version 1803 os build 17134.112

> 3. Did you change intl.tsf.*?

No.

> 4. Do you enable ATOKインサイト (設定 -> 入力・変換 -> 変換補助 -> 一時文書学習候補を表示する) and enable
> a11y feature of Firefox?

Yes.

Disabled a11y does not help.
Disabled ATOKインサイト does not help.

> 5. Do you see composition string when candidate window is not positioned
> correctly?
> 

Yes. the composition string position seems to be ok.

Comment 3

[Alice0775 White]

Interestingly, 
this problem only occurs in maximized browser.
Not occur in normal size-mode and fullscreen size-mode.
However even in normal size-mode, the problem occurs if the top-left corner of the browser is outside of monitor screen.

Comment 4

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

Alice-san:

Thank you for the information, and sorry for the delay to reply due to the business trip.

I'm still trying to reproduce this bug, but not yet reproducible.

According to comment 3, sounds like a bug around multi-monitor.

Do you use 2 or more displays? If yes, do you use GPU driver specific feature like treating all displays as part of a desktop?

Do you use virtual desktop feature of Windows 10? Or something similar software of 3rd party?

Comment 5

[Alice0775 White]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4)
> Do you use 2 or more displays? If yes, do you use GPU driver specific
> feature like treating all displays as part of a desktop?

No. I use only 1 monitor.


> 
> Do you use virtual desktop feature of Windows 10? Or something similar
> software of 3rd party?

No.



I can also reproduce with new account of Windows10 Home x64 Ver1803 Build17134.112(i.e. default setting of windows and Atok2016).

Comment 6

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

Okay, it seems that I succeeded to reproduce this bug on VM which does not have 2nd display nor 2nd virtual desktop.

As far as I've tested, this looks like a bug of hack of ATOK. If we don't create native caret for ATOK (with disabling intl.tsf.hack.atok.create_native_caret), I cannot reproduce this bug. How about you, Alice-san?

Comment 7

[Alice0775 White]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> Okay, it seems that I succeeded to reproduce this bug on VM which does not
> have 2nd display nor 2nd virtual desktop.
> 
> As far as I've tested, this looks like a bug of hack of ATOK. If we don't
> create native caret for ATOK (with disabling
> intl.tsf.hack.atok.create_native_caret), I cannot reproduce this bug. How
> about you, Alice-san?


I confirmed that I cannot reproduce that problem anymore by setting intl.tsf.hack.atok.create_native_caret=false.

Comment 8

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

Thank you. Perhaps, we should stop doing the hack for ATOK 2016 and later since I don't see any problem with disabling it but I see side-effect of this hack mentioned in the following article.
https://d-toybox.com/studio/weblog/show.php?id=2018060200#ATOK2017

FYI: I don't reproduce this bug with ATOK 2017 nor ATOK 2015.

Comment 9

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

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

Comment 10

[Alice0775 White]

(In reply to Alice0775 White from comment #7)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> > Okay, it seems that I succeeded to reproduce this bug on VM which does not
> > have 2nd display nor 2nd virtual desktop.
> > 
> > As far as I've tested, this looks like a bug of hack of ATOK. If we don't
> > create native caret for ATOK (with disabling
> > intl.tsf.hack.atok.create_native_caret), I cannot reproduce this bug. How
> > about you, Alice-san?
> 
> 
> I confirmed that I cannot reproduce that problem anymore by setting
> intl.tsf.hack.atok.create_native_caret=false.

I think the side-effect is more annoying than this bug, because, the flickering occurs every next candidate is selected with arrowkey in the candidate window.

I think that WONTFIX is best for this bug unless the side effect is resolved.

Comment 11

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

No, the side-effect appears when we *do* the hack, and not doing the hack *fixes* this bug.

Comment 12

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

Ah, but another side-effect appears without the hack. That is the candidate window may appear at first clause even when second or later clause is being converted. I'll try to look for preventing this issue.

Comment 13

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

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

Comment 14

[Alice0775 White]

FYI, The try build in comment#13 works as expected(no flicker) on win64 opt and win32 opt build with/without e10s on Windows10.

Comment 15

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

Attachment: Bug 1468917 - part 1: Make TSFTextStore not create native caret if ATOK 2016 is active

Old ATOK referred native caret position to decide its candidate window position.
However, at least ATOK 2016 does not need to refer it.  Additionally, if we
create native caret for ATOK 2016, the candidate window position, ATOK 2016
refers the native caret only when we cannot return expected rect.  Therefore,
only immediately after modifying composition string, the position is different
from actual position by a couple of pixels and that looks like flicks the
candidate window.

So, we should stop creating native caret for ATOK 2016 (as same as ATOK 2017).

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

Comment 16

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

Attachment: Bug 1468917 - part 2: TSFTextStore::GetTextExt() shouldn't return TS_E_NOLAYOUT when ATOK retrieves text rects *in* the composition string

Currently, TSFTextStore::GetTextExt() won't return TS_E_NOLAYOUT error when
ATOK retrieves text rect of all of the composition string.

However, if user converts 2nd or later clause, ATOK retrieves text rect after
start of the character of selected clause.  Returning TS_E_NOLAYOUT in this
case causes candidate window being positioned temporarily below first character
of the composition string.

For avoiding the flicker of the candidate window, TSFTextStore::GetTextExt()
shouldn't return TS_E_NOLAYOUT when ATOK retrieves text rects *in* the
composition string.

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

Comment 17

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

Thank you for your test. I also don't see any problem with the build even with ATOK 2015, ATOK 2016, ATOK 2017 and the latest ATOK of ATOK Passport. I think the patches are enough safe.

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8987045 [details]
Bug 1468917 - part 1: Make TSFTextStore not create native caret if ATOK 2016 is active

https://reviewboard.mozilla.org/r/252288/#review259088

Comment 19

[Makoto Kato [:m_kato]]

Comment on attachment 8987046 [details]
Bug 1468917 - part 2: TSFTextStore::GetTextExt() shouldn't return TS_E_NOLAYOUT when ATOK retrieves text rects *in* the composition string

https://reviewboard.mozilla.org/r/252290/#review259090

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b99a97dc402
part 1: Make TSFTextStore not create native caret if ATOK 2016 is active r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e3179eede59d
part 2: TSFTextStore::GetTextExt() shouldn't return TS_E_NOLAYOUT when ATOK retrieves text rects *in* the composition string r=m_kato

Comment 21

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6b99a97dc402
https://hg.mozilla.org/mozilla-central/rev/e3179eede59d

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Masayuki, is it ok to let this change ride to release with 63, or do you think it is good for uplift to 62? Thanks!

Comment 23

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> Masayuki, is it ok to let this change ride to release with 63, or do you
> think it is good for uplift to 62? Thanks!

I was thinking as the latter. However, the fix improves UX of the latest version of ATOK and the risk is really low. Sure. I'll request to uplift.

Comment 24

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

Comment on attachment 8987045 [details]
Bug 1468917 - part 1: Make TSFTextStore not create native caret if ATOK 2016 is active

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of e10s.

[User impact if declined]:
ATOK users see annoying flicker of candidate window.

[Is this code covered by automated tests?]:
No since we don't have any automated test framework for native IME handler.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No, ATOK is 3rd party's IME, not free.  So, it's difficult to create environment.

[List of other uplifts needed for the feature/fix]:
The following patch.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Background:
ATOK is a 3rd party's Japanese IME since many years ago. We've create Windows' native caret for some Chinese IMEs when Win9x, 2k and XP since they referred native caret position to show their reading window. I'm not sure the exact version number, ATOK started to refer the native caret position only when active window is Gecko. Therefore, for supporting old ATOK (2011 - 2015), we need to keep native caret even after we supported TSF on Vista and later.

On the other hand, at ATOK 2016, they probably rewrite the code to decide their candidate window. They still refer native caret if there is, however, it includes at least this bug.  Additionally, if there is no native caret, they decide the position only with API of TSF.

So, we should drop ATOK 2016 from the blacklist of ATOK which require native caret so that this patch only do it.  Note that ATOK 2017 and "ATOK" (starting 2018, there is only subscription license. So, the year is dropped from the product name).

[String changes made/needed]:
No.

Comment 25

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

Comment on attachment 8987046 [details]
Bug 1468917 - part 2: TSFTextStore::GetTextExt() shouldn't return TS_E_NOLAYOUT when ATOK retrieves text rects *in* the composition string

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of the previous patch for ATOK 2016 users.
Regression of bug 1273510 for ATOK 2017 and the latest ATOK users.

[User impact if declined]:
Candidate window of ATOK may dance around proper position when user is choosing a word from candidate window.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
The previous patch.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
When there is no native caret, ATOK decides candidate window position only with API of TSF. However, the API is still buggy in the latest Win10 (it'll be fixed on the next Win10 update at this fall). Therefore, until remote process finishes reflow for the new composition string, we need to notify ATOK of fake position rather than an error code to notify IME of waiting next layout change notification from us.

At fixing bug 1273510, I thought ATOK always queries rectangle of whole composition string when there is no native caret.  However, actually, ATOK may query rectangle between 2nd or later character and the last character of composition string.  In this case, we return error code so that ATOK confused.

This patch makes us not return the error code to ATOK if ATOK queries rectangle of *part* of composition string.

[String changes made/needed]:
No.

FYI: ATOK 2016 and later's share in Nightly build testers who input Japanese text is about 6%.

Comment 26

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

> FYI: ATOK 2016 and later's share in Nightly build testers who input Japanese text is about 6%.

Oops, I used wrong parameter, the correct number is about 11%.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8987045 [details]
Bug 1468917 - part 1: Make TSFTextStore not create native caret if ATOK 2016 is active

IME fix, verified in Nightly, let's uplift for beta 7.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b5967b6eb0a9
https://hg.mozilla.org/releases/mozilla-beta/rev/462984c1b4ff