[TSF] Should keep composition when we are deactivated

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla29
x86_64
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch Patch (obsolete) — Splinter Review
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.
Posted patch PatchSplinter Review
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
Attachment #8360219 - Attachment is obsolete: true
Attachment #8360303 - Flags: review?(bugs)
Attachment #8360303 - Flags: review?(VYV03354)
Attachment #8360303 - Flags: review?(bugs) → review+
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?
Attachment #8360303 - Flags: review?(VYV03354) → review+
https://hg.mozilla.org/mozilla-central/rev/3e17b97b0c9a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 1053053
You need to log in before you can comment on or make changes to this bug.