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)
Toolkit
Password Manager
Tracking
()
VERIFIED
FIXED
mozilla41
People
(Reporter: rittme, Assigned: rittme)
References
()
Details
Attachments
(3 files)
40 bytes,
text/x-review-board-request
|
MattN
:
review+
|
Details |
122.12 KB,
image/gif
|
Details | |
4.27 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•9 years ago
|
Blocks: 1153217
Flags: qe-verify+
Flags: firefox-backlog?
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Assignee | ||
Updated•9 years ago
|
Summary: Show password content on doorhanger when clicked on it → Show password content on doorhanger once the field is focused
Updated•9 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P1
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bernardo
Status: NEW → ASSIGNED
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
Bug1169702 - Show password content on focus and adds a SHOW placeholder
Attachment #8621918 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 3•9 years ago
|
||
Comment 4•9 years ago
|
||
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+
Comment 5•9 years ago
|
||
(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)
Comment 6•9 years ago
|
||
(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)
Comment 7•9 years ago
|
||
(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
Assignee | ||
Comment 8•9 years ago
|
||
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 10•9 years ago
|
||
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+
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/693b73e54c85
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 12•9 years ago
|
||
Patch without the string.
Comment 13•9 years ago
|
||
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?
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/06984d7e828e
status-firefox40:
--- → fixed
Comment 16•9 years ago
|
||
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+
Comment 17•9 years ago
|
||
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+
Updated•9 years ago
|
status-firefox39:
--- → wontfix
Comment 18•9 years ago
|
||
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)
Comment 19•9 years ago
|
||
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
Comment 20•9 years ago
|
||
Disable on 40 in https://hg.mozilla.org/releases/mozilla-beta/rev/34b95d9ef07e
Updated•9 years ago
|
Flags: needinfo?(rfeeley)
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.
status-firefox42:
--- → affected
status-firefox43:
--- → affected
tracking-firefox42:
--- → +
tracking-firefox43:
--- → +
Comment 22•9 years ago
|
||
I think the correct tracking flags are these one.
Comment 23•9 years ago
|
||
> status-firefox41: verified → disabled https://hg.mozilla.org/releases/mozilla-beta/rev/f3b10e98a1d6
Comment 24•9 years ago
|
||
> status-firefox42: verified → disabled https://hg.mozilla.org/releases/mozilla-beta/rev/bab3ced35371
Comment 25•9 years ago
|
||
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
Comment 26•9 years ago
|
||
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
Comment 27•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf585498f2a0 https://hg.mozilla.org/releases/mozilla-beta/rev/366dd2904726 https://hg.mozilla.org/releases/mozilla-release/rev/5b66df4523cf
You need to log in
before you can comment on or make changes to this bug.
Description
•