1171130 - Allow editing logins from the fill doorhanger subview

Status: RESOLVED WONTFIX

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

Bug 1164028 is adding the read-only subview with manual fill. This bug will allow making corrections to the login from the details view.

Comment 1

[Edwin Wong [:edwong]]

moving to next iteration

Comment 2

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1171130 - Allow editing logins from the fill doorhanger subview. r=Gijs

Bug 1171130 - Allow editing logins from the fill doorhanger subview. r=Gijs

Comment 3

[Matthew N. [:MattN]]

Hey Paolo, did you intend to flag Gijs for review on this?

Comment 4

[:Paolo Amadini]

Not sure. Since we're focusing on Tracking Protection, we might as well aim to land this patch later, when we resume working on the Login Fill Doorhanger.

Comment 5

[Matthew N. [:MattN]]

If it's done we may as well get a review pass on it. One of the interns may be able to work on the fill doorhanger while we're working on TP.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8625882 [details]
MozReview Request: Bug 1171130 - Allow editing logins from the fill doorhanger subview. r=Gijs

https://reviewboard.mozilla.org/r/11945/#review10849

I have too many questions for this to be an r+ first time 'round. Sorry!

::: browser/base/content/popup-notifications.inc:77
(Diff revision 1)
> +          <button id="login-fill-use" savelabel="Save"
> +                                      uselabel="Use in form"
> +                                      saveanduselabel="Save and use"/>

Where are we tracking l10n for this stuff? I hope this isn't enabled on devedition, ie hasn't merged? :-\

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:433
(Diff revision 1)
> +        // Explicitly set the password change time here (even though it would be
> +        // changed automatically), to ensure that it's exactly the same value as
> +        // timeLastUsed.

Can you give more details why this is necessary/useful?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:390
(Diff revision 1)
> +    setDisabled(this.el.revert, !hasChanged);

I think it would be preferable if we collapsed this button if it didn't apply, at least to start with (ie have it be invisible when the dialog is first opened). Maybe followup that and/or doublecheck with UX?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:456
(Diff revision 1)
> +      this.refreshList();

I would expect this to leave the detail view, but I don't see that happening in the code. Am I missing something?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:243
(Diff revision 1)
> +   * Current username for the login in the detail view, that could be different
> +   * from the one of detailLogin if the entry has been edited.

English nit:

Current username for the login in the detail view. This value may differ from detailLogin.username if the user edits the entry using the detail view.

(ditto for the password value)

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:416
(Diff revision 1)
> +      // If a login with the same username exists, it must be overwritten.

What happens if the user clears both username and password? I would expect the doorhanger to offer deleting the previously stored credential (or maybe to offer a delete button in its own right?). Right now, it seems like this will instead overwrite an existing credential (if any) with an empty password. That doesn't seem right.

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:424
(Diff revision 1)
> +          Services.logins.removeLogin(login);

This is also scary from a dataloss perspective...

If I have multiple logins on a site, what happens? Because if the detail view shows (or I select a login and cause it to show) and I then edit one login for another, I lose both original logins and keep only my new one (well, one gets removed and one gets replaced with the new login info). That seems counterintuitive.

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:427
(Diff revision 1)
> +      let propertyBag = Cc["@mozilla.org/hash-property-bag;1"]
> +                          .createInstance(Ci.nsIWritablePropertyBag);
> +      propertyBag.setProperty("username", this.detailUsername);

Yucky. I'm aware this isn't your fault, but we should really write sugar for (or deCOMtaminate) this. :-(

Comment 7

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #6)
> I have too many questions for this to be an r+ first time 'round. Sorry!

That's fine, thanks for taking the time for a first feedback pass!

This whole feature is disabled in all editions, in fact the entire doorhanger design is far from final and may change. The goal here was to get this panel in the tree as a first pass so we could reduce implementation risk and get more interactive feedback and testing from UX. We're not aiming to productize it at this stage since there may be some wasted effort. For example we're not adding strings to localize as we will likely change or remove them.

Now that most of the team is working on Tracking Protection I think we should revisit how we treat this.

Matt suggested in comment 5 that one of the interns could work on this. It's a possibility, though in my opinion the Logins Fill Doorhanger is lower priority (and more complexity) than the Logins Pad, and both are probably not getting UX resources in the short term. This may make life a bit more difficult.

I'm inclined to drop this entirely for now and resume work on it later with proper planning when we can make this feature Great. I would not be opposed to take the whole code out of the tree in the interim and putting it back later, if it helps maintenance.

I want to stress again that the current panel design is not how it will work in the end (despite it selectively matches some, but not all, of the dozens of mockups we have) and we cannot easily ship the current state to end users (tough if you're a developer you'll probably understand it anyways no matter how it's structured).

Comment 8

[Matthew N. [:MattN]]

We aren't going to ship the separate fill doorhanger (bug 1286718) so I'll close this for now. We can re-open if we want this feature in control center later on.