Closed Bug 767354 Opened 9 years ago Closed 9 years ago

Typing 1 character in password fields will insert 2 characters

Categories

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

ARM
Android
defect
Not set
normal

Tracking

(firefox14+ fixed, firefox15 verified, firefox16 verified, firefox17 verified, blocking-fennec1.0 .N+)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox14 + fixed
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified
blocking-fennec1.0 --- .N+

People

(Reporter: bugzilla, Assigned: m_kato)

References

Details

(Keywords: regression)

Attachments

(2 files)

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
Component: General → IME
QA Contact: general → ime
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
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
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
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
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.
Assignee: nobody → cpeterson
This looks like the same problem as 759025.
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.
tracking-fennec: --- → ?
blocking-fennec1.0: --- → ?
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.
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
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
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.
Masayuki, can you take a look at this?
Assignee: cpeterson → masayuki
blocking-fennec1.0: ? → .N+
Dynamis, is this ATOK for Android?  Does this reproduce with ATOK for Windows?
(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).
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.
s/LookAndFeelGetEchoPassword/LookAndFeel::GetEchoPassword/
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?
(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.
(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.
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.
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.
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)
(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....
Attached patch fixSplinter Review
Attachment #640191 - Flags: review?(cpeterson)
Assignee: masayuki → m_kato
Comment on attachment 640191 [details] [diff] [review]
fix

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

LGTM
Attachment #640191 - Flags: review?(cpeterson) → review+
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
Attachment #640191 - Flags: approval-mozilla-beta?
Attachment #640191 - Flags: approval-mozilla-aurora?
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
Attachment #640191 - Flags: approval-mozilla-beta?
Attachment #640191 - Flags: approval-mozilla-beta+
Attachment #640191 - Flags: approval-mozilla-aurora?
Attachment #640191 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/23e02d96d349
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.