Closed Bug 1145913 Opened 9 years ago Closed 9 years ago

Make the username in the password notification editable

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
mozilla39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox39 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 1 obsolete file)

This implements the first part of the editability on capture.
Attached patch Work in progress (obsolete) — Splinter Review
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8581009 - Flags: feedback?(MattN+bmo)
Iteration: --- → 39.2 - 23 Mar
Points: --- → 5
Flags: qe-verify+
Flags: firefox-backlog+
Attached patch The patchSplinter Review
Now with tests.
Attachment #8581009 - Attachment is obsolete: true
Attachment #8581009 - Flags: feedback?(MattN+bmo)
Attachment #8581932 - Flags: review?(MattN+bmo)
Comment on attachment 8581932 [details] [diff] [review]
The patch

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

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js
@@ +838,5 @@
> +      currentNotification.mainAction.accessKey = accessKey;
> +
> +      // Update the labels in real time if the notification is displayed.
> +      let element = [...currentNotification.owner.panel.childNodes]
> +                    .find(n => n.notification == currentNotification);

We should really expose this as an API on PopupNotifications at some point since enough code reaches into it like this.

@@ +843,5 @@
> +      if (element) {
> +        element.setAttribute("buttonlabel", label);
> +        element.setAttribute("buttonaccesskey", accessKey);
> +      }
> +    }

Nit: missing semicolon after all of the helper functions.
Attachment #8581932 - Flags: review?(MattN+bmo) → review+
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
For some reason the last fix from bug 1145789 was lost. Relanded with the fix:

https://hg.mozilla.org/integration/fx-team/rev/4944dc94d987
Flags: needinfo?(paolo.mozmail)
https://hg.mozilla.org/mozilla-central/rev/4944dc94d987
Status: ASSIGNED → RESOLVED
Closed: 9 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 # 1143903. 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
(In reply to Kamil Jozwiak [:kjozwiak] from comment #10)
> If someone sees a missing case in the etherpad, please let know and I'll add it!

Hi Kamil, sorry for the delay. I've taken a look at the Etherpad and that's a pretty impressive set of tests! It will probably be worth ensuring that at least some of them are defined as recurring tests in our manual test suites.

I have only two notes specific to this bug:

 * Ensure that you can use the same username for multiple websites without any collision issues
 ** Ensure that changing the password of a username that's being used multiple times doesn't cause any collision issues

It might be better to expand "collision issues" into the exact steps taken to verify no collisions occurred.

 * Ensure that the username field under the password doorhanger is editable

Did you do any specific tests for possible "expected" collisions as a result of changing the username in the dialog? In particular we implemented different button labels based on whether clicking the button will overwrite an existing login or not.
Thanks for the feedback Paolo, much appreciated!

> I have only two notes specific to this bug:
> 
>  * Ensure that you can use the same username for multiple websites without
> any collision issues
>  ** Ensure that changing the password of a username that's being used
> multiple times doesn't cause any collision issues
> 
> It might be better to expand "collision issues" into the exact steps taken
> to verify no collisions occurred.

Agreed! Added several test cases into the wiki that will cover checks for possible collisions.

>  * Ensure that the username field under the password doorhanger is editable
> 
> Did you do any specific tests for possible "expected" collisions as a result
> of changing the username in the dialog? In particular we implemented
> different button labels based on whether clicking the button will overwrite
> an existing login or not.

Do you mean something similar to the following cases?

- login into SiteA and save the credentials
- login into SiteB with the same username as SiteA
- delete the current username from the doorhanger and re-enter the same username once again
- you should still have "Remember Password" rather than "Update Password" because that user doesn't exists under SiteB
- make sure SiteA's credentials are not being affected

- login into SiteA and save the credentials
- login into SiteB and save the credentials
- login into SiteA with a different username
- in the doorhanger, change the current username to the one you used previously, "Remember Password" should change to "Update Password"
- add a completely different username under the doorhanger that hasn't been saved on SiteA and "Update" should be change back to "Remember Password"
- make sure SiteB is never affected with the changes being made in SiteA
Flags: needinfo?(paolo.mozmail)
No longer blocks: passwords-2015-UX
(In reply to Kamil Jozwiak [:kjozwiak] from comment #12)
> Do you mean something similar to the following cases?

Yes, they look good to me!
Flags: needinfo?(paolo.mozmail)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: