1133802 - Regression: duplicate 'i' is committed on input

Status: RESOLVED FIXED

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

Description

[Grant]

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.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

What device? What keyboard? What Android version?

Comment 2

[Grant]

Galaxy Note 3. Android Lollipop, stock keyboard.

Comment 3

[Aaron Train [:aaronmt]]

We have not had any bugs fixed in IME code in the last week or two.

Comment 4

[Grant]

(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...

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regression 

I can reproduce with the Samsung keyboard on a S5 running Android 5.0.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

Any ideas?

Comment 7

[Aaron Train [:aaronmt]]

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.

Comment 8

[Eugen Sawin [:esawin]]

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.

Comment 9

[Aaron Train [:aaronmt]]

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

Comment 10

[Eugen Sawin [:esawin]]

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).

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: event log without patch in bug 1125934

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...).

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: event log with patch in bug 1125934

Comment 13

[Tooru Fujisawa [:arai]]

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.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not sure which notification were fired when redundant composition change event was handled, though. You can check it in following lines with TSF mode:

selection change:
http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEContentObserver.cpp#356

text change:
http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEContentObserver.cpp#727
http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEContentObserver.cpp#793
http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEContentObserver.cpp#868

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1133802-Force-Gecko-update-on-endBatchEdit-for-r.patch

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.

Comment 17

[Chris Peterson [: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.

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1133802-Force-Gecko-update.patch

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?

Comment 19

[Chris Peterson [:cpeterson]]

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! :)

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 21

[Eugen Sawin [:esawin]]

Landing the simple original version.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=749d9cb63aa0

https://hg.mozilla.org/integration/fx-team/rev/e75d824c730d

Comment 22

[Eugen Sawin [:esawin]]

Landing the simple original version.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=749d9cb63aa0

https://hg.mozilla.org/integration/fx-team/rev/e75d824c730d

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e75d824c730d

Comment 24

[Kevin Brosnan [Ex-Mozilla]]

Looks good. No known fallout we should get this uplifted to Aurora.

Comment 25

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1133802-Force-Gecko-update.patch

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

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression and we're uplifting it to aurora.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9119e76f47d4

Comment 29

[Ryan VanderMeulen [:RyanVM]]

This fixed the issue I was having with Swype as well, except in that case, it was every word being duplicated!