Closed Bug 1217134 Opened 4 years ago Closed 3 years ago

Replace show password placeholder with conventional show password checkbox

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox49 --- verified

People

(Reporter: rfeeley, Assigned: gasolin, Mentored)

References

(Depends on 1 open bug)

Details

(Whiteboard: [lang=js][lang=xul])

User Story

So that I can make fewer mistakes and jog my memory, as a Firefox user saving a password, I want to be able to show the password.

ACCEPTANCE CRITERIA
Only when the user checks "Show password" is the password unmasked
When the user unchecks "Show password" the password is masked
When the user collapses or dismisses the dialog, the checkbox returns to the default (unchecked) state

Attachments

(5 files, 1 obsolete file)

Though it's nearly impossible to replicate real world conditions for this, user testing revealed a few users who did not notice the "show" placeholder in the password field. Instead we should use a more conventional show password checkbox.
Blocks: 1169702
Mentor: MattN+bmo
Whiteboard: [lang=js][lang=xul]
Personally I like the functionality given by login on Windows 8.1 : something like en eye always seen on the right of the password field : the password is shown as long as I click on it but disappear immediately after.
Is there a problem with this ?
 Patented ?
 Risk of user leaving its PC with password filed but not sent ? In this case use a time-out to clear the field. The user will have to reenter the password when he sees the field empty.

To show the password is very convenient when you have been disturbed or when you are not sure of what you have entered...
Summary: Replace show password placeholder with conventional show pasword checkbox → Replace show password placeholder with conventional show password checkbox
On OS X the checkbox is more conventional (I may be showing my Mac-bias). Should we be showing the "eye" on Windows and the checkbox on Mac?
Can I summarize my preferences in decreasing order ?
1) To show the password is very convenient and needed.
2) I don't like "show-password-proposed.png" because it convey the idea that I wish that Firefox memorize the password.
3) the "eye" (on Windows) does not need translation. Please Ryan Feeley (or other) show us the check-box on Mac : it may be as good ! What is Linux solution ? Are there other proposals ?
Attached image show-password-osx.png
1) We agreed!
2) We agree because this is only in the door hanger that is asking the user to save their credentials, although I don't know why you don't like this.
3) You can search the web for similar UI for Ubuntu wi-fi dialog
Password should be difficult to find by illegal ways.
 One way is to listen to the transmission of it : transmission should be encrypted (https).
 An other is to find where it is saved in the PC and how it has been encrypted (hopefully). If it is only in my head, this method does not work... So normally I do not use the functionality to save the password.

Connecting to a wi-fi network is slightly different because you have afterwards to give an other password to access critical applications and probably, for ease of use, you can accept to access it with a saved password.

I prefer a button to show password only as long I press it than a box to be checked that show it permanently (at least in a crowded area whee some one else can see it).

An other advantage of the Windows 8.1 login process is that it show a message if the upper case key is locked. This is a common case of wrong password...
I'm interested in starting work on this bug. Are the details ironed out enough to begin?
Flags: needinfo?(MattN+bmo)
Perhaps client side Telemetry to indicate how often users choose to show and hide the password. Otherwise, seems fairly straight-forward to me.
(In reply to Andrew Krawchyk from comment #8)
> I'm interested in starting work on this bug. Are the details ironed out
> enough to begin?

Yes, sorry for the delay. I had a work-in-progress patch that I was hoping to attach so someone can finish but since I had other higher priorities, I didn't get around to it. I don't remember what's left to do but probably some tests should be added/modified.

> ./mach test toolkit/components/passwordmgr/test/browser/
Flags: needinfo?(MattN+bmo)
I still don't see the need to put together the saving and the showing of the password as in the image.
I am not sufficient knowledge to see from the coding how long the password is visible but the shorter is the more secure...
(In reply to Jean-Marie COUPRIE from comment #12)
> I still don't see the need to put together the saving and the showing of the
> password as in the image.

Unfortunately, Firefox asks the users to save the password even if it is incorrect. For Firefox Accounts, passwords are entered incorrectly about 50% the time the login form is viewed. Allowing users see and optionally edit the password lets them helps them see their mistakes and correct them before their next login attempt.
Hi I'm trying to pick it up. 

I've applied the patch to my code base, do `./mach run` and try to login on some website, but the password prompt is not shown. Do I need to rebuild the whole gecko to make it work?
Flags: needinfo?(MattN+bmo)
Attached image passwindow.png
I `mach build` the source then switch to other branch, then the password box is shown. If I live in the patched branch, the password box does not shown.
Hi Fred, there may be an error in the Browser Console with this patch applied. It's possible it no longer cleanly applies but you should be able to log in the changed parts of the patch to debug if there's nothing in the Browser Console.

https://developer.mozilla.org/en-US/docs/Tools/Browser_Console
Flags: needinfo?(MattN+bmo)
https://reviewboard.mozilla.org/r/46669/#review43281

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:238
(Diff revision 1)
> -    usernameChangedTo: "",
> -    usernameChangedToExists: true,
> -    passwordInPage: "password",
> -    passwordChangedTo: "newPassword",
> -    timesUsed: 1,
>    }];

