831862 - Disable dictionary suggestions on password-fields?

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (01/17) - Screenshot

This was suggested to me from a friend using Firefox 18. Currently we offer dictionary suggestions on password-fields where Chrome and stock do not.

What happens is that ones password field is masked and that the password becomes clearly visible as a new suggestion

This is reproducible on Facebook's login.

Comment 1

[Aaron Train [:aaronmt]]

Password would be visible to all exposed if a user signs into something and ones device is hooked up via HDMI MHL to an external display. Also shoulder viewers I suppose.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Same cause that regressed Bug 819073

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Force restart input during notifyIMEEnabled (v1)

Because we coalesce restartInput calls, when notifyIMEEnabled calls restartInput(), it may not actually cause a restart, in which case the type (password, etc.) of the input is not registered.

This patch bypasses coalescing for calls coming from notifyIMEEnabled, so the input type is always registered. The purpose of coalescing is to avoid excessive restartInput() calls from notifyIME, so bypassing it has no bad side effects when the calls are coming from notifyIMEEnabled, which is not called excessively.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 703591 [details] [diff] [review]
Force restart input during notifyIMEEnabled (v1)

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

I dislike boolean parameters because they make the caller code less readable, they suggest that a function is "doing two jobs", and once you have one special case you may need more in the future. For multiple special cases, an enum parameter is good, but in this case you might consider splitting restartInput() into two methods. Maybe something like tryRestartInput() that checks mLastRestartInputTime and restartInput() or restartInputNow() that forces a restart.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Force restart input during notifyIMEEnabled (v1.1)

I like the tryRestartInput idea. Thanks!

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 704043 [details] [diff] [review]
Force restart input during notifyIMEEnabled (v1.1)

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

Nice. This diff is surprisingly concise! :)

::: mobile/android/base/GeckoInputConnection.java
@@ +208,5 @@
>          if (time < mLastRestartInputTime + 200) {
>              return;
>          }
>          mLastRestartInputTime = time;
> +        restartInput();

You should move `mLastRestartInputTime = SystemClock.uptimeMillis()` to restartInput(). We want the mLastRestartInputTime to be updated when restartInput() is called directly or by tryRestartInput().

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Chris Peterson (:cpeterson) from comment #6)
> Comment on attachment 704043 [details] [diff] [review]
> Force restart input during notifyIMEEnabled (v1.1)
> 
> Review of attachment 704043 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. This diff is surprisingly concise! :)
> 
> ::: mobile/android/base/GeckoInputConnection.java
> @@ +208,5 @@
> >          if (time < mLastRestartInputTime + 200) {
> >              return;
> >          }
> >          mLastRestartInputTime = time;
> > +        restartInput();
> 
> You should move `mLastRestartInputTime = SystemClock.uptimeMillis()` to
> restartInput(). We want the mLastRestartInputTime to be updated when
> restartInput() is called directly or by tryRestartInput().

Done.

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

Leaving open for Aurora

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 704043 [details] [diff] [review]
Force restart input during notifyIMEEnabled (v1.1)

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Bug 825120

User impact if declined: Wrong behavior when using special input fields such as password fields

Testing completed (on m-c, etc.): Locally, m-c

Risk to taking this patch (and alternatives if risky): Very little risk; this patch fixes a regression from Bug 825120, and because Bug 825120 was uplifted to Aurora, this patch should be uplifted too to fix the regression.

String or UUID changes made by this patch: None

Comment 10

[bhavana bajaj [:bajaj]]

Comment on attachment 704043 [details] [diff] [review]
Force restart input during notifyIMEEnabled (v1.1)

Approving on aurora, given the risk is manageable where we are in the cycle & as Bug 825120 is in FF20 .

We will consider backing out if any severe regressions are found late in the cycle.Also, request to QA to help verify this bug and some adhoc testing around Bug 825120 .Thank you !

Comment 11

[Aaron Train [:aaronmt]]

Looks good on trunk. Verified Fixed on mozilla-central (Nightly, 01/22)

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/26337130270a