Closed Bug 1143903 Opened 7 years ago Closed 7 years ago

Display username and password as separate fields in the password doorhanger

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox39 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Keywords: user-doc-needed)

Attachments

(2 files, 3 obsolete files)

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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Attached patch Work in progress (obsolete) — Splinter Review
I should work on removing some of the duplication, but in the meantime there is still material for feedback.
Attachment #8578304 - Flags: feedback?(MattN+bmo)
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.
Attachment #8578304 - Flags: feedback?(MattN+bmo) → feedback+
Attached image 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.
Blocks: 1144856
Depends on: 1144869
Attached patch The patch (obsolete) — Splinter Review
The final strings will be different, so I reused the current ones for now.
Attachment #8578304 - Attachment is obsolete: true
Attachment #8579612 - Flags: review?(MattN+bmo)
Keywords: user-doc-needed
Attachment #8579612 - Flags: review?(MattN+bmo) → review+
Attached patch Updated patch (obsolete) — Splinter Review
Fixed test cases that checked the displayed messages.
Attachment #8579612 - Attachment is obsolete: true
Attached patch Updated patchSplinter Review
Fixed leftover debugging code and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1191b25feba3
Attachment #8580403 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/aa2b8bc620db
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
QA Contact: 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!
Status: RESOLVED → VERIFIED
No longer blocks: passwords-2015-UX
You need to log in before you can comment on or make changes to this bug.