1217134 - Replace show password placeholder with conventional show password checkbox

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Ryan Feeley [:rfeeley]]

Attachment: show-password-proposed.png

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.

Comment 1

[Jean-Marie COUPRIE]

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...

Comment 2

[Jean-Marie COUPRIE]

BTW : I selected the line with my name, right clicked to see the source and found my email in clear text!
It works also for the others : Bugzilla is a paradise for the spamers !
 Where can I report this bug in bugzilla ?

Comment 3

[Tony Mechelynck [:tonymec]]

(In reply to Jean-Marie COUPRIE from comment #2)
> BTW : I selected the line with my name, right clicked to see the source and
> found my email in clear text!
> It works also for the others : Bugzilla is a paradise for the spamers !
>  Where can I report this bug in bugzilla ?

AFAIK it is not a bug. This page is sent by https (i.e. encrypted) between the server and your browser. When you do "View Source" in the browser it shows the decrypted text. If it didn't decrypt it you couldn't read the page. Otherwise bugs in Bugzilla software are reported on this very site, either in the "Bugzilla" product or (if specific to the Mozilla implementation) in the "bugzilla.mozilla.org" product.

The presence of your email address to allow sending you an email by clicking your name in the comment header is also intentional: it allows developers to send a private email to e.g. trolls without further spamming the bug.

Comment 4

[Ryan Feeley [:rfeeley]]

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?

Comment 5

[Jean-Marie COUPRIE]

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 ?

Comment 6

[Ryan Feeley [:rfeeley]]

Attachment: 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

Comment 7

[Jean-Marie COUPRIE]

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...

Comment 8

[Andrew Krawchyk]

I'm interested in starting work on this bug. Are the details ironed out enough to begin?

Comment 9

[Ryan Feeley [:rfeeley]]

Perhaps client side Telemetry to indicate how often users choose to show and hide the password. Otherwise, seems fairly straight-forward to me.

Comment 10

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox

WIP patch

Review commit: https://reviewboard.mozilla.org/r/40335/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40335/

Comment 11

[Matthew N. [:MattN]]

(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/

Comment 12

[Jean-Marie COUPRIE]

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...

Comment 13

[Ryan Feeley [:rfeeley]]

(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.

Comment 14

[[e-mail]]

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?

Comment 15

[[e-mail]]

Attachment: 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.

Comment 16

[Matthew N. [:MattN]]

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

Comment 17

[[e-mail]]

Attachment: MozReview Request: Bug 1217134 - Replace show password placeholder with conventional show password checkbox; r?MattN

Review commit: https://reviewboard.mozilla.org/r/46669/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46669/

Comment 18

[[e-mail]]

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

Comment 19

[Matthew N. [:MattN]]

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.

Comment 20

[[e-mail]]

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/

Comment 21

[[e-mail]]

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/

Comment 22

[[e-mail]]

Attachment: MozReview Request: remove skip file in ini to test on try server

Review commit: https://reviewboard.mozilla.org/r/47427/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47427/

Comment 23

[[e-mail]]

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 24

[[e-mail]]

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.

Comment 25

[[e-mail]]

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/

Comment 26

[[e-mail]]

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?

Comment 27

[Matthew N. [:MattN]]

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.

Comment 31

[[e-mail]]

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/

Comment 32

[[e-mail]]

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 33

[[e-mail]]

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 34

[[e-mail]]

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 35

[[e-mail]]

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/

Comment 36

[[e-mail]]

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 37

[[e-mail]]

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/

Comment 38

[[e-mail]]

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

Comment 39

[Kamil Jozwiak [:kjozwiak]]

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!

Comment 40

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 41

[Matthew N. [:MattN]]

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!

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ae989ae33250

Comment 45

[Carsten Book [:Tomcat]]

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

Comment 46

[Alexandru Simonca [:asimonca], Release Desktop QA]

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!