Closed Bug 1133802 Opened 5 years ago Closed 5 years ago

Regression: duplicate 'i' is committed on input

Categories

(Firefox for Android :: Keyboards and IME, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox37 --- unaffected
firefox38 + fixed
firefox39 --- verified
fennec 38+ ---

People

(Reporter: tenisthenewnine, Assigned: esawin)

References

()

Details

(Keywords: inputmethod, regression, reproducible)

Attachments

(3 files, 2 obsolete files)

Hi guys,

Since about a week ago's nightly Firefox started doing a very frustrating thing.

When I type a sentence with words starting with the letter i it comes out like this:

I love iicecream and I live on an iisland iin Europe.

It's only Firefox that this happens, and its only the letter i! It's almost like I'm completing the word and once I hit space it adds this extra character.

How can I test this further? It does it without addons for me as well.
Component: Untriaged → Keyboards and IME
Product: Firefox → Firefox for Android
What device? What keyboard? What Android version?
Galaxy Note 3. Android Lollipop, stock keyboard.
We have not had any bugs fixed in IME code in the last week or two.
(In reply to Aaron Train [:aaronmt] from comment #3)
> We have not had any bugs fixed in IME code in the last week or two.

Maybe it's been since a bit longer than that. Anything the week before? 

I know this is one of those tricky bugs to diagnose...
[Tracking Requested - why for this release]: regression 

I can reproduce with the Samsung keyboard on a S5 running Android 5.0.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Keywords: regression
Any ideas?
Flags: needinfo?(esawin)
I can't reproduce on my S5 on 4.4, for some reason it's failing to update to 5.0 properly. I guess Kevin can help with a window.
It's reproducing on Nexus 7 (Android 4.4) and Nexus 5 (Android 5) with the Google keyboard.
It looks like we are prematurely committing the auto correction (i -> I), since I is the only one-letter English word that is only valid as a capital letter.

A regression range would be great to have, this one isn't connected to the other IME bugs I'm looking into.
Flags: needinfo?(esawin)
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0351fbdeb37c&tochange=bd14f3a87117

Wed Feb 11 03:38:22 2015 +0000	bd14f3a87117	Tooru Fujisawa — Bug 1125934 - Discard redundant NS_COMPOSITION_CHANGE event which is send just before NS_COMPOSITION_END on TSF. r=masayuki
Blocks: 1125934
Flags: needinfo?(masayuki)
Keywords: reproducible
Summary: entering a word starting with i on android causes duplicate i's → Regression: duplicate 'i' is committed on input
Flags: needinfo?(arai.unmht)
It looks like we somehow depend on the redundant NS_COMPOSITION_CHANGE event (however not on the DOM text event) to reset the cursor back after a cancelled auto-correction for single letter words. This seems like an odd edge case for the English "I" word (auto-correction i->I).
Logged events in following position, with and without the patch in bug 1133802:
  https://hg.mozilla.org/mozilla-central/file/1b4c5daa7b7a/dom/events/TextComposition.cpp#l230

With the patch, NS_COMPOSITION_COMMIT_AS_IS is dispatched after discarding redundant event (still not sure why it happens...).
Flags: needinfo?(arai.unmht)
if this issue happens only on android, disabling the patch bug 1125934 on android (or enabling it only on windows) might be a quick workaround, since bug 1125934 happens only on windows.
According to the DOM Events spec, the current behavior is the best. So, I don't like it will be disabled only on Android. Anyway, I don't such #ifdef hack in XP code.

I guess that the reason of this bug is, the IME probably checks if the NS_COMPOSITION_CHANGE event is handled as expected. However, not dispatching any events doesn't cause notifications of text change nor selection change. Therefore, I guess that the IME retries to modify composition and gives up.
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#2090
http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#2100

So, I guess that sending text change notification and/or selection change notification from TextComposition at discarding a composition change event will fix this bug.
Flags: needinfo?(masayuki)
I think we have handled this edge case differently before bug 1125934, except that the redundant composition change event used to trigger a text change notification which invokes a Gecko update due to the increment in mIcUpdateSeqno.

Forcing a Gecko update for the case of no text or selection change in endBatchEdit should fix this issue by setting the range and selection accordingly.
Attachment #8568524 - Flags: feedback?(masayuki)
Attachment #8568524 - Flags: feedback?(cpeterson)
Comment on attachment 8568524 [details] [diff] [review]
0001-Bug-1133802-Force-Gecko-update-on-endBatchEdit-for-r.patch

Review of attachment 8568524 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me.

Boolean parameters are often a "code smell" that a method is doing more than one job. Would calling setUpdateGecko() with update=false and force=true make sense? For extra credit, you might consider collapsing the `update` and `force` booleans into an enum with named values like `Update`, `ForceUpdate`, and `DontUpdate` (or whatever you think makes sense).

::: mobile/android/base/GeckoInputConnection.java
@@ +259,4 @@
>          if (mBatchEditCount > 0) {
>              mBatchEditCount--;
>              if (mBatchEditCount == 0) {
> +                boolean forceUpdate = !mBatchTextChanged && !mBatchSelectionChanged;

Please add a comment explaining why it is necessary to force Gecko to update when neither the text nor selection changed.
Attachment #8568524 - Flags: feedback?(cpeterson) → feedback+
Addressed the issues from the feedback.

I'm not sure whether we prefer bitwise operations on enums or the alternative that I'm using for this case. Also I'm not sure about the enum name, as it doesn't feel like an event type, maybe a flag?
Attachment #8568524 - Attachment is obsolete: true
Attachment #8568524 - Flags: feedback?(masayuki)
Attachment #8568855 - Flags: review?(cpeterson)
Attachment #8568855 - Flags: feedback?(masayuki)
Comment on attachment 8568855 [details] [diff] [review]
0001-Bug-1133802-Force-Gecko-update.patch

Review of attachment 8568855 [details] [diff] [review]:
-----------------------------------------------------------------

r+

On second thought, I think your original approach using two booleans was clearer. The extra enum just seems to complicate the conditional logic instead of making it clearer. Please feel free to land which ever patch version you prefer. Sorry for the detour! :)
Attachment #8568855 - Flags: review?(cpeterson) → review+
Comment on attachment 8568855 [details] [diff] [review]
0001-Bug-1133802-Force-Gecko-update.patch

Although, I'm not familiar with Java, but looks like this attempts to take correct approach. Sorry for the delay.
Attachment #8568855 - Flags: feedback?(masayuki) → feedback+
https://hg.mozilla.org/mozilla-central/rev/e75d824c730d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
tracking-fennec: ? → 38+
Looks good. No known fallout we should get this uplifted to Aurora.
Flags: needinfo?(esawin)
Final patch version (with comments).

Approval Request Comment
[Feature/regressing bug #]: bug 1125934 
[User impact if declined]: typing words starting with 'i' would result in duplicated 'i' (on an English keyboard)
[Describe test coverage new/current, TreeHerder]: has been on Nightly for two weeks
[Risks and why]: low, it just enforces a Gecko update in an edge case to make IME less dependent on redundant event sequences
[String/UUID change made/needed]: none
Attachment #8568855 - Attachment is obsolete: true
Flags: needinfo?(esawin)
Attachment #8576620 - Flags: review+
Attachment #8576620 - Flags: approval-mozilla-aurora?
Tracking since this is a recent regression and we're uplifting it to aurora.
Comment on attachment 8576620 [details] [diff] [review]
0001-Bug-1133802-Force-Gecko-update.patch

iI am approving this for Aurora since it looks low-risk and fixes a recent regression.
Attachment #8576620 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This fixed the issue I was having with Swype as well, except in that case, it was every word being duplicated!
You need to log in before you can comment on or make changes to this bug.