Closed Bug 1169702 Opened 9 years ago Closed 9 years ago

Show password content on doorhanger once the field is focused

Categories

(Toolkit :: Password Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox39 --- wontfix
firefox40 --- disabled
firefox41 --- disabled
firefox42 + disabled
firefox43 + disabled

People

(Reporter: rittme, Assigned: rittme)

References

()

Details

Attachments

(3 files)

At the capture phase, the doorhanger password field content should become visible once clicked or tabbed into, as described by the following document:

toolkit/components/passwordmgr/test/test_basic_form_pwonly.html
Blocks: 1153217
Flags: qe-verify+
Flags: firefox-backlog?
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Summary: Show password content on doorhanger when clicked on it → Show password content on doorhanger once the field is focused
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P1
Assignee: nobody → bernardo
Status: NEW → ASSIGNED
For users who don't want Firefox to save passwords (so they don't click remember in the doorhanger) but haven't turned off saving globally we are making it easier for someone to walk up to another user's computer and see their password in plaintext with this change.

Before this change they would have to:
Open the capture doorhanger, click remember, open preferences, open saved passwords, toggle the visibility (or use the copy context menu) in the manager.

After this change:
Open the capture doorhanger and click into the password field.

Possible solution:
* Dismiss the capture anchor after some number of minutes?
Flags: needinfo?(rfeeley)
Bug1169702 - Show password content on focus and adds a SHOW placeholder
Attachment #8621918 - Flags: review?(MattN+bmo)
Comment on attachment 8621918 [details]
MozReview Request: Bug1169702 - Show password content on focus and adds a SHOW placeholder

https://reviewboard.mozilla.org/r/11099/#review9693

::: browser/base/content/browser.css:1304
(Diff revision 1)
> +  content:attr(show-content);
> +  display: block;
> +  position: absolute;
> +  right: 0;
> +  background: white;
> +  color:hsl(0,0%,60%);
> +  transition: color 250ms;
> +  pointer-events: none;

Nit: please sort the properties alphabetically.
Nit: Missing spaces after the colon on two properties.

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:18
(Diff revision 1)
> +# LOCALIZATION NOTE (showPasswordPlaceholder):
> +# This is displayed at the password field.
> +# Indicates that the password will be shown if focused.
> +showPasswordPlaceholder=SHOW

I guess we'll have to leave this out of the Beta patch.

I'm not sure if it's preferred to use `text-transform: uppercase;` or if that doesn't work for some locales?

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:868
(Diff revision 1)
> +    }

Nit: missing semicolon

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:872
(Diff revision 1)
> +    }

