Closed Bug 1153217 Opened 9 years ago Closed 9 years ago

Allow editing the password in the login capture doorhanger

Categories

(Toolkit :: Password Manager, defect, P1)

defect
Points:
8

Tracking

()

VERIFIED FIXED
mozilla41
Iteration:
41.3 - Jun 29
Tracking Status
firefox39 + fixed
firefox40 + fixed
firefox41 --- verified

People

(Reporter: Paolo, Assigned: rittme)

References

Details

User Story

As a Firefox User I want to edit the password captured in the doorhanger of Password Manager.

Attachments

(2 files)

The password field in the login capture doorhanger should be editable.
[Tracking Requested - why for this release]: This should get fixed in the same release as bug 1143903 and bug 1145913 to avoid confusing and churning of the login capture doorhanger UX.

Some of the cases to handle in this bug:
* Empty password field
* Handling duplicate logins after a manual change (same username+password+origin)
* Updating the button state for new logins vs. updating

Automated tests should be written for the above cases.
Points: --- → 8
Flags: qe-verify+
Flags: firefox-backlog+
(In reply to Matthew N. [:MattN] from comment #1)
> [Tracking Requested - why for this release]: This should get fixed in the
> same release as bug 1143903 and bug 1145913 to avoid confusing and churning
> of the login capture doorhanger UX.

I disagree on the necessity to have this in the same release. I think it would be a nice feature to have, but there isn't a strong downside to enabling password editing incrementally in a subsequent release.

For the moment I'm focusing on completing the fill doorhanger and I'll not work on this feature for the capture doorhanger, but if you think there may be someone available to work on this, there is no downside to having this in the same release too.
Assignee: nobody → bernardo
Status: NEW → ASSIGNED
Iteration: --- → 41.2 - Jun 8
Blocks: 1167657
Rank: 15
Depends on: 1169702
Priority: -- → P1
User Story: (updated)
Tracking enabled for 39 and 40, to ensure the password is editable. Would someone see if 41 is also affected, please?
Hello,

When does this need to land on beta to make it in 39?
Flags: needinfo?(lhenry)
Flags: needinfo?(inkpixelspaper)
Before June 17th, I believe. Needinfo from Liz, to be sure.
Flags: needinfo?(inkpixelspaper)
Well before that would be much nicer. 
For beta 5: land before end of the day this Wednesday
For beta 6: land before Sunday night
For beta 7: (last beta before RC) land it before next Wed. the 17th
Thanks MattN!
Flags: needinfo?(lhenry)
Bug 1153217 - allow editing password in doorhanger, disables button if password is empty
Attachment #8617769 - Flags: review?(MattN+bmo)
This is a first patch to see if I'm going in the right direction. Still needs automated tests and more thorough manual testing, like check if the disabled styling renders correctly in all the different platforms.
For styling the empty password field, you may want something like what Hello does when you try to add a contact but there is no name specified, which uses a border similar to HTML5 field validation. I don't think it uses a placeholder text.
Attachment #8617769 - Flags: review?(MattN+bmo)
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

https://reviewboard.mozilla.org/r/10697/#review9429

Also ask Ryan about how we should handle the case where the username and password that the user changed the values to exactly match one already in storage.
On the backend I don't think we should call `_updateLogin` in that case since that causes us to change the timestamps. Instead we should be incrementing the use count and last used time.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:826
(Diff revision 1)
> +      btn = chromeDoc.getAnonymousElementByAttribute(element.button, "anonid", "button");

Please use a more descriptive variable name e.g. mainActionButton and explicitly declare the variable here with `let`.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:867
(Diff revision 1)
>        chromeDoc.getElementById("password-notification-password")
> +               .setAttribute("placeholder", passwordPlaceholder);

I like Paolo's suggestion to use an outline like form validation and Hello uses. That also helps for Beta where we can't add the string. Please check with Ryan about this and the cursor for the disabled button.
Reverting to "Remember" makes sense in the case that the same credentials exist.

When the password field is cleared, would the save button be disabled?
Yes, it should be greyed out.
When the password field is empty the save button is disabled. To style this state I followed the rules for the in-content preferences buttons, with 50% opacity and the cursor set to "not-allowed".
What do you think about Paolo's suggestion of having the empty password field outlined, like we do in hello? I think it makes sense.
Flags: needinfo?(rfeeley)
Sure, as long as it's not too time-consuming for you.
Flags: needinfo?(rfeeley)
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

Bug 1153217 - allow editing password in doorhanger, disables button if password is empty
Attachment #8617769 - Flags: review?(MattN+bmo)
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

https://reviewboard.mozilla.org/r/10697/#review9563

You're getting there but I'd like to take another look.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:805
(Diff revision 2)
> +      // Disable the main button inside the menu-button if password is empty

Nit: "… if the password field is empty."

::: browser/base/content/browser.css:1300
(Diff revision 2)
> +.popup-notification-password-empty {
> +  box-shadow: 0 0 1.5px 1px red;
> +}
> +
> +.popup-notification-password-empty[focused="true"] {

Nit: I think it wouldn't hurt to make this more reusable:
.popup-notification-invalid-input

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:812
(Diff revision 2)
> +        mainActionButton.setAttribute("disabled", false);

.removeAttribute is preferred for boolean attributes that default to false.

::: browser/base/content/browser.css:1295
(Diff revision 2)
> +.popup-notification-button-disabled {
> +  opacity: 0.5;
> +  cursor: not-allowed;
> +}

Why do we need a new class when we have the [disabled] attribute? Can't we using something like:
.popup-notification-menubutton > button-menubutton-button[disabled]

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:808
(Diff revision 2)
> +        mainActionButton.classList.add("popup-notification-button-disabled");

See above about not using a custom class for this.

::: browser/base/content/browser.css:1295
(Diff revision 2)
> +.popup-notification-button-disabled {
> +  opacity: 0.5;
> +  cursor: not-allowed;
> +}
> +
> +.popup-notification-password-empty {
> +  box-shadow: 0 0 1.5px 1px red;
> +}
> +
> +.popup-notification-password-empty[focused="true"] {
> +  box-shadow: 0 0 2px 2px rgba(255,0,0,0.4);
> +}

It would probably be nice to have this in the toolkit stylesheet. It's debatable about whether some of these properties belong in the content or skin stylesheet. Can you move these rules to toolkit/themes/\*/global/notification.css?
Attachment #8617769 - Flags: review?(MattN+bmo)
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

Bug 1153217 - allow editing password in doorhanger, disables button if password is empty
Attachment #8617769 - Flags: review?(MattN+bmo)
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

https://reviewboard.mozilla.org/r/10697/#review9643

::: toolkit/themes/linux/global/notification.css:94
(Diff revision 3)
> +  cursor: not-allowed;

Ryan thinks the cursor isn't necessary so please remove that x3
Attachment #8617769 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

Bug 1153217 - allow editing password in doorhanger, disables button if password is empty
Attachment #8617769 - Flags: review+ → review?(MattN+bmo)
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

https://reviewboard.mozilla.org/r/10697/#review9657

Ship It!
Attachment #8617769 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/4c1105d5b6a9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

Approval Request Comment
[Feature/regressing bug #]: This should get fixed in the same release as bug 1143903 and bug 1145913 to avoid confusing and churning of the login capture doorhanger UX.
[User impact if declined]: Confusion about a readonly field and churning of the login capture doorhanger UX.
[Describe test coverage new/current, TreeHerder]: Manual testing on Nightly (email sent to passwords-dev).
[Risks and why]: Fairly straightforward usage of some existing logic from the username field
[String/UUID change made/needed]: None
Attachment #8617769 - Flags: approval-mozilla-beta?
Attachment #8617769 - Flags: approval-mozilla-aurora?
Looks fantastic to me. Only detail that immediately comes to mind for me is that caret (text cursor) should always be placed at the end of the value, not at the beginning.
Thank you, Ryan. I filled a follow up bug for that : Bug 1174815.
Iteration: 41.2 - Jun 8 → 41.3 - Jun 29
Depends on: 1174942
Assigning to Kamil since as far as I know he was working on Password Manager. Kamil, please let me know if that's not the case anymore.
QA Contact: kjozwiak
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

That sounds like a bit late for beta but Liz will make the call. Anyway, taking it for 40.
Attachment #8617769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
> Assigning to Kamil since as far as I know he was working on Password
> Manager. Kamil, please let me know if that's not the case anymore.

Yup, that's still the case :) Thanks Florin!
That does look confusing and it would be nice to fix. Sylvestre is right, it is late for Beta since we will release beta 7 on Friday and then will be building the RC for 39 on Monday. So, if anything goes wrong because of this we would need to catch it and fix it over the weekend. 
Kamil and Matt are you prepared for that possibility?
Flags: needinfo?(kjozwiak)
Flags: needinfo?(MattN+bmo)
> That does look confusing and it would be nice to fix. Sylvestre is right, it
> is late for Beta since we will release beta 7 on Friday and then will be
> building the RC for 39 on Monday. So, if anything goes wrong because of this
> we would need to catch it and fix it over the weekend. 
> Kamil and Matt are you prepared for that possibility?

I know there were some discussions in today's pwd mgr meeting relating to this bug and if it should be merged into the b-c channel. I believe the team agreed to merge it but I'm not 100% sure. I'll let Matt answer :)

I will be available on Saturday morning and Sunday so if this lands, I have no problem going through testing/verification on the b-c channel if this ends up landing.
Flags: needinfo?(kjozwiak)
> Thank you, Ryan. I filled a follow up bug for that : Bug 1174815.

We should probably include this as well if we merge to the b-c channel.
(In reply to Kamil Jozwiak [:kjozwiak] from comment #33)
> > Thank you, Ryan. I filled a follow up bug for that : Bug 1174815.
> 
> We should probably include this as well if we merge to the b-c channel.

Yes, I just requested uplift for it too.

I will be able to do any last-minute landings or backouts.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

Approved for uplift to beta after discussion with MattN and Kamil. They will be on top of this over the weekend to test it out and fix if anything goes wrong in beta 7.
Attachment #8617769 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1175279
Comment on attachment 8617769 [details]
MozReview Request: Bug 1153217 - allow editing password in doorhanger, disables button if password is empty

I think we're going to let this bug bake on Aurora longer but leave the editable censored password on Beta.
Attachment #8617769 - Flags: approval-mozilla-beta+
Oops, I meant that wontfix for bug 1169702. We're going to allow editing but not make the text visible for 39 to get more feedback.
Depends on: 1177272
I was able to reproduce this issue on Firefox 41.0a1 (2015-05-14) using Windows 7 64-bit.

Verified fixed on Firefox 41.0b6 (20150831172306) on Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.4.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.