Closed Bug 825120 Opened 11 years ago Closed 11 years ago

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

Categories

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

20 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox20 fixed, firefox21 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- fixed
firefox21 --- fixed

People

(Reporter: Biesinger, Assigned: jchen)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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
Please try Nightly or Aurora builds. bug 805162 is in Fx 19 and fixed the majority of IME issues.
Nightly (http://nightly.mozilla.org), Aurora (http://aurora.mozilla.org) for Android. Please report back if the issue is fixed for you using either.
unfortunately this is still an issue in nightly 2012-12-29. seeing this on a nexus 10 with android 4.2.1
Component: General → Keyboards and IME
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Bug 815430 fixed this bug but Bug 821979 regressed it :(
Blocks: 821979
Summary: strange input field behavior in firefox for android → Previous composition is not cleared when changing focus from old to new input
Depends on: 815430
Keywords: regression
Hardware: Other → ARM
Version: Firefox 18 → Firefox 20
Detailed explanations are in the patch's comments.
Attachment #700112 - Flags: review?(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.
Attachment #700112 - Flags: review?(cpeterson) → feedback+
(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.
Attachment #700112 - Attachment is obsolete: true
Attachment #700601 - Flags: review?(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.
Attachment #700601 - Flags: review?(cpeterson) → review+
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.
Attachment #700601 - Attachment is obsolete: true
Attachment #701162 - Flags: review?(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. <:)
Attachment #701162 - Flags: review?(cpeterson) → review+
(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.
Attachment #701162 - Attachment is obsolete: true
Attachment #701245 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3e172789ba

Leaving open for nom Aurora
Flags: in-testsuite-
Whiteboard: [leave open]
Target Milestone: --- → Firefox 21
Depends on: 830131
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
Attachment #701245 - Flags: approval-mozilla-aurora?
Attachment #701245 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 819073
Depends on: 831862
https://hg.mozilla.org/releases/mozilla-aurora/rev/289fc4462db5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: