The default bug view has changed. See this FAQ.

Letters are replaced when typing fast

VERIFIED FIXED in Firefox 8

Status

Fennec Graveyard
General
P2
major
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Ioana Chiorean, Assigned: snorp)

Tracking

({inputmethod})

Trunk
Firefox 8
ARM
Android
inputmethod

Details

(Whiteboard: [hkb][qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Severity: normal → major
Priority: -- → P2
Whiteboard: [hkb]

Comment 1

6 years ago
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.
(Reporter)

Comment 2

6 years ago
Build id : Mozilla/5.0 (Android;Linux armv7l;rv:2.2a1pre)Gecko/20110411
Firefox/4.2a1pre Fennec /4.1a1pre

Still reproducing.
Related to Bug 634555. Users confirm that cursor still jumps when typing quickly, especially on devices with hardware keyboards, like the HTC Desire Z.

Updated

6 years ago
tracking-firefox7: --- → ?
Duplicate of this bug: 672339
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

Updated

6 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → +
tracking-firefox7: ? → ---
Summary: Letters are replace when typing fast → Letters are replaced when typing fast
Yeah, I can reproduce the wonky insertion order. Taking.
Assignee: nobody → snorp
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
Created attachment 553577 [details] [diff] [review]
Bug 627019 - ignore IME changes from gecko when other changes are still pending
Attachment #553577 - Flags: review?(nchen)
The attached patch is likely not a perfect solution, but it vastly improves things for me.
Keywords: inputmethod
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
Created attachment 553843 [details] [diff] [review]
Bug 627019 - ignore IME changes from gecko when other changes are still pending
(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.
Attachment #553843 - Flags: review?(nchen)
Attachment #553577 - Flags: review?(nchen)
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!
Attachment #553843 - Flags: review?(nchen) → review+
Created attachment 553865 [details] [diff] [review]
Bug 627019 - ignore IME changes from gecko when other changes are still pending
Keywords: checkin-needed
Attachment #553577 - Attachment is obsolete: true
Attachment #553843 - Attachment is obsolete: true
Attachment #553865 - Flags: review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/4cdc35309098
Keywords: checkin-needed
Whiteboard: [hkb] → [hkb][inbound]
http://hg.mozilla.org/mozilla-central/rev/4cdc35309098
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Ioana/Andreea, I never ran into this. Can you verify this?
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

6 years ago
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.
Attachment #553865 - Flags: approval-mozilla-aurora?
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).
Attachment #553865 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [hkb][inbound] → [hkb]
Andreea, can you verify the fix in the landed patch on Aurora?

Comment 22

6 years ago
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
I just pushed this to aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/37d1ce72cc35

Please re-verify tomorrow. Thanks.
status-firefox8: --- → fixed
status-firefox9: --- → fixed

Comment 24

6 years ago
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)
Status: RESOLVED → VERIFIED
Target Milestone: Firefox 9 → Firefox 8
QA not tracking; has been sufficiently tested and verified already.
Whiteboard: [hkb] → [hkb][qa-]
You need to log in before you can comment on or make changes to this bug.