Closed Bug 751864 Opened 9 years ago Closed 9 years ago

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

Categories

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

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +

People

(Reporter: martijn.martijn, Assigned: cpeterson)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → cpeterson
blocking-fennec1.0: --- → ?
I think this bug should be considered for beta blocker status. This is a bad regression, but it "only" affects Galaxy Nexus.
Depends on: 743468
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.
Ok, that makes this a regression from bug 743468.
Blocks: 743468
No longer depends on: 743468
QA, does this bug happen on ICS devices other than Galaxy Nexus?
Keywords: qawanted
blocking-fennec1.0: ? → +
No longer blocks: 743468
Depends on: 743468
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.
Keywords: qawanted
Summary: Typing character followed by space adds the same character another time on Samsung Galaxy Nexus → Typing character followed by space adds the same character another time on ICS
Duplicate of this bug: 752924
bug 743468 has been backed out of aurora, but this bug needs to be fixed soon if we want bug 743468 to make Fx14
Attached patch bug-751864-end-composition.patch (obsolete) — Splinter Review
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.
Attachment #624314 - Flags: review?(blassey.bugs)
Status: NEW → ASSIGNED
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
Attachment #624314 - Flags: review?(blassey.bugs) → review-
(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.
patch v2 removes curly brackets from single-line if statements.
Attachment #624314 - Attachment is obsolete: true
Attachment #624602 - Flags: review?(blassey.bugs)
> 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")
Attachment #624602 - Flags: review?(blassey.bugs) → review+
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
Attachment #624602 - Flags: approval-mozilla-aurora?
oops, I mistakenly list bug 751513 in THIS bug's commit message.

http://hg.mozilla.org/mozilla-central/rev/43aa1f392da4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Comment on attachment 624602 [details] [diff] [review]
bug-751864-end-composition-v2.patch

Please renom when bug 743468 fallout is completely fixed.
Attachment #624602 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Attachment #624602 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora?
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.
Attachment #624602 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Blocks: 743468
No longer depends on: 743468
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.