Last Comment Bug 627019 - Letters are replaced when typing fast
: Letters are replaced when typing fast
Status: VERIFIED FIXED
[hkb][qa-]
: inputmethod
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: P2 major (vote)
: Firefox 8
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
:
Mentors:
: 672339 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-19 07:28 PST by Ioana Chiorean
Modified: 2011-09-09 15:03 PDT (History)
16 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Bug 627019 - ignore IME changes from gecko when other changes are still pending (3.38 KB, patch)
2011-08-16 14:00 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Bug 627019 - ignore IME changes from gecko when other changes are still pending (4.04 KB, patch)
2011-08-17 11:13 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
nchen: review+
Details | Diff | Splinter Review
Bug 627019 - ignore IME changes from gecko when other changes are still pending (5.54 KB, patch)
2011-08-17 12:14 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
snorp: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Ioana Chiorean 2011-01-19 07:28:55 PST
Build Identifier: Mozilla/5.0 (Android; Linux armv7l; en-US; rv:2.0b10pre)
Gecko/20110119 Firefox/4.0b10pre Fennec/4.0b4pre 
Device: HTC Desire A8181
OS: Android 2.1

Steps to Reproduce:
1.enter bugzilla.mozilla.org
2.tap the search upper field 
3.when entering letters press fas for ex: fghjhgjghgfjh (letters close one to
each other)

Expected Results:  
letters should be added at the end, not replacing other letters

Actual Results:  
some letters are replace by the next ones after they are typed 

please take into account that bug https://bugzilla.mozilla.org/show_bug.cgi?id=599550 is resolved fixed.
Comment 1 Andreea Pod 2011-03-08 05:28:43 PST
Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110307 Firefox/4.0b13pre Fennec /4.0b6pre

This is still reproducible when typing fast enough.
Comment 2 Ioana Chiorean 2011-04-11 08:16:36 PDT
Build id : Mozilla/5.0 (Android;Linux armv7l;rv:2.2a1pre)Gecko/20110411
Firefox/4.2a1pre Fennec /4.1a1pre

Still reproducing.
Comment 3 Thomas Arend [:tarend] 2011-06-22 09:11:27 PDT
Related to Bug 634555. Users confirm that cursor still jumps when typing quickly, especially on devices with hardware keyboards, like the HTC Desire Z.
Comment 4 Kevin Brosnan [:kbrosnan] 2011-07-18 16:03:40 PDT
*** Bug 672339 has been marked as a duplicate of this bug. ***
Comment 5 Richard Soderberg [:atoll] 2011-07-18 16:32:32 PDT
hi from bug 672339, i'm seeing letters *inserted* in the wrong place, so all letters end up being present, but in completely bogus order
Comment 6 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-10 11:44:23 PDT
Yeah, I can reproduce the wonky insertion order. Taking.
Comment 7 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-11 13:01:13 PDT
Log of IME events when I quickly type 'asdf'.  The output ends up as adfs.

