Closed Bug 1275906 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression, Whiteboard: btpp-active)

Attachments

(2 files)

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.
Whiteboard: btpp-active
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/
Attachment #8759019 - Flags: review?(m_kato)
Attachment #8759020 - Flags: review?(bugs)
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/
Attachment #8759019 - Flags: review?(m_kato) → review+
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
This is hard to follow. Reading the patch still couple of more times.
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
Attachment #8759020 - Flags: review?(bugs) → review+
(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.
Pushed by masayuki@d-toybox.com:
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
https://hg.mozilla.org/mozilla-central/rev/1104c46a20f2
https://hg.mozilla.org/mozilla-central/rev/8374debdc933
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.