1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[:Paolo Amadini]

Implement an initial version of the sliding subview in the login fill doorhanger.

Comment 1

[:Paolo Amadini]

Attachment: MozReview Request: bz://1164028/paolo

/r/8913 - Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger.

Pull down this commit:

hg pull -r bc23fc0912d580ab0a5dc850dcb81041214a7789 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/8911/#review7589

This is an initial work in progress patch, not ready for a final review at this point.

The sliding view is implemented with simple CSS, borrowed from the sliding view of the main menu. The bindings created for the main menu cannot be used here without modifications because of the assumptions they make about their environment (in particular they don't expect the DOM element to be moved around, and they expect to be a direct child of the panel). In general, most of the features of the main menu (dynamic height, mutation observers) are not needed here.

Comment 3

[Edwin Wong [:edwong]]

Bill and I setting as P1

Comment 4

[:Paolo Amadini]

Comment on attachment 8607038 [details]
MozReview Request: bz://1164028/paolo

/r/8913 - Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger.

Pull down this commit:

hg pull -r 096a905c111f846f0a6052ad5ed816f07f32659e https://reviewboard-hg.mozilla.org/gecko/

Comment 5

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42ae5744ee17

Comment 6

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #5)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=42ae5744ee17

If people want to play with the experimental UI, quick reminder that it's currently behind the "signon.ui.experimental" preference in "about:config".

Comment 7

[:Paolo Amadini]

Comment on attachment 8607038 [details]
MozReview Request: bz://1164028/paolo

/r/8913 - Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger.

Pull down this commit:

hg pull -r 13633263fb5c7c289f1a3bc21d4a53a7039f69ed https://reviewboard-hg.mozilla.org/gecko/

Comment 8

[:Paolo Amadini]

Comment on attachment 8607038 [details]
MozReview Request: bz://1164028/paolo

Asking just for a preliminary review for now. I don't think we need additional automated UI testing at this stage, because the interface is experimental and will most likely change again, but the existing browser-chrome test still has to be updated to work with the new flow.

Comment 9

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/8911/#review8063

::: browser/base/content/browser.css:1325
(Diff revision 3)
> +#login-fill-mainview {
> +  transform: translateX(0);
> +  transition: transform 150ms;
> +}
> +
> +#login-fill-details {
> +  transform: translateX(0);
> +  transition: transform 150ms;
> +}

Seems like these two rules could be unified?

::: browser/themes/shared/login-doorhanger.inc.css:51
(Diff revision 3)
> +  color: var(--panel-arrowcontent-color);

Have you checked what happens if this is a light color? I wonder if this inherits into the textboxes and if so, if we need to do something there to make sure they continue to be readable.

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:44
(Diff revision 3)
> +    for (elementName of Object.keys(this.events))
> +    for (eventName of Object.keys(this.events[elementName]))
> +    [elementName, eventName, this.events[elementName][eventName].bind(this)]

