835280 - IME shouldn't be committed at activating composing window

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

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

This is a regression of bug 805766.

In bug 805766, we make nsIMEStateManager::OnChangeFocus() called without aContent at activating the window. This causes nsIMEStateManager misunderstanding focus is actually changing. Therefore, nsIMEStateManager commits composition forcibly.

We need to keep composition on most platforms even during deactivated.

Comment 1

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

Attachment: Patch

The attempt of the regression cause is, nsFocusManager notifies focus change to a designMode editor of nsIMEStateManager. In other words, we don't need to call nsIMEStateManager::OnChangeFocus() in the other cases.

By calling it always, nsIMEStateManager misunderstands the focus move at activating a window. nsIMEStateManager thinks all elements in the activating document lost focus first and then, focused content gets focus again. At the first step, IME state becomes disabled. Therefore, at activating our window, the composition string is forcibly committed.

So, we should limit the call only when the document is editable (i.e., the document is in designMode).

Comment 2

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

Note that when a designMode editor has focus and the method is called at activating, the IME state doesn't become disabled since it's editable. I.e., it's problem if it's called with non-editable document but an editable element in it will have focus actually.

Comment 3

[Neil Deakin]

The IsEditable() check used here is different than what nsFocusManager::IsNonFocusableRoot uses. Does that matter?

Comment 4

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

Attachment: Patch

It doesn't matter for the meaning. I meant that, in this case, probably they always return same result. However, in the nsINode::IsEditableInternal(), the method checks its owner document too. This is redundant in this case. So, checking the flag directly is probably better here.

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a072eb27f1d

landed on inbound.

This is a regression starting 19 (i.e., current beta).

The regression cause is bug 805766 which adds the calling nsIMEStateManager::OnChangeFocus() at document getting focus.

However, it's needed when designMode editor gets focus but the patch calls it every time at any document getting focus.

When the non-designMode editor gets focus but the active element (i.e., last focused element in the document) is an editable element (e.g., <input type="text"> or <textarea>), the call causes nsIMEStateManager misunderstanding as the editable element getting lost the focus. Then, nsIMEStateManager commits active composition forcibly. So, as the landed patch, we can fix this regression by limiting to call only when designMode editor gets focus.

If we don't fix this bug, active composition may be committed unexpectedly when another applications window steals focus such as a popup window of security software. And also, on some Window Manager of Linux, IME's candidate list window might steal focus when it opens. If there is such environment, the user cannot use IME on Firefox 19 (Although, such environment is very rare, probably).

If we would back out the patch, some automated tests would fail because the fix improves the designMode editor behavior on Android and TSF mode on Windows. So, landing this fix is better and safer.

So, I strongly recommend that we should fix this regression even on beta.

Comment 6

[Alex Keybl [:akeybl]]

Please nominate for uplift as soon as possible.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0a072eb27f1d

Comment 8

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

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
bug 805766, and it's base of some other IME related changes.

User impact if declined:
On Windows, if some other application's window steals focus from us (I met this situation with an alert popup of security software, NOD32), then, the composing string is forcibly committed and the user need to input the composition string from start again.

On Linux, if there is some Window Manager whose popup window steals focus, that causes that showing IME's candidate window would commit the composition string at converting it. I.e., the user cannot use IME on such environment.

And also, the unexpected commit might be registered into the dictionary of the IME.

Testing completed (on m-c, etc.):
Landed on m-c. And I confirmed that the bug is fixed.

Risk to taking this patch (and alternatives if risky):
I believe that the risk is not high. In bug 805766, I added to call nsIMEStateManager::OnChangeFocus() at document getting focus for designMode editor. However, it causes nsIMEStateManager misunderstanding the focused editor losing focus at activating the window if the document isn't in designMode. Therefore, this patch limits to call the method only when the document is in desginMode.

String or UUID changes made by this patch:

Nothing.

Comment 9

[Alex Keybl [:akeybl]]

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

Spoke with Masayuki - given where we are in the cycle, and the low impact to users, this IME change is too risky for FF19. We'll wontfix for FF19 and resolve for the first time in FF20.

Comment 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/6008ec29e703