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

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

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

Comment 1

[Martijn Wargers (dead)]

This is not a regression.

Comment 2

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

I cannot reproduce on Galaxy Nexus 4.1, using stock and swype keyboards, and using Nightly and Aurora.

Can you reproduce on Aurora?

Comment 3

[Martijn Wargers (dead)]

Yes, I can also reproduce on Aurora.
I'm using the stock android vkb, not the Swype keyboard, fyi.

Comment 4

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

Attachment: Fix IME status notification handling (v1)

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.

Comment 5

[Chris Peterson [: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.

Comment 6

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

Attachment: Fix IME status notification handling (v2)

(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.

Comment 7

[Chris Peterson [:cpeterson]]

Comment on attachment 688466 [details] [diff] [review]
Fix IME status notification handling (v2)

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

LGTM!

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5632adeaa4b8

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5632adeaa4b8