835262 - [TSF] Should keep composition when we are deactivated

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

STR:

1. Start composition in TSF mode
2. Activate another application
3. Activate the composed window again

Actual Result:

The composition string looks like committed, but the composition is still alive in TIP.

Expected Result:

The composition string shouldn't be committed at deactivating.


Currently, when we're deactivated, nsIWidget::OnIMEFocusChange(false) is called and the nsTextStateManager instance is destroyed. That causes the nsTextStore is destroyed but the composition can be alive without the TextStore because we failed to commit the composition forcibly in TIP.

For bug 789708, we should keep the nsTextStore instance even during deactivated.

Comment 1

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

Attachment: Patch

This also fixes bug 789708 on Win 8.1. I'll test this patch on other versions of Windows. After that, I'll request the review.

Comment 2

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

Attachment: Patch

This patch works fine on WinXP, WinVista, Win7 and Win8.1. However, all versions of MS-IME commits composition at deactivating our process. I guess that when the thread manager loses focus, MS-IME automatically commits composition. Looks like this is MS-IME specific behavior since it works as same with wordpad which is also TSF-aware application.

Kimura-san:

Could you review the nsTextStore and the behavior?

Smaug:

Could you review nsIWidget and nsIMEStateManager?

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=1c532f5157ee

Comment 3

[Masatoshi Kimura [:emk]]

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

>--- a/widget/nsIWidget.h
>+++ b/widget/nsIWidget.h
>@@ -209,33 +209,36 @@ enum nsTopLevelWidgetZPlacement { // for
>   typedef int8_t Notifications;
...
>+    NOTIFY_DURING_DEACTIVE   = 0x80

0x80 will overflow from int8_t. Please change Notifications to an alias of uint8_t.

>--- a/widget/windows/nsTextStore.cpp
>+++ b/widget/windows/nsTextStore.cpp
>@@ -3047,17 +3047,18 @@ nsIMEUpdatePreference
> nsTextStore::GetIMEUpdatePreference()
> {
>   int8_t notifications = nsIMEUpdatePreference::NOTIFY_NOTHING;

Why don't you use Notifications here?

Comment 4

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

Nice catch!

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e17b97b0c9a

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e17b97b0c9a