Closed
Bug 825120
Opened 11 years ago
Closed 11 years ago
Previous composition is not cleared when changing focus from old to new input
Categories
(Firefox for Android Graveyard :: Keyboards and IME, defect)
Tracking
(firefox20 fixed, firefox21 fixed)
RESOLVED
FIXED
Firefox 21
People
(Reporter: Biesinger, Assigned: jchen)
References
Details
(Keywords: regression)
Attachments
(1 file, 3 obsolete files)
6.78 KB,
patch
|
jchen
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Android; Tablet; rv:18.0) Gecko/18.0 Firefox/18.0 Build ID: 20121219073110 Steps to reproduce: on the bugzilla helper, enter "hanging" in the third textarea. click in the second textarea. enter f Actual results: "hangingf" appears in second texrarea Expected results: f should appear in second area
Comment 1•11 years ago
|
||
Please try Nightly or Aurora builds. bug 805162 is in Fx 19 and fixed the majority of IME issues.
Comment 2•11 years ago
|
||
Nightly (http://nightly.mozilla.org), Aurora (http://aurora.mozilla.org) for Android. Please report back if the issue is fixed for you using either.
Reporter | ||
Comment 3•11 years ago
|
||
unfortunately this is still an issue in nightly 2012-12-29. seeing this on a nexus 10 with android 4.2.1
Updated•11 years ago
|
Component: General → Keyboards and IME
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 years ago
|
||
Bug 815430 fixed this bug but Bug 821979 regressed it :(
Blocks: 821979
Summary: strange input field behavior in firefox for android → Previous composition is not cleared when changing focus from old to new input
Updated•11 years ago
|
Assignee | ||
Comment 5•11 years ago
|
||
Detailed explanations are in the patch's comments.
Attachment #700112 -
Flags: review?(cpeterson)
Comment 6•11 years ago
|
||
Comment on attachment 700112 [details] [diff] [review] Fake selection update to force IME hard reset (v1) Review of attachment 700112 [details] [diff] [review]: ----------------------------------------------------------------- Feedback with questions: ::: mobile/android/base/GeckoInputConnection.java @@ +223,5 @@ > + // the selection changes, even if soft-resetting. Offsets here must be > + // different from the previous selection offsets, and -1 seems to be a > + // reasonable, deterministic value > + notifySelectionChange(-1, -1); > + } Is the old notifySelectionChange() code no longer needed for Android < 4.2? If it is still needed, can we use the say notifySelectionChange(-1, -1) code to clear the selection (so we are running the same code on all Android versions)? ::: mobile/android/base/InputMethods.java @@ +21,4 @@ > public static final String METHOD_ATOK = "com.justsystems.atokmobile.service/.AtokInputMethodService"; > public static final String METHOD_GOOGLE_JAPANESE_INPUT = "com.google.android.inputmethod.japanese/.MozcService"; > public static final String METHOD_IWNN = "jp.co.omronsoft.iwnnime.ml/.standardcommon.IWnnLanguageSwitcher"; > public static final String METHOD_OPENWNN_PLUS = "com.owplus.ime.openwnnplus/.OpenWnnJAJP"; Move METHOD_STOCK_LATINIME so this list of IMEs is in alphabetical order.
Attachment #700112 -
Flags: review?(cpeterson) → feedback+
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Chris Peterson (:cpeterson) from comment #6) > Comment on attachment 700112 [details] [diff] [review] > Fake selection update to force IME hard reset (v1) > > Review of attachment 700112 [details] [diff] [review]: > ----------------------------------------------------------------- > > Feedback with questions: > > ::: mobile/android/base/GeckoInputConnection.java > @@ +223,5 @@ > > + // the selection changes, even if soft-resetting. Offsets here must be > > + // different from the previous selection offsets, and -1 seems to be a > > + // reasonable, deterministic value > > + notifySelectionChange(-1, -1); > > + } > > Is the old notifySelectionChange() code no longer needed for Android < 4.2? > If it is still needed, can we use the say notifySelectionChange(-1, -1) code > to clear the selection (so we are running the same code on all Android > versions)? notifySelectionChange() is used elsewhere in normal cases. This is just a workaround that happens to require notifySelectionChange() too. The reason I checked for stock 4.2 IME specifically is because 1) This is a workaround for an issue specific to the stock 4.2 IME 2) I'm not sure what effects notifySelectionChange(-1, -1) has on other IMEs. Theoretically it can make an IME crash if the IME doesn't check bounds, but for the stock 4.2 IME it's not an issue. > ::: mobile/android/base/InputMethods.java > @@ +21,4 @@ > > public static final String METHOD_ATOK = "com.justsystems.atokmobile.service/.AtokInputMethodService"; > > public static final String METHOD_GOOGLE_JAPANESE_INPUT = "com.google.android.inputmethod.japanese/.MozcService"; > > public static final String METHOD_IWNN = "jp.co.omronsoft.iwnnime.ml/.standardcommon.IWnnLanguageSwitcher"; > > public static final String METHOD_OPENWNN_PLUS = "com.owplus.ime.openwnnplus/.OpenWnnJAJP"; > > Move METHOD_STOCK_LATINIME so this list of IMEs is in alphabetical order. Okay.
Attachment #700112 -
Attachment is obsolete: true
Attachment #700601 -
Flags: review?(cpeterson)
Comment 8•11 years ago
|
||
Comment on attachment 700601 [details] [diff] [review] Fake selection update to force IME hard reset (v2) Review of attachment 700601 [details] [diff] [review]: ----------------------------------------------------------------- LGTM now that I understand that the old notifySelectionChange() call is no longer needed for Android < 4.2.
Attachment #700601 -
Flags: review?(cpeterson) → review+
Assignee | ||
Comment 9•11 years ago
|
||
I noticed that the bug can still occur if you start typing right after switching focus. This is because we wait 200ms before actually calling restartInput. I changed to patch to call restartInput right away, and ignore subsequent calls within 200ms. This also saves the expense of posting a Runnable to the message queue. I don't know if there will be any side effects from calling restartInput early rather than late, but I don't think the effects will be as bad as this bug.
Attachment #700601 -
Attachment is obsolete: true
Attachment #701162 -
Flags: review?(cpeterson)
Comment 10•11 years ago
|
||
Comment on attachment 701162 [details] [diff] [review] Fake selection update to force IME hard reset (v3) Review of attachment 701162 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with a comment. In the future, this mRestartInputEndTime fix could have been its own patch (attached to this same bug). These two fixes are related but independent. I think multi-part patches are easier to review (and if one patch gets r+, you might be able to land it without having to wait for the other patches to get r+). ::: mobile/android/base/GeckoInputConnection.java @@ +208,3 @@ > return; > } > + mRestartInputEndTime = time + 200; I think the mRestartInputEndTime name might be a little ambiguous. It sounds like "time when restartInput() will finish executing". I can't think of a great name, but maybe we could just remember something like mLastRestartInputTime (since that was the time of a real event) and adding 200 when comparing: if (time < mLastRestartInputTime + 200) { ... mLastRestartInputTime = time; It is less micro-optimized, but the logic feels a little easier to follow (to me). If *you* prefer mRestartInputEndTime, then please feel free to land this version of the patch. I don't have a strong opinion here. <:)
Attachment #701162 -
Flags: review?(cpeterson) → review+
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Chris Peterson (:cpeterson) from comment #10) > Comment on attachment 701162 [details] [diff] [review] > Fake selection update to force IME hard reset (v3) > > Review of attachment 701162 [details] [diff] [review]: > ----------------------------------------------------------------- > > LGTM with a comment. > > In the future, this mRestartInputEndTime fix could have been its own patch > (attached to this same bug). These two fixes are related but independent. I > think multi-part patches are easier to review (and if one patch gets r+, you > might be able to land it without having to wait for the other patches to get > r+). Fair enough. > ::: mobile/android/base/GeckoInputConnection.java > @@ +208,3 @@ > > return; > > } > > + mRestartInputEndTime = time + 200; > > I think the mRestartInputEndTime name might be a little ambiguous. It sounds > like "time when restartInput() will finish executing". I can't think of a > great name, but maybe we could just remember something like > mLastRestartInputTime (since that was the time of a real event) and adding > 200 when comparing: > > if (time < mLastRestartInputTime + 200) { > ... > mLastRestartInputTime = time; > > It is less micro-optimized, but the logic feels a little easier to follow > (to me). If *you* prefer mRestartInputEndTime, then please feel free to land > this version of the patch. I don't have a strong opinion here. <:) Changed to mLastRestartInputTime. I agree the logic is more clear.
Attachment #701162 -
Attachment is obsolete: true
Attachment #701245 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3e172789ba Leaving open for nom Aurora
status-firefox20:
--- → affected
status-firefox21:
--- → fixed
Flags: in-testsuite-
Whiteboard: [leave open]
Updated•11 years ago
|
Target Milestone: --- → Firefox 21
Assignee | ||
Comment 14•11 years ago
|
||
Comment on attachment 701245 [details] [diff] [review] Fake selection update to force IME hard reset (v3.1) [Approval Request Comment] Bug caused by (feature/regressing bug #): N/A User impact if declined: Unpleasant user experience on Android 4.2 when inputting text Testing completed (on m-c, etc.): Locally, m-c Risk to taking this patch (and alternatives if risky): Small risk; patch changes previous behavior but the impact of the change is small String or UUID changes made by this patch: None
Attachment #701245 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #701245 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/289fc4462db5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•