In this request-for-feedback version,
`Editing username to an empty one and a new password.` case does not work.

Haven't figure it out yet so commet it out first, and try to running treeherder on different OS and see how other tests goes.

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties:27
(Diff revision 1)
>  # String is the login's hostname.
>  rememberPasswordMsgNoUsername = Would you like to remember the password on %S?
>  # LOCALIZATION NOTE (noUsernamePlaceholder):
>  # This is displayed in place of the username when it is missing.
>  noUsernamePlaceholder=No username
> -# LOCALIZATION NOTE (showPasswordPlaceholder):
> +togglePasswordLabel=Show password

The patch is pretty identical with your origin patch. Though I found `showPasswordAccessKey` is very similar to existing `showPasswordsAccessKey`, so I rename it as togglePasswordAccessKey/Label
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

I haven't looked closely yet but can you please make sure there a test in that file that test that the checkbox works. You will need to run that one test specifically with mach as I believe it's skipped when run with the directory.
Attachment #8741653 - Flags: feedback?(MattN+bmo)
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46669/diff/1-2/
Attachment #8741653 - Flags: feedback?(MattN+bmo)
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46669/diff/2-3/
Review commit: https://reviewboard.mozilla.org/r/47427/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47427/
Attachment #8741653 - Attachment description: MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox → MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; f?MattN
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46669/diff/3-4/
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

