The default bug view has changed. See this FAQ.

Make the username in the password notification editable

VERIFIED FIXED in Firefox 39

Status

()

Toolkit
Password Manager
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla39
Points:
5
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox39 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This implements the first part of the editability on capture.
(Assignee)

Comment 1

2 years ago
Created attachment 8581009 [details] [diff] [review]
Work in progress
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8581009 - Flags: feedback?(MattN+bmo)
(Assignee)

Updated

2 years ago
Iteration: --- → 39.2 - 23 Mar
Points: --- → 5
(Assignee)

Updated

2 years ago
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9562b36ca5
(Assignee)

Comment 3

2 years ago
Created attachment 8581932 [details] [diff] [review]
The patch

Now with tests.
Attachment #8581009 - Attachment is obsolete: true
Attachment #8581009 - Flags: feedback?(MattN+bmo)
Attachment #8581932 - Flags: review?(MattN+bmo)
(Assignee)

Comment 4

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aa7860b278e
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+
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/56ce9fdb62e3
Backed out for mochitest-5 failures:
https://hg.mozilla.org/integration/fx-team/rev/c12899fa2f8a

https://treeherder.mozilla.org/logviewer.html#?job_id=2373378&repo=fx-team
Flags: needinfo?(paolo.mozmail)

Updated

2 years ago
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
(Assignee)

Comment 8

2 years ago
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
Last Resolved: 2 years ago
status-firefox39: --- → fixed
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
status-firefox39: fixed → verified
(Assignee)

Comment 11

2 years ago
(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)
Blocks: 1129619
No longer blocks: 1141601
(Assignee)

Comment 13

2 years ago
(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.