I/GeckoInputConnection(23006): IME_SET_SELECTION chars=a s=0 c=1 oc=0
I/Gecko   (23006): AndroidGeckoEvent: 0x4057cd68 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057a3b0 : 6
I/Gecko   (23006): IME: IME_SET_SELECTION: o=0, l=0
I/Gecko   (23006): AndroidGeckoEvent: 0x405ffbf0 : 6
I/Gecko   (23006): IME: IME_COMPOSITION_BEGIN
I/Gecko   (23006): AndroidGeckoEvent: 0x4057d630 : 6
I/Gecko   (23006): IME: IME_SET_TEXT: l=1, r=1
I/Gecko   (23006): AndroidGeckoEvent: 0x4063aeb0 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x40604010 : 15
I/Gecko   (23006): IME: IME_COMPOSITION_END
I/Gecko   (23006): IME: IME_SET_SELECTION: o=1, l=0
I/GeckoInputConnection(23006): IME_SET_SELECTION chars=as s=1 c=1 oc=0
I/Gecko   (23006): AndroidGeckoEvent: 0x40609308 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057f7f8 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057f888 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057b6b8 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057b748 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x40581138 : 15
I/Gecko   (23006): IME: OnIMETextChange: s=0, oe=0, ne=1
I/GeckoAppJava(23006): IME: notifyTextChange: text=a s=0 e=1 oe=0
I/Gecko   (23006): IME: OnIMESelectionChange
I/GeckoAppJava(23006): IME: notifySelectionChange: s=1 e=1
I/Gecko   (23006): IME: IME_SET_SELECTION: o=1, l=0
I/Gecko   (23006): IME: IME_COMPOSITION_BEGIN
I/Gecko   (23006): IME: IME_SET_TEXT: l=1, r=1
I/Gecko   (23006): IME: IME_COMPOSITION_END
I/Gecko   (23006): IME: IME_SET_SELECTION: o=2, l=0
I/GeckoInputConnection(23006): IME_SET_SELECTION chars=ad s=1 c=1 oc=0
I/Gecko   (23006): AndroidGeckoEvent: 0x4057b2a8 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057b338 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057b490 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057d1e8 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x4057d278 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x40660968 : 15
I/Gecko   (23006): IME: OnIMETextChange: s=1, oe=1, ne=2
I/GeckoAppJava(23006): IME: notifyTextChange: text=as s=1 e=2 oe=1
I/Gecko   (23006): IME: OnIMESelectionChange
I/GeckoAppJava(23006): IME: notifySelectionChange: s=2 e=2
I/Gecko   (23006): IME: IME_SET_SELECTION: o=1, l=0
I/Gecko   (23006): IME: IME_COMPOSITION_BEGIN
I/Gecko   (23006): IME: IME_SET_TEXT: l=1, r=1
I/Gecko   (23006): IME: IME_COMPOSITION_END
I/Gecko   (23006): IME: IME_SET_SELECTION: o=2, l=0
I/GeckoInputConnection(23006): IME_SET_SELECTION chars=asf s=2 c=1 oc=0
I/Gecko   (23006): AndroidGeckoEvent: 0x40605768 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x406057f8 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x40605888 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x406044c0 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x40604550 : 6
I/Gecko   (23006): AndroidGeckoEvent: 0x40604600 : 15
I/Gecko   (23006): IME: OnIMETextChange: s=1, oe=1, ne=2
I/GeckoAppJava(23006): IME: notifyTextChange: text=ads s=1 e=2 oe=1
I/Gecko   (23006): IME: OnIMESelectionChange
I/GeckoAppJava(23006): IME: notifySelectionChange: s=2 e=2
I/Gecko   (23006): IME: IME_SET_SELECTION: o=2, l=0
I/Gecko   (23006): IME: IME_COMPOSITION_BEGIN
I/Gecko   (23006): IME: IME_SET_TEXT: l=1, r=1
I/Gecko   (23006): IME: IME_COMPOSITION_END
I/Gecko   (23006): IME: IME_SET_SELECTION: o=3, l=0
I/Gecko   (23006): IME: OnIMETextChange: s=2, oe=2, ne=3
I/GeckoAppJava(23006): IME: notifyTextChange: text=adfs s=2 e=3 oe=2
I/Gecko   (23006): AndroidGeckoEvent: 0x40580c18 : 1
I/Gecko   (23006): IME: OnIMESelectionChange
I/GeckoAppJava(23006): IME: notifySelectionChange: s=3 e=3
I/Gecko   (23006): AndroidGeckoEvent: 0x40580e90 : 1
I/Gecko   (23006): AndroidGeckoEvent: 0x40580f68 : 1
I/Gecko   (23006): AndroidGeckoEvent: 0x40581040 : 1
Comment 8 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-16 14:00:03 PDT
Created attachment 553577 [details] [diff] [review]
Bug 627019 - ignore IME changes from gecko when other changes are still pending
Comment 9 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-16 14:01:37 PDT
The attached patch is likely not a perfect solution, but it vastly improves things for me.
Comment 10 Jim Chen [:jchen] [:darchons] 2011-08-17 10:28:00 PDT
Comment on attachment 553577 [details] [diff] [review]
Bug 627019 - ignore IME changes from gecko when other changes are still pending