I've temporarily removed the skip browser_notification in browser.ini and test it on try server. Will add the skip section back when all test works fine.
Attachment #8741653 - Flags: feedback?(MattN+bmo)
Comment on attachment 8742705 [details]
MozReview Request: remove skip file in ini to test on try server

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47427/diff/1-2/
https://reviewboard.mozilla.org/r/46669/#review44361

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:239
(Diff revision 4)
>      usernameChangedToExists: true,
>      passwordInPage: "password",
>      passwordChangedTo: "password",
>      timesUsed: 2,
> -    checkPasswordNotUpdated: true,
> -  }, {
> +    checkPasswordNotUpdated: true
> +  }/*, {

The comment out test case saved `usernameChangedTo/passwordChangedTo` with  `Services.logins.addLogin` function when `usernameChangedToExists` property is set. 
Then the test submit `usernameInPage/passwordInPage` in web form.
Since the user name is different, the `expectedNotification` will set as `addLogin`. 

But in this case `TestUtils.topicObserved("passwordmgr-storage-changed"...` got `modifyLogin` as returned data.  It looks like an issue that can be solved in a separate bug?
https://reviewboard.mozilla.org/r/46669/#review47383

Apologies for the delay…

With the test changes this should be ready for review.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:439
(Diff revision 4)
>          // Look for existing logins.
> -        var foundLogins = this._pwmgr.findLogins({}, hostname, null,
> +        var foundLogins = this._pwmgr.findLogins({}, hostname, null, realm);
> -                                                 realm);
>  

Please revert this to avoid changing blame for unrelated code.

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:9
(Diff revision 4)
> -    password: "password",
> +    password: "password"
>    }, {
>      username: "",
> -    password: "password",
> +    password: "password"
>    }, {
>      username: "username",

Please don't remove trailing commas on objects. We prefer them as blame won't change when a new property is added afterwards

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:90
(Diff revision 4)
>    let testCases = [{
>      usernameInPage: "username",
> -    usernameChangedTo: "newUsername",
> +    usernameChangedTo: "newUsername"
>    }, {
>      usernameInPage: "username",
>      usernameInPageExists: true,
> -    usernameChangedTo: "newUsername",
> +    usernameChangedTo: "newUsername"
>    }, {
>      usernameInPage: "username",

Revert these too

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:214
(Diff revision 4)
>      passwordInPage: "password",
>      passwordChangedTo: "newPassword",
> -    timesUsed: 1,
> +    timesUsed: 1
>    }, {
>      usernameInPage: "username",
>      usernameInPageExists: true,
>      passwordInPage: "password",
>      passwordInStorage: "oldPassword",
>      passwordChangedTo: "newPassword",
> -    timesUsed: 2,
> +    timesUsed: 2
>    }, {

Here too

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:245
(Diff revision 4)
>      usernameInPage: "newUsername",
>      usernameChangedTo: "",
>      usernameChangedToExists: true,
>      passwordInPage: "password",
>      passwordChangedTo: "newPassword",
> -    timesUsed: 1,
> +    timesUsed: 1

I think this testcase was just broken or was affected by changes more recently to handling of empty usernames. Please change timesUsed to 2 since an update is expected and comment with the follwing fix…

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:300
(Diff revision 4)
>        // We expect a modifyLogin notification if the final username used by the
>        // dialog exists in the logins database, otherwise an addLogin one.
>        let expectModifyLogin = testCase.usernameChangedTo
>                                ? testCase.usernameChangedToExists
>                                : testCase.usernameInPageExists;

In the commented case `testCase.usernameChangedTo` is falsy because it's an empty string but I think this code meant to check if it's not undefined instead of falsy:
`typeof(testCase.usernameChangedTo) !== "undefined"`

With this change the testcase passes.

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:383
(Diff revision 4)
> +      // Focus the password textbox
> +      BrowserTestUtils.waitForEvent(passwordTextbox, "focus");

The test fails here since we're waiting for a focus but didn't actually do anything to focus it in the first place since you removed the .focus() call.

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:402
(Diff revision 4)
>   * Checks that clicking at the doorhanger pane takes the focus
>   * out of the password field.
>   *
>   * We skip Linux for now because focusing elements on the doorhanger
>   * doesn't work as expected (Bug 1182296)
>   */
>  add_task(function* test_unfocus_click() {

You deleted the code for this should this function/comment should be renamed/deleted/updated.

You should probably add a test that unchecking the box after checking reverts to the dots.
Attachment #8741653 - Flags: feedback?(MattN+bmo) → feedback+
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46669/diff/4-5/
Attachment #8741653 - Attachment description: MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; f?MattN → MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN
Attachment #8741653 - Flags: feedback+ → review?(MattN+bmo)
Attachment #8742705 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/46669/#review47383

> In the commented case `testCase.usernameChangedTo` is falsy because it's an empty string but I think this code meant to check if it's not undefined instead of falsy:
> `typeof(testCase.usernameChangedTo) !== "undefined"`
> 
> With this change the testcase passes.

it works!

> You deleted the code for this should this function/comment should be renamed/deleted/updated.
> 
> You should probably add a test that unchecking the box after checking reverts to the dots.

Issues addressed, add unchecking test inside of previous test case
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46669/diff/5-6/
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46669/diff/6-7/
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46669/diff/7-8/
https://reviewboard.mozilla.org/r/46669/#review47699

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:379
(Diff revision 8)
> -      let focusPassword = BrowserTestUtils.waitForEvent(passwordTextbox, "focus");
> -      passwordTextbox.focus();
> -      yield focusPassword;
> -
> -      // Wait for the textbox type to change
> -      yield ContentTaskUtils.waitForCondition(() => passwordTextbox.type == "", "Password textbox changed type");
> +      Assert.ok(toggleCheckbox.checked);
> +      Assert.equal(passwordTextbox.type, "", "Password textbox changed to plain text");
> +
> +      yield EventUtils.synthesizeMouseAtCenter(toggleCheckbox, {});
> +      Assert.ok(!toggleCheckbox.checked);
> +      Assert.equal(passwordTextbox.type, "password", "Password textbox changed to * text");

Test further for browser_notification test, I found if we dont use `ContentTaskUtils.waitForCondition` syntax then the test also works on linux

update the test, put a new try to see how it goes
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46669/diff/8-9/
the try shows some intermittent issue related to the test 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b346a27ca09e

I've separate the tests and push a new try
Florin, this should probably get tested once it's completed as it's a pretty big change on how the password manager behaves. Switching to the explicit toggle will also fix several other issue that were found while the password manager was using an implicit toggle.. Example :bug # 1194344, bug # 1176025 and bug # 1179870

If your team needs any help testing this once it's done, just let me know!
Flags: needinfo?(florin.mezei)
Thanks Kamil for the heads up! Setting the qe-verify+ flag and moving ni? to Andrei so we have this on our team's radar.
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Comment on attachment 8741653 [details]
MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

https://reviewboard.mozilla.org/r/46669/#review48875

::: toolkit/components/passwordmgr/test/browser/browser_notifications_2.js:1
(Diff revision 9)
> +Cu.import("resource://testing-common/ContentTaskUtils.jsm", this);

I think this is unused in both test files now. I'll remove it and push. Thanks!
Attachment #8741653 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/ae989ae33250
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1272849
Depends on: 1275100
We performed Smoke and Exploratory Testing on the Password Manager, targeting both core functionality and the changes introduced by this patch. We used Nightly 49.0a1 (20160530071207) to do this, covering Windows 7 (x64), Windows 10 (x64), OS X 10.9.5 and Ubuntu 14.04 (x86). We haven't found any new issues and the known ones are not of major impact. 

More details of our testing can be found in the following doc: https://goo.gl/XLHbz0

Kamil, please let us know if you have in mind other scenarios that we might have missed. Thanks!
Status: RESOLVED → VERIFIED
Flags: needinfo?(andrei.vaida) → needinfo?(kjozwiak)
Flags: needinfo?(kjozwiak)
You need to log in before you can comment on or make changes to this bug.