1149172 - Query IMEStateManager for composition state

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Right now we keep tracking of the current composition state ourselves (nsWindow::mIMEComposing, nsWindow::mIMEComposingStart, etc.).

However, we should query IMEStateManager for that state and that should make things more consistent between widget and content.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Fix small bugs in IME focus handshake (v1)

Small patch to piggyback on this bug

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Send well-formed IME events (v1)

Small patch to piggyback on this bug

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Query IMEStateManager for composition state (v1)

Use IMEStateManager to query the current composition states.

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8585524 [details] [diff] [review]
Fix small bugs in IME focus handshake (v1)

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

Looks good.

::: mobile/android/base/GeckoEditable.java
@@ +764,5 @@
>                  mActionQueue.syncWithGecko();
>                  mListener.notifyIME(type);
> +
> +                if (type == NOTIFY_IME_OF_BLUR) {
> +                    mFocused = false;

Maybe add a comment on why we shouldn't set this before unmasking events and syncing.

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8585526 [details] [diff] [review]
Send well-formed IME events (v1)

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

Looks good.

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8585527 [details] [diff] [review]
Query IMEStateManager for composition state (v1)

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

I like the simplified state machine, looks good.

::: widget/android/nsWindow.cpp
@@ +1673,5 @@
>  /*
> + * Get the current composition object, if any.
> + */
> +nsRefPtr<mozilla::TextComposition>
> +nsWindow::GetIMEComposition()

I wish we could declare this function const, but then the rest of the chain would need to const-correct first.

@@ +1785,5 @@
>                  Text updates are passed on, so the Java text can shadow the
>                    Gecko text
>              */
>              AutoIMEMask selMask(mIMEMaskSelectionUpdate);
> +            auto composition(GetIMEComposition());

TextComposition is mostly const-correct, we could declare this one const.

@@ +1926,5 @@
>                    temporary events are not passed on to Java
>              */
>              AutoIMEMask selMask(mIMEMaskSelectionUpdate);
>              AutoIMEMask textMask(mIMEMaskTextUpdate);
> +            auto composition(GetIMEComposition());

Const.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Addressed the review comments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee5e3a5f27c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b49b2e9529
https://hg.mozilla.org/integration/mozilla-inbound/rev/660f813d551e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee5e3a5f27c1
https://hg.mozilla.org/mozilla-central/rev/b1b49b2e9529
https://hg.mozilla.org/mozilla-central/rev/660f813d551e