1275906 - NOTIFY_IME_OF_COMPOSITION_UPDATE should be notified after NOTIFY_IME_OF_SELECTION_CHANGE, NOTIFY_IME_OF_TEXT_CHANGE and NOTIFY_IME_OF_POSITION_CHANGE

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

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

This is a regression of bug1203381.

We've not documented explicitly, NOTIFY_IME_OF_COMPOSITION_UPDATE should be notified after NOTIFY_IME_OF_SELECTION_CHANGE, NOTIFY_IME_OF_TEXT_CHANGE and NOTIFY_IME_OF_POSITION_CHANGE.

Actually, TSFTextStore assumes this order. So, this regression might have some symptoms actually.

I guess, TextComposition should notify NOTIFY_IME_OF_COMPOSITION_UPDATE via IMEContentObserver since IMEContentObserver manages all notifications.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bbd0b6cd0d9

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7299b2a95283

Comment 3

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

Attachment: MozReview Request: Bug 1275906 part.1 Rename NOTIFY_IME_OF_COMPOSITION_UPDATE to NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED r?m_kato

It's not clear to me what NOTIFY_IME_OF_COMPOSITION_UPDATE means only from the name. For making the name clearer, this patch renames it to NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED and add some explanation to the definition.

Review commit: https://reviewboard.mozilla.org/r/57116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57116/

Comment 4

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

Attachment: MozReview Request: Bug 1275906 part.2 TextComposition should use IMEContentObserver for sending NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED if the editor which has the composition is in the active IMEContentObserver r?smaug

For sending NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED after the other change notifications which was caused by the user input, we need to use IMEContentObserver::IMENotificationSender because it sends the notifications when it's safe to do it.

This patch makes TextComposition use IMEContentObserver to send the notification.  However, if there is no active IMEContentObserver, e.g., composition events are fired on unfocused window, TextComposition sends it by itself (same as current implementation).

If IMEContentObserver stops observing when it has pending NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED, it cannot send the notification (i.e., it is discarded completely in such case). However, in such case, IMEContentObserver sends NOTIFY_IME_OF_BLUR.  So, anyway, native IME handler should treat the blur notification as it including NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED.

On the other hand, we're buggy if composition events are fired in non-active window.  Even in such case, IMEContentObserver should be created for active editor in each document and it notifies IME of the changes. But this is out of the scope of this bug.

Review commit: https://reviewboard.mozilla.org/r/57118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57118/

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8759019 [details]
MozReview Request: Bug 1275906 part.1 Rename NOTIFY_IME_OF_COMPOSITION_UPDATE to NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED r?m_kato

https://reviewboard.mozilla.org/r/57116/#review53860

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

This is hard to follow. Reading the patch still couple of more times.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8759020 [details]
MozReview Request: Bug 1275906 part.2 TextComposition should use IMEContentObserver for sending NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED if the editor which has the composition is in the active IMEContentObserver r?smaug

https://reviewboard.mozilla.org/r/57118/#review54064

Comment 8

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

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #6)
> This is hard to follow. Reading the patch still couple of more times.

Sorry for the big patch. Most part of IMEContentObserver is copy of other methods (e.g., MaybeNotifyCompositionEventHandled(), PostCompositionEventHandledNotification() and SendCompositionEventHandled() are almost same as the methods for NOTIFY_IME_OF_POSITION_CHANGE). I should've explained about that.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1104c46a20f2
part.1 Rename NOTIFY_IME_OF_COMPOSITION_UPDATE to NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/8374debdc933
part.2 TextComposition should use IMEContentObserver for sending NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED if the editor which has the composition is in the active IMEContentObserver r=smaug

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1104c46a20f2
https://hg.mozilla.org/mozilla-central/rev/8374debdc933