767354 - Typing 1 character in password fields will insert 2 characters

Status: VERIFIED FIXED

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

Description

[dynamis (Tomoya ASAI)]

Attachment: Typing into the password filed on Glaxy Nexus

See the attached video.
When I input into 1 character in password fields, 2 character will be inserted.

This is happen for 3rd or later chars only when I input slowly.
This is not happen for the first 2 chars.
This is not happen when I input quickly.
I'm not sure this problem depends on IME or not.

Tested with:
Device: Galaxy Nexus, ISW11F (sharp)
Firefox: Firefox 14 Beta6, Nightly 2012-06-22
IME: Google IME, ATOK

Comment 1

[dynamis (Tomoya ASAI)]

I tested a little more. This bug maybe depends on IMEs as Aaron update.

Reproduced with:
Device: Galaxy Nexus, Desire HD, ISW11F (sharp)
Firefox: Firefox 14 Beta6, Nightly 2012-06-22
IME: Google IME, ATOK, iWnn

not Reproduced with:
IME: no IME (English keyboard), Open Wnn

Comment 2

[dynamis (Tomoya ASAI)]

This is the regression bug of NativeUI.

This is not happen on XUL Firefox 10.0.5.
# with some IME like Google IME, last char will brink twice but only 1 char will
# be inserted properly

Comment 3

[dynamis (Tomoya ASAI)]

Checked with Google IME on ISW11F Android 2.3.

Not Reproduced:
2011-12-31 birch
2011-12-16 mozilla-central

Reproduced:
2011-12-18 mozilla central
2011-12-21 mozilla-central

With Google IME and some other IMEs, when user type next char, last char will shown new char will be inserted (works fine but looks poor, another bug).
Regression may happen 
# if user input chars quickly before last char become "*", last char keep shown
# and this bug is not happen

Comment 4

[Cristian Nicolae (:xti)]

I cannot reproduce this issue on the latest Nightly using the Android IME.

--
Firefox 16.0a1 (2012-06-27)
Device: Galaxy Nexus
OS: Android 4.0.2

Comment 5

[Zac C (:zac)]

I can replicate this on:
Nightly 16.0a1 (2012-06-28)
Device: Desire HD
Android: 2.3.5
Keyboard: Touch Input (Phone setting)

Also keyboard:
GoogleJapanese IME Beta on latin keyboard mode.

IMO this should be a higher priority, it makes Fx extremely frustrating for some of the most common tasks such as logging in to a website.

Comment 6

[Kaz Nishimura]

This looks like the same problem as 759025.

Comment 7

[Zac C (:zac)]

Yes it definitely sounds like the same. Chris, can we get the priority bumped on this one? It really does make entering passwords a balance of extreme patience or impossible.
I can make a video of this on nightly but not until next week.

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This only seems to occur on the password field.  I can't seem to reproduce the issue on a normal field.

Reduced test case can be found : http://people.mozilla.com/~nhirata/html_tp/formsninput.html (Password field has been added)

1. install Japanese IME
2. go to http://people.mozilla.com/~nhirata/html_tp/formsninput.html
3. with latin keyboard mode using Japanese IME, type several characters in the Password field.

Expected: 1 character for each input
Actual: first character entered seems ok.  Second overwrites first.  Third character will enter double characters.

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Going off of Comment 3:
Not Repro : 2011-12-16 mozilla-central
Repro : 2011-12-18 mozilla central

Also reproduced on 2011-12-17 mozilla-central.

16th was a merge date.  Merge mozilla-central and mozilla-inbound 
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7a510c246bf2

This occurs on the inbound of 12/16/2011

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Does not seem to occur on inbound:
11/01/2011 (sometimes a character writes over the latest character typed; but does not double type characters)

Mildly occurs on 11/17/2011 inbound:
Most of the time it does not double type, it occasionally overwrites.
about 70 % typing normal 15% chance of overwrite, 15 % chance of overwrite with double typing.

Does occur on inbound:
12/07/2011 (overwrites current character and double types)
12/16/2011

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

inbound 11/15/2011 does not have this issue.
inbound 11/16/2011 build has this bug reproducing

Looks like this bug existed for a while on inbound.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Masayuki, can you take a look at this?

