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)
Tracking
(firefox8 fixed, firefox9 fixed, fennec+)
VERIFIED
FIXED
Firefox 8
People
(Reporter: ioana.chiorean, Assigned: snorp)
References
Details
(Keywords: inputmethod, Whiteboard: [hkb][qa-])
Attachments
(1 file, 2 obsolete files)
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.
Updated•13 years ago
|
Severity: normal → major
Priority: -- → P2
Updated•13 years ago
|
Whiteboard: [hkb]
Comment 1•13 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•13 years ago
|
||
Build id : Mozilla/5.0 (Android;Linux armv7l;rv:2.2a1pre)Gecko/20110411 Firefox/4.2a1pre Fennec /4.1a1pre Still reproducing.
Comment 3•13 years ago
|
||
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•13 years ago
|
tracking-firefox7:
--- → ?
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•13 years ago
|
tracking-fennec: ? → +
tracking-firefox7:
? → ---
Summary: Letters are replace when typing fast → Letters are replaced when typing fast
Assignee | ||
Comment 6•13 years ago
|
||
Yeah, I can reproduce the wonky insertion order. Taking.
Assignee: nobody → snorp
Assignee | ||
Comment 7•13 years ago
|
||
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
Assignee | ||
Comment 8•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #553577 -
Flags: review?(nchen)
Assignee | ||
Comment 9•13 years ago
|
||
The attached patch is likely not a perfect solution, but it vastly improves things for me.
Updated•13 years ago
|
Keywords: inputmethod
Comment 10•13 years ago
|
||
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
Assignee | ||
Comment 11•13 years ago
|
||
Assignee | ||
Comment 12•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Attachment #553843 -
Flags: review?(nchen)
Assignee | ||
Updated•13 years ago
|
Attachment #553577 -
Flags: review?(nchen)
Comment 13•13 years ago
|
||
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+
Assignee | ||
Comment 14•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•13 years ago
|
Attachment #553577 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #553843 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #553865 -
Flags: review+
Comment 15•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/4cdc35309098
Keywords: checkin-needed
Whiteboard: [hkb] → [hkb][inbound]
Comment 16•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4cdc35309098
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment 17•13 years ago
|
||
Ioana/Andreea, I never ran into this. Can you verify this?
Assignee | ||
Comment 18•13 years ago
|
||
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•13 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.
Assignee | ||
Updated•13 years ago
|
Attachment #553865 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 20•13 years ago
|
||
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).
Updated•13 years ago
|
Attachment #553865 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Whiteboard: [hkb][inbound] → [hkb]
Comment 21•13 years ago
|
||
Andreea, can you verify the fix in the landed patch on Aurora?
Comment 22•13 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
Comment 23•13 years ago
|
||
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•13 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
Updated•13 years ago
|
Target Milestone: Firefox 9 → Firefox 8
Comment 25•13 years ago
|
||
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.
Description
•