Alice0775 White Reporter
	Description • 10 years ago

Using the old Atok, Candidate window position is always opened the top-left of browser window.

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16998eb738aa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141002222135
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec43ac8b65c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20141002233435
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=16998eb738aa&tochange=3ec43ac8b65c

Triggered by: Bug 975383

	Toshihiro Yamada
	Comment 2 • 10 years ago

Similar problem is observed with Atok 2014(latest version).
Candidate window position is always bottom-left of browser window.

	Alice0775 White Reporter
	Comment 3 • 10 years ago

[Tracking Requested - why for this release]: UI broken due to Bug 975383

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 10 years ago

This is NOT a new regression. However, this bug was hidden by another bug which was removed by bug 975383.

At bug 975383, a caller of nsIMM32Handler::SetIMERelatedWindowsPos() immediately after dispatching NS_COMPOSITION_UPDATE is removed.  However, for making nsIMM32Handler e10s-aware, we shouldn't call it synchronously. The correct approach is, the method should be called when NOTIFY_IME_OF_COMPOSITION_UPDATE is notified by TextComposition.

However, in TSF mode, WinIMEHandler::NotifyIME() never calls nsIMM32Handler::OnUpdateComposition(). This is the cause of this bug.

This bug shouldn't be reproduced on most users of Aurora, Beta and Release. But somebody may enable TSF. For such people, we need to uplift this.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 10 years ago

This will fix the design of WinIMEHandler. That is necessary for enabling TSF in Aurora for IMM IME users.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 10 years ago

The cause of this bug is, WinIMEHandler doesn't send notification of composition updates to nsIMM32Handler. Therefore, nsIMM32Handler never calls SetIMERelatedWindowsPos(). The reason why this is regressed by bug 975383 is that NS_COMPOSITION_UPDATE event dispatcher called SetIMERelatedWindowsPos() unexpectedly and that was removed by the change.

WinIMEHandler was designed before I realized that even in TSF mode, we need to use nsIMM32Handler for handling IMM IME. Therefore, there are some bugs. All of them should be fixed here:

1. IMEHandler::IsComposing() and IsComposingOn() don't check if nsIMM32Handler has composition in TSF mode. This causes dispatching key events during composition.

2. IMEHandler::NotifyIME() doesn't send notifications in TSF mode.

3. IMEHandler::GetOpenState() queries IME open state to TSF even if IMM IME is active.

	Masatoshi Kimura [:emk]
	Comment 7 • 10 years ago

Comment on attachment 8504802 [details] [diff] [review]
Patch

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

::: widget/windows/WinIMEHandler.cpp
@@ +189,5 @@
> +        // composition window position.
> +        if (IsIMMActive()) {
> +          nsIMM32Handler::OnUpdateComposition(aWindow);
> +        }
> +        return NS_OK;

When IMM IME is inactive, this patch will change the return value from NS_ERROR_NOT_IMPLEMENTED to NS_OK. Is it intentional?

@@ +208,5 @@
>        case REQUEST_TO_COMMIT_COMPOSITION:
>          if (nsTextStore::IsComposingOn(aWindow)) {
>            nsTextStore::CommitComposition(false);
> +        } else if (IsIMMActive()) {
> +          nsIMM32Handler::CommitComposition(aWindow);

Is nsIMM32Handler::IsComposingOn() unnecessary?

	Masatoshi Kimura [:emk]
	Comment 8 • 10 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> WinIMEHandler was designed before I realized that even in TSF mode, we need
> to use nsIMM32Handler for handling IMM IME.

Out of curiosity, why is it needed?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 10 years ago

(In reply to Masatoshi Kimura [:emk] from comment #7)
> Comment on attachment 8504802 [details] [diff] [review]
> Patch
> 
> Review of attachment 8504802 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/WinIMEHandler.cpp
> @@ +189,5 @@
> > +        // composition window position.
> > +        if (IsIMMActive()) {
> > +          nsIMM32Handler::OnUpdateComposition(aWindow);
> > +        }
> > +        return NS_OK;
> 
> When IMM IME is inactive, this patch will change the return value from
> NS_ERROR_NOT_IMPLEMENTED to NS_OK. Is it intentional?

Yes. The result shouldn't depend on active IME. Currently, the caller doesn't change the behavior with the result, but if it would do that, depending on active IME type may have caused unexpected bug.

> @@ +208,5 @@
> >        case REQUEST_TO_COMMIT_COMPOSITION:
> >          if (nsTextStore::IsComposingOn(aWindow)) {
> >            nsTextStore::CommitComposition(false);
> > +        } else if (IsIMMActive()) {
> > +          nsIMM32Handler::CommitComposition(aWindow);
> 
> Is nsIMM32Handler::IsComposingOn() unnecessary?

Yes. It will be checked in the method:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsIMM32Handler.cpp#199
and currently, NotifyIME() doesn't check it in IMM mode:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/WinIMEHandler.cpp#198

(In reply to Masatoshi Kimura [:emk] from comment #8)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > WinIMEHandler was designed before I realized that even in TSF mode, we need
> > to use nsIMM32Handler for handling IMM IME.
> 
> Out of curiosity, why is it needed?

Because IMM IMEs don't cause accessing ITextStoreACP. Instead, they send/post WM_IME_* messages and respond to ImmFoo() APIs. So, actually, IMM IME is still handled by nsIMM32Handler. See bug 951966.

	Masatoshi Kimura [:emk]
	Comment 10 • 10 years ago

Comment on attachment 8504802 [details] [diff] [review]
Patch

Thanks for the clarifications.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 11 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e59d5635eef2

Thanks!

	Carsten Book [:Tomcat]
	Comment 12 • 10 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 13 • 10 years ago

Looks like we need an aurora uplift here to prevent shipping this regression, please nominate.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 10 years ago

Right. I'll check the patch can be applied to Aurora.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 15 • 10 years ago

Comment on attachment 8504802 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 975383
[User impact if declined]: A candidate window of IME is very important UI. It should be near the caret position. However, this bug makes the window top-left corner of the window or hidden (depends on IME because we don't set candidate window position, so, the result is the default behavior of IME)
[Describe test coverage new/current, TBPL]: I tested manually. We cannot test native IME behavior with automated tests.
[Risks and why]: Risk isn't so high. Even if this has other regressions, it affects only few testers who enable TSF mode in Aurora, Beta or Release channel (TSF is now only enabled on Nightly in default settings) and the user uses IME implemented with IMM. This patch checks if active IME is not a IME of TSF aware. If so, it just falls back to IMM event handler (IMM is old IME framework of Windows).
[String/UUID change made/needed]: Nothing.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/9936bcca134f