Comment 13

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Dynamis, is this ATOK for Android?  Does this reproduce with ATOK for Windows?

Comment 14

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

(In reply to Naoki Hirata :nhirata from comment #13)
> Dynamis, is this ATOK for Android?  Does this reproduce with ATOK for
> Windows?

On Windows, password fields make IME disabled (i.e., IME is not available in password fields).

Comment 15

[Makoto Kato [:m_kato]]

This issue is both editor and GeckoInputConnection.  Since LookAndFeelGetEchoPassword() on Android's widget return true, nsTextEditRules has timer for password mask.  But this code replaces composition string with '*', this problem occurs.  If this isn't composition string, this won't occur.

Even if nsTextEditRules:::Notify() calls editor->ForceCompostionEnd(), due to IME status updater is timer-based implementation, it won't worked well.

Comment 16

[Makoto Kato [:m_kato]]

s/LookAndFeelGetEchoPassword/LookAndFeel::GetEchoPassword/

Comment 17

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

I don't know the ATOK for Android's behavior in password, however, it sounds like we don't need to set the timer if there is composition string, right?

Comment 18

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> I don't know the ATOK for Android's behavior in password, however, it sounds
> like we don't need to set the timer if there is composition string, right?

When masking character, we should commit composition string before replacing it.  So This is no way without timer for masking.

Also, by bug 769520, some race issue is fixed.  After that, we may fix this easily.

Comment 19

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

(In reply to Makoto Kato from comment #18)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #17)
> > I don't know the ATOK for Android's behavior in password, however, it sounds
> > like we don't need to set the timer if there is composition string, right?
> 
> When masking character, we should commit composition string before replacing
> it.  So This is no way without timer for masking.

I meant that we do NOT need to mask ourselves if the current inputting character is in composition string. The reasons is, the composition string is owned by IME, if IME thinks that it should be hidden, IME should do it itself. And the behavior should be same as the case focus in native password field.

I think that when editor receives a compositionend event, it should set the timer. And if editor receives a compositionstart event during waiting the timer, it should cancel the timer and mask the visible character at starting to handle the compositionstart event.

Comment 20

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

I think that it's wrong approach to call forceCompositionEnd() at masking the character. It would cause unexpected behavior if IME were waiting next input.

Comment 21

[Makoto Kato [:m_kato]]

Nakano-san, do you make sense TextView behavior on Android framework?  If password field by TextView, inputted character that is composing state is committed as '*' after a few minutes, then composting state is reset.

You should image k-tai (10-key) input style.  Password field must have composting state and we must render composition string.  So we cannot be hidden.

Comment 23

[JP Rosevear [:jpr]]

Does anyone have a feel on ETA here?  Like to take this into a beta go-to build on Monday for FF 14 (FF14.0.1 for native fennec)

Comment 24

[Makoto Kato [:m_kato]]

(In reply to JP Rosevear [:jpr] from comment #23)
> Does anyone have a feel on ETA here?  Like to take this into a beta go-to
> build on Monday for FF 14 (FF14.0.1 for native fennec)

I found workaround, but it is risky for 14.0.1.

GeckoInputConnection.notifyTextChange doesn't work well with composition string on this situation.  I am looking for better workaround....

Comment 25

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 26

[Chris Peterson [:cpeterson]]

Comment on attachment 640191 [details] [diff] [review]
fix

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

LGTM

Comment 27

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b7b40019a0

Comment 28

[Chris Peterson [:cpeterson]]

Comment on attachment 640191 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Some users can't login into websites because password fields are broken!
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): This is a medium risky change because it modifies code where Gecko updates IME content, BUT I think this fix is worth the risk because this password bug is a serious threat to retaining users.
String or UUID changes made by this patch: None

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 640191 [details] [diff] [review]
fix

This is a .N blocker and mobile-only. The code change is small, but not without risk. The small risk is worth it though.

Let's do some heavy testing though in this last beta

Comment 30

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/mozilla-central/rev/23e02d96d349

Comment 31

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6e760e2a01a9

Comment 32

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-beta/rev/07b2f28dee11

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(In reply to Chris Peterson (:cpeterson) from comment #27)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b7b40019a0

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