Looks good. Thanks! Can you add a few comments explaining why we need mNumPendingChanges? Basically the problem is notifySelectionChange doesn't always have the latest selection indices, right?

> 
>+        // Log.d("GeckoAppShell", String.format("notifyIMEChange: t=%s s=%d ne=%d oe=%d", text, start, newEnd, end));
>+

nit: if you want this to stay, can you change to match the other ones? i.e. 'GeckoAppJava' tag and 'IME: xxx' message

> 
>-        if (!text.contentEquals(GeckoApp.surfaceView.mEditable))
>+        mNumPendingChanges = Math.max(mNumPendingChanges - 1, 0);
>+        if (mNumPendingChanges == 0 && !text.contentEquals(GeckoApp.surfaceView.mEditable)) {
>             GeckoApp.surfaceView.setEditable(text);
>+        }

more nit: i think the general style is to omit braces for single statements

>+
>+        if (mNumPendingChanges > 0) {
>+            return;
>+        }
> 
>         if (mComposing)
>             imm.updateSelection(GeckoApp.surfaceView,

same as above

also can you test to see if it's necessary to skip imm.updateSelection when mNumPendingChanges > 0? AFAIK updateSelection only deals with updating the displayed cursor of any fullscreen textarea that Android shows. i think if we don't skip over imm.updateExtractedText in notifyTextChange, we shouldn't skip over imm.updateSelection.

>     {
>+        //Log.d("GeckoInputConnection", String.format("onTextChanged: t=%s s=%d b=%d l=%d",
>+        //                                            s, start, before, count));

same as above
Comment 11 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-17 11:13:58 PDT
Created attachment 553843 [details] [diff] [review]
Bug 627019 - ignore IME changes from gecko when other changes are still pending
Comment 12 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-17 11:15:29 PDT
(In reply to Jim Chen [:jchen] (mobile intern :) from comment #10)
> Comment on attachment 553577 [details] [diff] [review]
> Bug 627019 - ignore IME changes from gecko when other changes are still
> pending
> 
> Looks good. Thanks! Can you add a few comments explaining why we need
> mNumPendingChanges? Basically the problem is notifySelectionChange doesn't
> always have the latest selection indices, right?

Right.  Added some comments to that effect.

> 
> > 
> >+        // Log.d("GeckoAppShell", String.format("notifyIMEChange: t=%s s=%d ne=%d oe=%d", text, start, newEnd, end));
> >+
> 
> nit: if you want this to stay, can you change to match the other ones? i.e.
> 'GeckoAppJava' tag and 'IME: xxx' message
> 

Done.

> > 
> >-        if (!text.contentEquals(GeckoApp.surfaceView.mEditable))
> >+        mNumPendingChanges = Math.max(mNumPendingChanges - 1, 0);
> >+        if (mNumPendingChanges == 0 && !text.contentEquals(GeckoApp.surfaceView.mEditable)) {
> >             GeckoApp.surfaceView.setEditable(text);
> >+        }
> 
> more nit: i think the general style is to omit braces for single statements
> 

Done.

> >+
> >+        if (mNumPendingChanges > 0) {
> >+            return;
> >+        }
> > 
> >         if (mComposing)
> >             imm.updateSelection(GeckoApp.surfaceView,
> 
> same as above
> 

Done.

> also can you test to see if it's necessary to skip imm.updateSelection when
> mNumPendingChanges > 0? AFAIK updateSelection only deals with updating the
> displayed cursor of any fullscreen textarea that Android shows. i think if
> we don't skip over imm.updateExtractedText in notifyTextChange, we shouldn't
> skip over imm.updateSelection.
> 

Indeed, we don't need to skip the imm.updateSelection calls.  Moved the mNumPendingChanges block below that.

> >     {
> >+        //Log.d("GeckoInputConnection", String.format("onTextChanged: t=%s s=%d b=%d l=%d",
> >+        //                                            s, start, before, count));
> 
> same as above

Done.
Comment 13 Jim Chen [:jchen] [:darchons] 2011-08-17 11:58:39 PDT
Comment on attachment 553843 [details] [diff] [review]
Bug 627019 - ignore IME changes from gecko when other changes are still pending

> 
>+        // Log.d("GeckoAppShell", String.format("IME: notifyIMEChange: t=%s s=%d ne=%d oe=%d",
>+        //                                      text, start, newEnd, end));
>+
>         if (newEnd < 0)

Sorry, I meant also the tag should be 'GeckoAppJava', not 'GeckoAppShell'

Don't forget to generate a Mozilla format patch. Thanks!
Comment 14 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-17 12:14:30 PDT
Created attachment 553865 [details] [diff] [review]
Bug 627019 - ignore IME changes from gecko when other changes are still pending
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2011-08-17 12:35:02 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/4cdc35309098
Comment 16 Marco Bonardo [::mak] 2011-08-18 03:55:41 PDT
http://hg.mozilla.org/mozilla-central/rev/4cdc35309098
Comment 17 Aaron Train [:aaronmt] 2011-08-18 06:57:34 PDT
Ioana/Andreea, I never ran into this. Can you verify this?
Comment 18 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-18 06:58:39 PDT
FWIW, the best way I found to reproduce this is to plug in a standard USB keyboard to any capable device (my tablet has a full-size USB port) and type 'asdf' quickly.
Comment 19 Andreea Pod 2011-08-22 06:29:36 PDT
It works fine on Nightly: Mozilla /5.0 (Android;Linux armv7l;rv:9.0a1) Gecko/20110817 Firefox/9.0a1 Fennec/9.0a1

But I can reproduce the jumping cursor like in comment 3 with HTC Desire Z on Aurora: Mozilla /5.0 (Android;Linux armv7l;rv:8.0a2) Gecko/20110821 Firefox/8.0a2 Fennec/8.0a2. You can see a screencast here: http://www.youtube.com/watch?v=5OmbCRqh_HE, sorry for the orientation and hope you can understand what is happening.
Comment 20 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-08-22 07:05:45 PDT
This patch missed the Aurora switchover, but it would be nice to have now.  It's fairly low risk, IMHO, and the benefit for physical keyboard users is pretty big (as in, it works).
Comment 21 Aaron Train [:aaronmt] 2011-08-27 18:21:04 PDT
Andreea, can you verify the fix in the landed patch on Aurora?
Comment 22 Andreea Pod 2011-08-29 08:00:04 PDT
I can still reproduce the jumping cursor. I made another video,hope it's better: http://www.youtube.com/watch?v=xEuuvpepPA0&feature=channel_video_title.

Mozilla /5.0 (Android;Linux armv7l;rv:8.0a2) Gecko/20110829 Firefox/8.0a2 Fennec/8.0a2
Device : HTC Desire Z
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-29 08:06:50 PDT
I just pushed this to aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/37d1ce72cc35

Please re-verify tomorrow. Thanks.
Comment 24 Andreea Pod 2011-08-30 07:38:37 PDT
Verified fixed on: Mozilla /5.0 (Android;Linux armv7l;rv:8.0a2) Gecko/20110830 Firefox/8.0a2 Fennec/8.0a2
Device : HTC Desire Z (Android 2.3)
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-09 15:03:59 PDT
QA not tracking; has been sufficiently tested and verified already.

Note You need to log in before you can comment on or make changes to this bug.