962920 - [TSF] Should commit composition at entering modal state

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

Attachment: testcase

Even if modal dialog such as file picker is opened during composition, composition isn't committed.

Especially in TSF mode, this makes TIP confused. We should add new notification NOTIFY_IME_OF_ENTERING_MODAL_STATE and nsTextStore should handle it.

STR:

0. Allow popup on the test page
1. Open the testcase
2. Start composition, then, file picker is opened per twice compositionupdate.

Comment 1

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

Attachment: testcase

Hmm, handling WM_ENTERIDLE has a problem. The composition events for committing composition are fired *while* in modal state. This means that web contents can run in the modal loop.

Comment 2

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

Ah, but looks like it's another bug. Even while a file picker is open, setInterval() works.

Comment 3

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

Attachment: Patch

Handling WM_ENTERIDLE message in nsTextStore is enough at least for now.

While a native modal dialog such as file picker, color picker or print dialog is open, the parent window receives almost only WM_ENTERIDLE. So, if WM_ENTERIDLE is received on a window which has composition, nsTextStore should commit the composition. It means that we ungrab IME from nsTextStore. Then, users will be able to use IME on the modal dialog.

By this change, compositionupdate, compositionend and somebody handlers can run while native modal dialog is open. But this doesn't break Gecko's design because setInterval() is also available. So, it's not the first way to run script under native modal dialog.

In IMM mode, we have same issue. However, even if we request IME to commit the composition, WM_IME_COMPOSITION* messages will be handled by the native modal dialog since focus is already moved and active IMM context is the dialog's. So, we cannot fix this bug in IMM mode easy.

As I commented in comment #0, we should use nsIWidget::NotifyIME() when we call API to open native modal dialog. However, there is no good point to insert the call. We need to add it for each opener. Therefore, it's not so better than this patch. And when we will support new native dialog, we need to add it. But probably adding a call of nsIWidget::NotifyIME() must be forgotten. So, this approach must not be easy to maintain.

In conclusion, we should fix this bug only in TSF mode and handle WM_ENTERIDLE directly. This is the simplest fix.

Comment 4

[Jim Mathies [:jimm]]

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

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

sgtm!

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d98395828c7

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d98395828c7