1143903 - Display username and password as separate fields in the password doorhanger

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[:Paolo Amadini]

To clarify the password doorhanger, separate read-only username and password fields can be used as a first step towards the ability to edit them.

Comment 1

[:Paolo Amadini]

Attachment: Work in progress

I should work on removing some of the duplication, but in the meantime there is still material for feedback.

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8578304 [details] [diff] [review]
Work in progress

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

Awesome!

::: browser/base/content/popup-notifications.inc
@@ +61,5 @@
>      </popupnotification>
>  
> +    <popupnotification id="password-notification" hidden="true">
> +      <popupnotificationcontent orient="vertical">
> +        <textbox id="password-notification-username" flex="1"

Reminder to remove @flex

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ -1023,5 @@
> -                                    "updatePasswordMsg",
> -                                    [displayUser]);
> -    } else {
> -      notificationText  = this._getLocalizedString(
> -                                    "updatePasswordMsgNoUser");

Paolo tells me these strings are still used for old notifications.

@@ +1073,5 @@
> +                 .value = aOldLogin.username;
> +        chromeDoc.getElementById("password-notification-password")
> +                 .value = aNewPassword;
> +
> +        return false;

Remove this r.v. as we discussed in case it gets used for something later

::: toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
@@ +13,5 @@
>  # String is the login's hostname.
>  rememberPasswordMsgNoUsername = Would you like to remember the password on %S?
> +# LOCALIZATION NOTE (usernamePlaceholder):
> +# This is displayed in place of the username when it is missing.
> +usernamePlaceholder=No username

This doesn't match the UX spec so you should probably discuss this with Ryan. Also, I think the ID should include "No" since we will want usernamePlaceholder to be the new value after it's editable. We may want to avoid churn on the string for l10n.

Comment 3

[:Paolo Amadini]

Attachment: Screenshot

Talked with Ryan and we concluded the "No username" string is better in general even for the final version. Here is a screenshot of how this looks on OS X.

Comment 4

[:Paolo Amadini]

Attachment: The patch

The final strings will be different, so I reused the current ones for now.

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

Fixed test cases that checked the displayed messages.

Comment 6

[:Paolo Amadini]

Attachment: Updated patch

Fixed leftover debugging code and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1191b25feba3

Comment 7

[:Paolo Amadini]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Kamil Jozwiak [:kjozwiak]]

Went through all the test cases listed below in the etherpad without any issues. I've also included the OS's and websites that were used:

* https://etherpad.mozilla.org/Bug1143903-Bug1145913

As per my conversation with Paolo, this was tested in conjunction with bug # 1145913. The main scope was to concentrate on editing the username field in the doorhanger, especially on "change password" pages that don't supply a username.

If someone sees a missing case in the etherpad, please let know and I'll add it!