751864 - Typing character followed by space adds the same character another time on ICS

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

I could reproduce this on the Samsung Galaxy Nexus, not on the HTC Desire HD (where there is also some IME enabled).

Steps to reproduce:
- Go to a page with some text input, focus it
- With the vkb, type 'a'
- Type a space

Expected result:
- Space character shown

Actual result:
- Another 'a' shown, then the space character

I can't reproduce this on an Aurora build from 2012-04-02, so looks like a regression.

Comment 1

[Chris Peterson [:cpeterson]]

I can repro on my Galaxy Nexus, but not Galaxy Note. I suspect this is a regression from bug 743468. If so, I would expect the regression to show up in build 2012-04-27 of both Nightly and Aurora.

Comment 2

[Chris Peterson [:cpeterson]]

I think this bug should be considered for beta blocker status. This is a bad regression, but it "only" affects Galaxy Nexus.

Comment 3

[Chris Peterson [:cpeterson]]

On further thought, this sounds more like a 1.0 blocker than a beta blocker.

The bug only affects Galaxy Nexus and only occurs when entering a single letter followed by space in a text form (not AwesomeBar). So typing "a " produces "aa ", but typing "ab " correctly produces "ab ".

A workaround is to just delete the second letter with backspace and press space again. Pressing space the second time does not produces a duplicate letter.

Comment 4

[Catalin Suciu [:csuciu]]

This issue doesn't occur on:
Nightly 15.0a1 (2012-04-27)
http://hg.mozilla.org/mozilla-central/rev/450d8cd16316

but it occurs on:
Nightly 15.0a1 (2012-04-28)
http://hg.mozilla.org/mozilla-central/rev/b5c0d6abf69b

Possible range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=450d8cd16316&tochange=b5c0d6abf69b

Comment 5

[Martijn Wargers (dead)]

Ok, that makes this a regression from bug 743468.

Comment 6

[Chris Peterson [:cpeterson]]

QA, does this bug happen on ICS devices other than Galaxy Nexus?

Comment 7

[Chris Peterson [:cpeterson]]

I think this bug is ICS-specific, not just the Galaxy Nexus. Bug 752924 reports the same problem on a Galaxy SII when running ICS.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 743468 has been backed out of aurora, but this bug needs to be fixed soon if we want bug 743468 to make Fx14

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: bug-751864-end-composition.patch

End composition in onTextChanged() only if we are currently committing. This patch revises (backed out) bug 743468 to only end the composition if we just started it AND we are currently committing.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 624314 [details] [diff] [review]
bug-751864-end-composition.patch

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

want to hear about why we're not throwing

::: mobile/android/base/GeckoInputConnection.java
@@ +143,5 @@
>  
>      @Override
>      public boolean commitText(CharSequence text, int newCursorPosition) {
> +        if (mCommittingText) {
> +            Log.e(LOGTAG, "commitText, but already committing text?!", new IllegalStateException());

1) should this throw?
2) don't use curly brackets

@@ +630,2 @@
>              endComposition();
> +        }

don't use curly brackets

@@ +671,5 @@
>      private void endComposition() {
>          if (DEBUG) Log.d(LOGTAG, "IME: endComposition: IME_COMPOSITION_END");
> +
> +        if (!hasCompositionString()) {
> +           Log.e(LOGTAG, "endComposition, but not composing text?!", new IllegalStateException());

same as above

Comment 12

[Chris Peterson [:cpeterson]]

(In reply to Brad Lassey [:blassey] from comment #11)
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +143,5 @@
> >  
> >      @Override
> >      public boolean commitText(CharSequence text, int newCursorPosition) {
> > +        if (mCommittingText) {
> > +            Log.e(LOGTAG, "commitText, but already committing text?!", new IllegalStateException());
> 
> 1) should this throw?

I logged the current stack trace (using an exception) because this would be an unexpected event, worthy of a bug report but not serious enough to crash the browser with an exception. I'm adding this debug code to codify my understanding of wacky IME state changes triggered by the Android IMEs I'm testing.

Keyboard input might go haywire, but the browser would survive. The logged stack trace would make this error stand out in logs from beta testers or when I'm watching adb logcat.

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: bug-751864-end-composition-v2.patch

patch v2 removes curly brackets from single-line if statements.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

> I logged the current stack trace (using an exception) because this would be
> an unexpected event, worthy of a bug report but not serious enough to crash
> the browser with an exception. I'm adding this debug code to codify my
> understanding of wacky IME state changes triggered by the Android IMEs I'm
> testing.
> 
> Keyboard input might go haywire, but the browser would survive. The logged
> stack trace would make this error stand out in logs from beta testers or
> when I'm watching adb logcat.

ok add that to the log then ("please report this bug")

Comment 15

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43aa1f392da4

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 624602 [details] [diff] [review]
bug-751864-end-composition-v2.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Galaxy Nexus users will have text input problems.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Android only.
String or UUID changes made by this patch: None

Comment 17

[Chris Peterson [:cpeterson]]

oops, I mistakenly list bug 751513 in THIS bug's commit message.

http://hg.mozilla.org/mozilla-central/rev/43aa1f392da4

Comment 18

[JP Rosevear [:jpr]]

Comment on attachment 624602 [details] [diff] [review]
bug-751864-end-composition-v2.patch

Please renom when bug 743468 fallout is completely fixed.

Comment 19

[Chris Peterson [:cpeterson]]

Comment on attachment 624602 [details] [diff] [review]
bug-751864-end-composition-v2.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 743468
User impact if declined: 
Testing completed (on m-c, etc.): m-c for two weeks
Risk to taking this patch (and alternatives if risky): v2 of this patch, in conjunction with my fix for bug 756429, should fix the earlier IME regressions. Patch v2 has been on m-c for two weeks and we really need to beta test it. Android Java only. blocking-fennec1.0=+
String or UUID changes made by this patch: N/A

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 624602 [details] [diff] [review]
bug-751864-end-composition-v2.patch

[Triage Comment]
Approved for Beta 14 (post merge), since this is blocking Fennec release and poses no risk to desktop.

Comment 21

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da68f7ed63d5

Comment 22

[Andreea Pod]

This is not reproducible anymore following the steps from description.

Build: Firefox 16.0a1 (2012-07-03), Firefox 15.0a2 (2012-07-03), Firefox 14.0 (Beta 10)
Device: Galaxy Nexus 
OS: Android 4.0.2