Closed Bug 627019 Opened 13 years ago Closed 13 years ago

Letters are replaced when typing fast

Categories

(Firefox for Android Graveyard :: General, defect, P2)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect

Tracking

(firefox8 fixed, firefox9 fixed, fennec+)

Status:
VERIFIED FIXED
Milestone:
Firefox 8
Tracking Flags:
Tracking Status
firefox8 --- fixed
firefox9 --- fixed
fennec + ---

People

(Reporter: ioana.chiorean, Assigned: snorp)

References

Details

(Keywords: inputmethod, Whiteboard: [hkb][qa-])

Attachments

(1 file, 2 obsolete files)

Bug 627019 - ignore IME changes from gecko when other changes are still pending
5.54 KB, patch
snorp
: review+
mfinkle
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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.
Severity: normal → major
Priority: -- → P2
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.
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.
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
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
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
(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 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+
Attachment #553577 - Attachment is obsolete: true
Attachment #553843 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/4cdc35309098
Status: NEW → RESOLVED
Closed: 13 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.
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?
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: