Closed Bug 815430 Opened 9 years ago Closed 9 years ago

Typing on one text input, then going to another input, then typing causes previous text to appear in other text input

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 20

People

(Reporter: martijn.martijn, Assigned: jchen)

References

()

Details

(Keywords: testcase)

Attachments

(1 file, 1 obsolete file)

This was tested on the Galaxy Nexus, Android 4.2, using trunk build.

Steps to reproduce:
- Go to http://people.mozilla.org/~mwargers/tests/forms/textinput/inputs.html
- In the first text input, tap on it, type 'abc'
- Tap on the second text input, type 'd'

Expected result:
- 'd' shows up

Actual result:
- 'abcd' shows up
This is not a regression.
I cannot reproduce on Galaxy Nexus 4.1, using stock and swype keyboards, and using Nightly and Aurora.

Can you reproduce on Aurora?
Assignee: nobody → nchen
Flags: needinfo?(martijn.martijn)
OS: Windows 7 → Android
Hardware: x86 → ARM
Yes, I can also reproduce on Aurora.
I'm using the stock android vkb, not the Swype keyboard, fyi.
Flags: needinfo?(martijn.martijn)
This patch restructures some of the old status notification code and gets rid of the timer which was a source of threading issues. Patch also fixes the symptoms of this bug on Android 4.2.
Attachment #688326 - Flags: review?(cpeterson)
Comment on attachment 688326 [details] [diff] [review]
Fix IME status notification handling (v1)

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

Looking good, but I have some questions:

::: mobile/android/base/GeckoInputConnection.java
@@ -258,5 @@
> -            mBatchEditCount = 0;
> -        }
> -
> -        removeComposingSpans(getEditable());
> -        mUpdateRequest = null;

Do you need set `mUpdateRequest = null` somewhere, such as restartInput()? Or is it OK to leave the most recent mUpdateRequest hanging around?

@@ +520,3 @@
>                  }
> +            }
> +        }, 200); // Delay 200ms to prevent repeated IME showing/hiding

Won't you still get repeated IME showing/hiding events, just delayed 200 ms? The IMEStateUpdater saved the future mIMEState in a static variable so subsequent show/hide events would reschedule the IMEStateUpdater, attempting to coalesce the events.
Attachment #688326 - Flags: review?(cpeterson) → feedback+
(In reply to Chris Peterson (:cpeterson) from comment #5)
> ::: mobile/android/base/GeckoInputConnection.java
> @@ -258,5 @@
> > -            mBatchEditCount = 0;
> > -        }
> > -
> > -        removeComposingSpans(getEditable());
> > -        mUpdateRequest = null;
> 
> Do you need set `mUpdateRequest = null` somewhere, such as restartInput()?
> Or is it OK to leave the most recent mUpdateRequest hanging around?

Good call! I think adding that to the focus change handler is most appropriate. I also forgot to delete remnants of the timer code so I added that to this patch.

> @@ +520,3 @@
> >                  }
> > +            }
> > +        }, 200); // Delay 200ms to prevent repeated IME showing/hiding
> 
> Won't you still get repeated IME showing/hiding events, just delayed 200 ms?
> The IMEStateUpdater saved the future mIMEState in a static variable so
> subsequent show/hide events would reschedule the IMEStateUpdater, attempting
> to coalesce the events.

The callback routine checks mIMEState, so only the most current show/hide status is used. You're right though that the code might be calling show a bunch of time or hide a bunch of times. But I don't think that has any significant consequences.
Attachment #688326 - Attachment is obsolete: true
Attachment #688466 - Flags: review?(cpeterson)
Comment on attachment 688466 [details] [diff] [review]
Fix IME status notification handling (v2)

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

LGTM!
Attachment #688466 - Flags: review?(cpeterson) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5632adeaa4b8
Flags: in-testsuite-
Target Milestone: --- → Firefox 20
https://hg.mozilla.org/mozilla-central/rev/5632adeaa4b8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 821229
Blocks: 825120
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.