825120 - Previous composition is not cleared when changing focus from old to new input

Status: RESOLVED FIXED

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

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

User Agent: Mozilla/5.0 (Android; Tablet; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20121219073110

Steps to reproduce:

on the bugzilla helper, enter "hanging" in the third textarea. click in the second textarea. enter f


Actual results:

"hangingf" appears in second texrarea


Expected results:

f should appear in second area

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Please try Nightly or Aurora builds. bug 805162 is in Fx 19 and fixed the majority of IME issues.

Comment 2

[Aaron Train [:aaronmt]]

Nightly (http://nightly.mozilla.org), Aurora (http://aurora.mozilla.org) for Android. Please report back if the issue is fixed for you using either.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

unfortunately this is still an issue in nightly 2012-12-29. seeing this on a nexus 10 with android 4.2.1

Comment 4

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

Bug 815430 fixed this bug but Bug 821979 regressed it :(

Comment 5

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

Attachment: Fake selection update to force IME hard reset (v1)

Detailed explanations are in the patch's comments.

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 700112 [details] [diff] [review]
Fake selection update to force IME hard reset (v1)

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

Feedback with questions:

::: mobile/android/base/GeckoInputConnection.java
@@ +223,5 @@
> +                    // the selection changes, even if soft-resetting. Offsets here must be
> +                    // different from the previous selection offsets, and -1 seems to be a
> +                    // reasonable, deterministic value
> +                    notifySelectionChange(-1, -1);
> +                }

Is the old notifySelectionChange() code no longer needed for Android < 4.2? If it is still needed, can we use the say notifySelectionChange(-1, -1) code to clear the selection (so we are running the same code on all Android versions)?

::: mobile/android/base/InputMethods.java
@@ +21,4 @@
>      public static final String METHOD_ATOK = "com.justsystems.atokmobile.service/.AtokInputMethodService";
>      public static final String METHOD_GOOGLE_JAPANESE_INPUT = "com.google.android.inputmethod.japanese/.MozcService";
>      public static final String METHOD_IWNN = "jp.co.omronsoft.iwnnime.ml/.standardcommon.IWnnLanguageSwitcher";
>      public static final String METHOD_OPENWNN_PLUS = "com.owplus.ime.openwnnplus/.OpenWnnJAJP";

Move METHOD_STOCK_LATINIME so this list of IMEs is in alphabetical order.

Comment 7

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

Attachment: Fake selection update to force IME hard reset (v2)

(In reply to Chris Peterson (:cpeterson) from comment #6)
> Comment on attachment 700112 [details] [diff] [review]
> Fake selection update to force IME hard reset (v1)
> 
> Review of attachment 700112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Feedback with questions:
> 
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +223,5 @@
> > +                    // the selection changes, even if soft-resetting. Offsets here must be
> > +                    // different from the previous selection offsets, and -1 seems to be a
> > +                    // reasonable, deterministic value
> > +                    notifySelectionChange(-1, -1);
> > +                }
> 
> Is the old notifySelectionChange() code no longer needed for Android < 4.2?
> If it is still needed, can we use the say notifySelectionChange(-1, -1) code
> to clear the selection (so we are running the same code on all Android
> versions)?

notifySelectionChange() is used elsewhere in normal cases. This is just a workaround that happens to require notifySelectionChange() too.

The reason I checked for stock 4.2 IME specifically is because
1) This is a workaround for an issue specific to the stock 4.2 IME
2) I'm not sure what effects notifySelectionChange(-1, -1) has on other IMEs. Theoretically it can make an IME crash if the IME doesn't check bounds, but for the stock 4.2 IME it's not an issue.

> ::: mobile/android/base/InputMethods.java
> @@ +21,4 @@
> >      public static final String METHOD_ATOK = "com.justsystems.atokmobile.service/.AtokInputMethodService";
> >      public static final String METHOD_GOOGLE_JAPANESE_INPUT = "com.google.android.inputmethod.japanese/.MozcService";
> >      public static final String METHOD_IWNN = "jp.co.omronsoft.iwnnime.ml/.standardcommon.IWnnLanguageSwitcher";
> >      public static final String METHOD_OPENWNN_PLUS = "com.owplus.ime.openwnnplus/.OpenWnnJAJP";
> 
> Move METHOD_STOCK_LATINIME so this list of IMEs is in alphabetical order.