Nit: missing semicolon

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:866
(Diff revision 1)
> +    let onPasswordFocus = () => {
> +      chromeDoc.getElementById("password-notification-password").type = "";

I find it weird that the caret is at the beginning of the textbox when focusing with the mouse. I think we should match the usual OS behaviour and position the caret based on the click position.

Since I'd like to get user testing ASAP on Nightly, please file a follow-up for this and marked it as affecting Beta and later.

::: browser/base/content/browser.css:1303
(Diff revision 1)
> +#password-notification-password:after {
> +  content:attr(show-content);

Pseudo elements (in contrast with pseudo selectors) should have two colons (2x below too)

::: browser/base/content/browser.css:1308
(Diff revision 1)
> +  background: white;

Hmm… shouldn't the background be transparent by default. Is this to cover up long passwords? If so, please add a comment above it.
Attachment #8621918 - Flags: review?(MattN+bmo) → review+
(Quoting Matthew N. [:MattN] from comment #4)
> Comment on attachment 8621918 [details]
> MozReview Request: Bug1169702 - Show password content on focus and adds a
> SHOW placeholder
> 
> https://reviewboard.mozilla.org/r/11099/#review9693
> 
> ::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:18
> (Diff revision 1)
> > +# LOCALIZATION NOTE (showPasswordPlaceholder):
> > +# This is displayed at the password field.
> > +# Indicates that the password will be shown if focused.
> > +showPasswordPlaceholder=SHOW
> 
> I'm not sure if it's preferred to use `text-transform: uppercase;` or if
> that doesn't work for some locales?

Is `font-variant: small-caps` better (a la bug 992637)?
Flags: needinfo?(l10n)
(In reply to Matthew N. [:MattN] from comment #5)
> > I'm not sure if it's preferred to use `text-transform: uppercase;` or if
> > that doesn't work for some locales?
> 
> Is `font-variant: small-caps` better (a la bug 992637)?

Less broken than 'text-transform: uppercase' yes (bug 1171839 is a recent one), probably not the best solution.

One of the reason to use small-caps on the menu was to provide a way for theme developers to change it. If we don't need that here, I'd suggest to use uppercase text in the label.
Flags: needinfo?(l10n)
(In reply to Francesco Lodolo [:flod] from comment #6)
> Less broken than 'text-transform: uppercase' yes (bug 1171839 is a recent
> one), probably not the best solution.

To be honest I didn't like the outcome of bug 992637, decided to try to get a bit more information
https://groups.google.com/forum/#!topic/mozilla.dev.l10n/DzhY-d_38HQ
Comment on attachment 8621918 [details]
MozReview Request: Bug1169702 - Show password content on focus and adds a SHOW placeholder

Bug1169702 - Show password content on focus and adds a SHOW placeholder
Attachment #8621918 - Flags: review+ → review?(MattN+bmo)
Comment on attachment 8621918 [details]
MozReview Request: Bug1169702 - Show password content on focus and adds a SHOW placeholder

https://reviewboard.mozilla.org/r/11099/#review9701

I pushed a version with a few fixed.
Attachment #8621918 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/693b73e54c85
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attached patch Beta patchSplinter Review
Patch without the string.
Comment on attachment 8622593 [details] [diff] [review]
Beta patch

Approval Request Comment
[Feature/regressing bug #]: This should get fixed in the same release as bug 1143903 and bug 1145913 to avoid confusing and churning of the login capture doorhanger UX.
[User impact if declined]: Confusion about a readonly field and churning of the login capture doorhanger UX.
[Describe test coverage new/current, TreeHerder]: Manual testing on Nightly (email sent to passwords-dev).
[Risks and why]: Fairly straightforward pattern of toggling the field type
[String/UUID change made/needed]: None
Attachment #8622593 - Attachment description: bug1169702.patch → Beta patch
Attachment #8622593 - Flags: approval-mozilla-beta?
Attachment #8622593 - Flags: approval-mozilla-aurora?
Depends on: 1174815
Depends on: 1174900
Comment on attachment 8622593 [details] [diff] [review]
Beta patch

That sounds like a bit late for beta but Liz will make the call. Anyway, taking it for 40.
Attachment #8622593 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8622593 [details] [diff] [review]
Beta patch

Approved for uplift to beta after discussion with MattN and Kamil. They will be on top of this over the weekend to test it out and fix if anything goes wrong in beta 7.
Attachment #8622593 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1175273
Blocks: 1175279
Comment on attachment 8622593 [details] [diff] [review]
Beta patch

We're going to let this bug bake on Aurora longer but leave the editable censored password on Beta.
Attachment #8622593 - Flags: approval-mozilla-beta+
Went through verification using the following m-c build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-06-18-03-02-06-mozilla-central/

Test Cases Used:

* ensured that "SHOW" is highlighted blue while hovering over the password field
* ensured that clicking on the password field or SHOW will display the content in the password field
* ensured that the entire border of the password field is highlighted using the appropriate color when selected
* ensured that clicking on "SHOW" will place the cursor at the end of the content in the password field
* ensured that "SHOW" is not being displayed when a user selects the field and the content of the password field is visible
* ensure that a really long password will not overlap "SHOW" once the password has been hidden
* ensured that clicking anywhere on the password will place the cursor in the correct position
* ensured that the entire border of the password field is highlighted red when there's no content in the password field
* ensured that the password is re-hidden if the doorhanger is closed while the password was originally visible
** while pressing "X", "Not Now", "ESC", and clicking anywhere on the current website
* ensured that you can tab through both fields and the appropriate borders are highlighted

Websites Used:

- facebook.com
- tumbler.com
- linkedin.com
- amazon.com

OS's Used:

- OSX 10.10.3 x64
- Win 8.1 x64 (VM)
- Ubuntu 14.04.2 x64 (VM)
Went through verification using the following m-a build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-06-18-00-40-06-mozilla-aurora/

Went through the same test cases listed under comment # 18 without any issues. The only difference is that the "SHOW" text didn't make it into the password field do to strings/localization on m-a. Other than that, everything seems to be working correctly.

However I did find an issues relating to the "SHOW" flickering at times and created bug # 1175941
Status: RESOLVED → VERIFIED
Depends on: 1176025
Depends on: 1175941
Depends on: 1175987
Depends on: 1183908
Depends on: 1186123
Depends on: 1194344
Flags: needinfo?(rfeeley)
Depends on: 1203294
This (mini) feature was planned for 41 but is pref'd off by default in bug 1203294 for 41. The remaining work will likely land in 42/43 so tracking it.
I think the correct tracking flags are these one.
Used the following build to ensure that "SHOW" has been removed from fx42:
- https://archive.mozilla.org/pub/firefox/releases/42.0b8/

OS's Used:
==========

- Windows 7 x64 VM 
** fx42 -> PASSED

- Windows 10 x64 VM
** fx42 -> PASSED

- Ubuntu 14.04.3 x64 VM
** fx42 -> PASSED

- OSX 10.11 x64
** fx42 -> PASSED

Test Cases Used:
================

- ensure that the original issue from bug # 1190938 comment # 0 isn't occurring
--> found some issues and added a comment under bug # 1190938 comment # 16
- ensured you can tab through the fields in the doorhanger
- ensured that clicking on the password field doesn't show the password
- ensured that Undo, Cut and Paste have been disabled via the password field
- ensured you can't copy the password via the "CTRL + C" shortcut
- ensured you can still add/edit both the username and password fields via the doorhanger
- ensured that changes made under about:preferences#security are reflected under the doorhanger
As discussed, replace this unconventional show password affordance with a more conventional password unmasking checkbox. Usability tests indicated some users did not see the SHOW affordance. https://bugzilla.mozilla.org/show_bug.cgi?id=1217134
Depends on: 1217134
Depends on: 1230391
You need to log in before you can comment on or make changes to this bug.