While this is certainly very clever, it's also, IMHO, really hard to read. Can you change it to use loops instead?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:163
(Diff revision 3)
> -    this.filter.setAttribute("value", this.filterString);
> +      if (formLogins.length == 1) {

What happens if there are multiple forms?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:95
(Diff revision 3)
> +              doorhanger.bound = true;

This seems like something bind() should be doing, not this function.

Also, nothing ever sets this to false. That seems like a bug.

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:325
(Diff revision 3)
> +      list.removeChild(list.firstChild);

nit: list.firstChild.remove();

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:89
(Diff revision 3)
>                // Since we specified the "dismissed" option, this event will only
>                // be called after the "show" method returns, so the reference to
>                // "this.notification" will be available at this point.

This comment looks outdated; we don't use "this.notification".

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:356
(Diff revision 3)
> +      Cu.reportError("The selected login has been removed in the meantime.");

Are there no notifications for this event so we can avoid fizzling like this and remove the item from the list instead? Most users will not see the Cu.reportError and the click will seem to do nothing.

(I don't know how realistic it is that this would ever happen, AIUI the list will get reinitialized if the panel is reshown, and the panel will dismiss if you interact with anything outside it)

::: browser/base/content/browser.css:1313
(Diff revision 3)
> +#notification-popup[popupid="login-fill"] > .panel-arrowcontainer > .panel-arrowcontent {
> +  padding: 0;
> +}

Add a comment explaining why you're doing this. Also, why is this here and not in the themes CSS? Have you checked this on more than one OS?

::: browser/themes/shared/login-doorhanger.inc.css:3
(Diff revision 3)
> +  padding: 16px;

Shouldn't this be in em? Why is it this number, particularly?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:19
(Diff revision 3)
> +// Helper function needed because the "disabled" property is unreliable.

Explain why instead of just calling it "unreliable". This is basically because it's a XBL binding and because the binding might not have been constructed yet, in which case setting the property will create an expando.

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:295
(Diff revision 3)
>    refreshList() {

If I have a lot of logins, this is quite an expensive operation to run whenever I open this panel, especially if only one of them is applicable for this website and I'll therefore end up on the details view anyway. Can we avoid doing this on panel show in that case, and only do it if the user reverts to the page in question?

In general, I see a lot of "find me all the logins" followed by "filter the list to these usernames and hostnames". It seems like the findLogins API should be offering this functionality directly.

Comment 10

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #9)
> Have you checked what happens if this is a light color? I wonder if this
> inherits into the textboxes and if so, if we need to do something there to
> make sure they continue to be readable.

Will check, thanks!

> ::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:44
> While this is certainly very clever, it's also, IMHO, really hard to read.
> Can you change it to use loops instead?

In the end I think you're right, two for loops and Array.push will be more readable. See bug 980828 comment 1 for what I think would be even better, but unsupported for now.

> ::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:163
> (Diff revision 3)
> > -    this.filter.setAttribute("value", this.filterString);
> > +      if (formLogins.length == 1) {
> 
> What happens if there are multiple forms?

For the moment we only support filling forms with the same origin as the top-level page, and we only fill the first one we find, if that's the question.

> ::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:95
> Also, nothing ever sets this to false. That seems like a bug.

It is. Thanks for catching it!

> ::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:89
> (Diff revision 3)
> >                // Since we specified the "dismissed" option, this event will only
> >                // be called after the "show" method returns, so the reference to
> >                // "this.notification" will be available at this point.
> 
> This comment looks outdated; we don't use "this.notification".

I'll update it to say: "this.notification" in "bind()" will be available at this point.

> > +      Cu.reportError("The selected login has been removed in the meantime.");
> 
> (I don't know how realistic it is that this would ever happen, AIUI the list
> will get reinitialized if the panel is reshown, and the panel will dismiss
> if you interact with anything outside it)

This. No reason for the added complexity.

> ::: browser/base/content/browser.css:1313
> Add a comment explaining why you're doing this. Also, why is this here and
> not in the themes CSS? Have you checked this on more than one OS?

A similar rule for another notification is also not in the theme CSS. I also wondered why, and I thought the assumption was that we want it to be zero by default regardless.

But the reason might actually be that we didn't have the unified ".inc.css" for themes at the time and this was a reason to way to only write it once.

I haven't tested this on anything other than Mac OS X at present.

> ::: browser/themes/shared/login-doorhanger.inc.css:3
> (Diff revision 3)
> > +  padding: 16px;
> 
> Shouldn't this be in em? Why is it this number, particularly?

I can try and find a better value for this.

> If I have a lot of logins, this is quite an expensive operation to run

Simplicity over performance is one of the goals here. The structure of the panel is likely to change substantially and optimizing at this stage would be unwise.

Also, given how fast the operation is and that's it outside of any critical path, it's likely we won't have a strong optimization need, even later.

> In general, I see a lot of "find me all the logins" followed by "filter the
> list to these usernames and hostnames". It seems like the findLogins API
> should be offering this functionality directly.

We're quite constrained by the legacy XPCOM interfaces. I'm all for having a better non-XPCOM API, but it seems to me we're not going to de-XPCOMifying this anytime soon. A wrapper JSM could be feasible though, but I see this as a separate task.

Comment 11

[Edwin Wong [:edwong]]

I moved the iteration to 41.2.

Comment 12

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger. r=Gijs

Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger. r=Gijs

Comment 13

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/8913/#review9167

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:104
(Diff revisions 3 - 4)
> +                doorhanger.bound = false;

Whoops, leftover.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8616650 [details]
MozReview Request: Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger. r=Gijs

https://reviewboard.mozilla.org/r/8913/#review9169

::: browser/themes/shared/login-doorhanger.inc.css:73
(Diff revision 4)
> +  background: var(--panel-arrowcontent-background);
> +  color: var(--panel-arrowcontent-color);

Why is it necessary to reset these at all?

::: browser/themes/shared/login-doorhanger.inc.css:21
(Diff revision 4)
> +  transform: translateX(0);

Isn't this unnecessary?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:15
(Diff revision 4)
> +Cu.import("resource://gre/modules/Timer.jsm");

You don't seem to use this at all.

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:261
(Diff revision 4)
> +        if (event.button != 0 || !this.el.list.selectedItem) {
> +          return;
> +        }
> +        this.displaySelectedLoginDetails();

nit:

```
if (event.button == 0 && this.el.list.selectedItem) {
  this.displaySelectedLoginDetails();
}
```

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:267
(Diff revision 4)
> +        if (event.keyCode != Ci.nsIDOMKeyEvent.DOM_VK_RETURN ||
> +            !this.el.list.selectedItem) {
> +          return;
> +        }
> +        this.displaySelectedLoginDetails();

Same thing here.

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:357
(Diff revision 4)
> -        !this.list.selectedItem) {
> +    let formLogins = Services.logins.findLogins({}, "", "", null);

Why not:

let formLogins = Serices.logins.findLogins({}, selectedItem.getAttribute("hostname"), "", null);

?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm
(Diff revision 4)
> -    if (this.list.selectedItem.hasAttribute("disabled")) {
> -      return;
> -    }

Why were list items disabled before, and can that no longer happen?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:178
(Diff revision 4)
> -    this.list.addEventListener("dblclick", this.onListDblClick);
> +      this.el[elementName].addEventListener(eventName, handler, true);

This won't actually work because you've added bound handlers, and so the function references won't be the same.

Did this not show in your testing? This effectively makes me more worried about not having automated tests for this.

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> ::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:178
> (Diff revision 4)
> > -    this.list.addEventListener("dblclick", this.onListDblClick);
> > +      this.el[elementName].addEventListener(eventName, handler, true);
> 
> This won't actually work because you've added bound handlers, and so the
> function references won't be the same.
> 
> Did this not show in your testing? This effectively makes me more worried
> about not having automated tests for this.

Sorry, I misread this, so I've removed the comment.

Comment 16

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/8913/#review9175

> Why is it necessary to reset these at all?

Because otherwise the subview that slides in would have a fully transparent background.

> Why not:
> 
> let formLogins = Serices.logins.findLogins({}, selectedItem.getAttribute("hostname"), "", null);
> 
> ?

Slightly less readable, but works as well.

> Why were list items disabled before, and can that no longer happen?

In this first iteration, all list items appear as disabled when there is no login form in the page. Note how this in not necessarily part of the final design.

Previously, we used this to determine whether a double click would fill the login, and we could have just checked this.loginFormPresent. Now, the presence of the form determines whether the "Use in form" button is enabled to begin with, so no further check is needed.

Comment 17

[:Paolo Amadini]

Comment on attachment 8616650 [details]
MozReview Request: Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger. r=Gijs

Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger. r=Gijs

Comment 18

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/8911/#review9165

> Have you checked what happens if this is a light color? I wonder if this inherits into the textboxes and if so, if we need to do something there to make sure they continue to be readable.

Tested the panel on a high contrast theme on Windows and seems to work fine.

Comment 19

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #16)
> > Why not:
> > 
> > let formLogins = Serices.logins.findLogins({}, selectedItem.getAttribute("hostname"), "", null);
> > 
> > ?
> 
> Slightly less readable, but works as well.

Well, it filters the thing by hostname so you don't have to, and so the loginmanager doesn't give you all the logins, just the ones that could potentially apply. :-)

Is there some reason other than readability not to do that?

Comment 20

[:Gijs (he/him)]

Comment on attachment 8616650 [details]
MozReview Request: Bug 1164028 - Show login details and allow manual fill from a sliding subview in the login fill doorhanger. r=Gijs

https://reviewboard.mozilla.org/r/8913/#review9185

Ship It!

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:338
(Diff revision 5)
> -      return;
> +      this.el.username.setAttribute("value", this.detailLogin.username);

What happens if a login doesn't have a username associated with it? I'm assuming typeof this.detailLogin will be 'undefined'.

It seems to me like this will set the attribute to the string "undefined", and then later we will compare login.username (which is 'actual' undefined) to "undefined". That comparison will fail.

I don't know what kind of specialcasing we'd need to do here in order to get that case to work. r+ and followup for that issue, if that's OK with you.

Comment 21

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #20)
> I'm assuming typeof this.detailLogin will be 'undefined'.

Err, I meant the type of this.detailLogin.username.

Comment 22

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #19)
> Well, it filters the thing by hostname so you don't have to, and so the
> loginmanager doesn't give you all the logins, just the ones that could
> potentially apply. :-)
> 
> Is there some reason other than readability not to do that?

The point is that the Login Manager Service does basically the same filtering, as it works on an in-memory JavaScript array representation. But thinking it over, we might actually save some time because I believe the back-end currently decrypts the usernames and the passwords every time.

Comment 23

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #20)
> What happens if a login doesn't have a username associated with it? I'm
> assuming typeof this.detailLogin.username will be 'undefined'.

It should be the empty string "". I tested the case of no username and it works correctly.

Comment 24

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #22)
> (In reply to :Gijs Kruitbosch from comment #19)
> > Well, it filters the thing by hostname so you don't have to, and so the
> > loginmanager doesn't give you all the logins, just the ones that could
> > potentially apply. :-)
> > 
> > Is there some reason other than readability not to do that?
> 
> The point is that the Login Manager Service does basically the same
> filtering, as it works on an in-memory JavaScript array representation. But
> thinking it over, we might actually save some time because I believe the
> back-end currently decrypts the usernames and the passwords every time.

Hm. It sounds like the in-memory thing should be keyed by hostname already, though a discussion about why that should/shouldn't happen isn't really within the scope of this bug, I guess. :-)

Comment 25

[:Paolo Amadini]

(In reply to :Gijs Kruitbosch from comment #24)
> though a discussion about why that should/shouldn't happen isn't really
> within the scope of this bug, I guess. :-)

Yeah definitely :-)

Tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3354500fa045

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1391f87657d9

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1391f87657d9