Okay.

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 700601 [details] [diff] [review]
Fake selection update to force IME hard reset (v2)

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

LGTM now that I understand that the old notifySelectionChange() call is no longer needed for Android < 4.2.

Comment 9

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

Attachment: Fake selection update to force IME hard reset (v3)

I noticed that the bug can still occur if you start typing right after switching focus. This is because we wait 200ms before actually calling restartInput. I changed to patch to call restartInput right away, and ignore subsequent calls within 200ms. This also saves the expense of posting a Runnable to the message queue. I don't know if there will be any side effects from calling restartInput early rather than late, but I don't think the effects will be as bad as this bug.

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 701162 [details] [diff] [review]
Fake selection update to force IME hard reset (v3)

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

LGTM with a comment.

In the future, this mRestartInputEndTime fix could have been its own patch (attached to this same bug). These two fixes are related but independent. I think multi-part patches are easier to review (and if one patch gets r+, you might be able to land it without having to wait for the other patches to get r+).

::: mobile/android/base/GeckoInputConnection.java
@@ +208,3 @@
>              return;
>          }
> +        mRestartInputEndTime = time + 200;

I think the mRestartInputEndTime name might be a little ambiguous. It sounds like "time when restartInput() will finish executing". I can't think of a great name, but maybe we could just remember something like mLastRestartInputTime (since that was the time of a real event) and adding 200 when comparing:

  if (time < mLastRestartInputTime + 200) {
  ...
  mLastRestartInputTime = time;

It is less micro-optimized, but the logic feels a little easier to follow (to me). If *you* prefer mRestartInputEndTime, then please feel free to land this version of the patch. I don't have a strong opinion here. <:)

Comment 11

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

Attachment: Fake selection update to force IME hard reset (v3.1)

(In reply to Chris Peterson (:cpeterson) from comment #10)
> Comment on attachment 701162 [details] [diff] [review]
> Fake selection update to force IME hard reset (v3)
> 
> Review of attachment 701162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM with a comment.
> 
> In the future, this mRestartInputEndTime fix could have been its own patch
> (attached to this same bug). These two fixes are related but independent. I
> think multi-part patches are easier to review (and if one patch gets r+, you
> might be able to land it without having to wait for the other patches to get
> r+).

Fair enough.

> ::: mobile/android/base/GeckoInputConnection.java
> @@ +208,3 @@
> >              return;
> >          }
> > +        mRestartInputEndTime = time + 200;
> 
> I think the mRestartInputEndTime name might be a little ambiguous. It sounds
> like "time when restartInput() will finish executing". I can't think of a
> great name, but maybe we could just remember something like
> mLastRestartInputTime (since that was the time of a real event) and adding
> 200 when comparing:
> 
>   if (time < mLastRestartInputTime + 200) {
>   ...
>   mLastRestartInputTime = time;
> 
> It is less micro-optimized, but the logic feels a little easier to follow
> (to me). If *you* prefer mRestartInputEndTime, then please feel free to land
> this version of the patch. I don't have a strong opinion here. <:)

Changed to mLastRestartInputTime. I agree the logic is more clear.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3e172789ba

Leaving open for nom Aurora

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e3e172789ba

Comment 14

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

Comment on attachment 701245 [details] [diff] [review]
Fake selection update to force IME hard reset (v3.1)

[Approval Request Comment]

Bug caused by (feature/regressing bug #): N/A

User impact if declined: Unpleasant user experience on Android 4.2 when inputting text

Testing completed (on m-c, etc.): Locally, m-c

Risk to taking this patch (and alternatives if risky): Small risk; patch changes previous behavior but the impact of the change is small

String or UUID changes made by this patch: None

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/289fc4462db5