376668 - Improve discoverability of login autocompletion (used with multiple accounts)

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement, P1)

Description

[Justin Dolske [:Dolske]]

When a user has multiple accounts on a site, password manager is currently a bit clunky to use. For forms, a username/password will not be filled in until a user enters a username (at which point they might discover we offer to autocomplete it). For HTTP authentication, password manager doesn't help at all.

We shouldn't make life difficult for multiple-account users, and the ability to use those accounts shouldn't be hard to discover/use.

Comment 2

[Arkady Sherman]

I've just moved from SeaMonkey - the multiple-account case is quite comfortable there, accounts are sorted by order of usage and asked with dialog every time the page loaded. Why it isn't so in FF?
I guess it's better to sort accounts by usage and autofill forms with one last used (first in the sorted list).

Comment 3

[Robert Kaiser]

The popup window of SeaMonkey is bad, I don't think we want that (neither for Firefox nor for future SeaMonkey).
IMHO, we should prefill the last used username/password, and on a "down" arrow key on the input field we should do a dropdown popup (in satchel style) that offers a list of saved user names. Additionally, we should offer some way to get this list via the mouse only, in context menu or whatever good possibility we have there.

Comment 4

[Arkady Sherman]

In that case It would be nice to have the login list sorted by usage order.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking on P2 items, but really-really want. Also adding myself and a uiwanted flag for fun and profit.

The "Skipper" add-on (http://www.sxipper.com/) has a pretty decent solution where they insert a little icon inside the textbox when there are userids stored.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Possible solutions (a lot of these are also mentioned in the dep bug 222653)

1. Replace the text field with a drop-down field to make it evident that the user can select from the N userIDs we have stored:

Sign into Gmail with your
  Google Account

  Username: [_________________|v]
  Password: [___________________]

The last option could be "Other..." which would put the text "Other..." in the field, highlighted such that typing would replace it.

potential problems...
 - what happens if the site was using JS to react to the content in the field? does this break that?
 - what happens if the field is styled to be small, or with a different background or appearance; will our styling totally break that?
 - what happens when we collide with Username fields that *are* drop-downs?


2. Add an indicator in the username field that indicates there are multiple logins to select from - the Sxipper (www.sxip.com) extention does something like this:

Sign into Gmail with your
  Google Account

  Username: [________________(#)]
  Password: [___________________]

Clicking on the button would spawn the drop-down

potential problems:
 - again, styling issues might end up being problematic for us, though maybe we could scale the little indicator graphic to fit?

3. Just show the drop down when the Username field gets focus:

Sign into Gmail with your
  Google Account

  Username: [_________________|v]
  Password: |userID1            |
            |userID2            |
            |userID3            |
            '-------------------' 

4. Show a <notificationbox> that asks the user to pick the login to use for this page.

I'm pretty sure that's in the order of my preference, too.

Comment 9

[Justin Dolske [:Dolske]]

#1 is what I had been thinking of as well. It's simple UI that users are familiar with, and is used for exactly for this situation.

* I don't think JS is a concern, as that would probably already be an issue with the current implementation. I think in both cases, the |value| just suddenly changes (as if the user had pasted a value, so it's not really out of the ordinary).

* I'd hope we can render small <select>-type widgets. Something to test...

* There shouldn't be a conflict problem with a site already using a <select>, because pwmgr will ignore such form controls (so it won't be working anyway :-).


A simple proof-of-concept would be to replace the form's <input type="text"> (for the username) with something like an editable <html:select> or <xul:menulist>. The tricky bit, though, would be to do this in a way that's transparent to script, so that it doesn't smells the same as a textfield. Maybe by modifying the existing autocomplete binding to optionally show a dropdown-widget?

Comment 10

[Georg Maaß]

I like the popup window of SM; the only thing about it is its coming so late probably after onload of the whole page. This timing forces me to wait for all the unwanted commercial pictures until the form is filled an can be sent back to the server.

The big advantage of the popup against any other solution is that the popup solution is obvious to the user to be "browser made" and not "page content". To make even the most stupid user understand, what is support provided by the browser and what is content (UI or whatever you like to call it) provided by the web developer.

The GUI of the browser must be familiar to the browser users and it must be impossible to emulate the native GUI of the browser through the website to phishing attacks by browser GUI emulation.

The popup solution is familiar from Mozilla to SM. Users know it and it can not be emulated by the website, which is important to remain impossible.

If you want to replace it by an other GUI, that other GUI must provide big advantages to the user. Advantages in the API are out of interest of the user. The user is never affected by the API. Never change the GUI without an added value to the user.

I do not see any advantage for the user when using a GUI integrated into the page that breaks the page layout or does not fit into the page layout. I also do not see any advantage for the user, if the user first must select any thing to get the list of accounts and then bless the mostly correct guess of the desired account to prefill the field. With the poopup we do not have any of these problems. The popup does not need to care about the layout of the page, because it is outside of the page. The popup does not need an additional activity of the user to be displayed. In most cases the default of the popup is exactly, what the user want's, so the only thing with the popup the user has to do is, hit OK and then submit the form.

This popup is a very good solution. It is very hard to have a better idea than the existing popup, which is a very good solution.

Comment 11

[Robert Kaiser]

A modal popup is IMHO very bad, and I'll be cheering for getting this out of SeaMonkey, really.
I hate that everytime I'm looking on a page of a site where I have multiple logins and only want to click a link on it, I have to click away this damn modal popup dialog in SeaMonkey. I wouldn't have to comment that in a Firefox bug, as Firefox UI designers are intelligent enough to have abolished it a long time ago. I don't agree with quite a few things in Firefox UI design, but getting rid of any modal dialog that is triggered by web page loading is a very good thing.

That said, I think what would be a good option for us (Firefox as well as SeaMonkey, i.e. anyone using LoginManager) would be to
1) prefill the last-used combination
2) do what bug 222653 proposes and just display the autocomplete popup on the name field at page load.

We're currently using a satchel popup, I think this should be filled from password manager instead of satchel though, as we want to selection of saved name/password combinations, not a selection of saved formfill values for the name field only in this case.

I think the most nasty thing developing this will be to sort out this satchel vs. password manager prefilling stuff.

Comment 12

[Justin Dolske [:Dolske]]

I've been looking at how to go about implementing "#1", as described in comments 8 and 9. I'm not really sure how to so such a thing sensibly. While the effect could be achieved by replacing the <html:input> in the page with the equivalent of a <xul:menulist editable=true>, such hamfisted mucking about in the page is sure to break things [see bug 242621?].

I think doing something like that would require some fairly deep changes to how <html:input type="text"> elements are implemented. That all seems to be widgety platform stuff, which I don't understand well and is in a feature freeze anyway. Perhaps this form of UI should be a long-term goal. [I see lots of webdev chatter about this kind of thing, maybe HTML5 should standardize it.]

Or maybe I'm wrong, and Neil can point out some XBL binding that can be twiddled to simply add a dropmarker when desired. I looked around but found naught for this.

Anyway, now I'm thinking a simpler thing to do would be a little more aggressive about showing the autocomplete dropdown (like Beltzner's #3 in comment 8), when the field is first focused (which might be at pageload, like Gmail). The argument for is a subtle shift from "discoverability" to "usability"...

* We already have autocomplete stuff all over (location bar, search bar, form history), and already show the autocompete popup when you start typing in such a field. In that sense, this feature isn't entirely undiscoverable. [If you had you press Control-Alt-Wonka-Z to see the autocomplete popup, that would be different.]

* What *is* tricky/annoying/hard to use here is how to select a login without using the keyboard. For example, I use the mouse to click a username field, the blank username field invites me to move my hands to the keyboard to type something, and then I move back to the mouse to select and entry and/or click Login. If the autocomplete dropdown was shown earlier, the mouse-kb-mouse switch wouldn't be called for.

Comment 13

[Robert Kaiser]

I think more agressively showing the popup is a good idea - just that the current popup is AFAIK not a popup showing the usernames for actually saved passwords, but a popup showing the previously entered usernames (i.e. a normal satchel popup).
In many cases, those lists are the same, but once you delete a password from password manager or even use shift+del in the popup to delete the entry from satchel, the lists go out of sync.
Ideally, we'd show a list provided by password/login manager...

And yes, the selection of a login by mouse only is currently hard - that's probably the major problem right now.

I'd envision doing those steps to make the user experience better here:
1) Fill the popup from login manager instead of satchel, make it not be a choice of autocomplete entries, but of all saved usernames.
2) Prefil the last used username and password immediately.
3) When the username input gets focus, always show the popup immediately.

Comment 14

[Neil Deakin]

(In reply to comment #12)

> Or maybe I'm wrong, and Neil can point out some XBL binding that can be
> twiddled to simply add a dropmarker when desired. I looked around but found
> naught for this.

Yeah, currently you would need to change the C++ code in order to insert an anonymous dropmarker.

Comment 15

[Justin Dolske [:Dolske]]

(In reply to comment #13)
> I think more agressively showing the popup is a good idea - just that the
> current popup is AFAIK not a popup showing the usernames for actually saved
> passwords, but a popup showing the previously entered usernames (i.e. a normal
> satchel popup).

Actually, the autocomplete popup is generated by Login Manager from the list of stored logins, the guts are at the botton of nsLoginManager.js. [Satchel is a little braindead and stores them too, although I hope to fix that -- bug 394612.]

Comment 16

[Justin Dolske [:Dolske]]

Attachment: Mostly works, WIP

This mostly works...

Gmail is causing something strange, though. It has a <body> onload handler that focuses the username field. When it does that, I now get an exception and the autocomplete popup won't open:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Permission denied to get property XULElement.popupOpen' when calling method: [nsIAutoCompletePopup::popupOpen]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: https://www.google.com/accounts/blahblahblah :: gaia_setFocus :: line 286"  data: no]
************************************************************

Comment 17

[Cees T.]

I use <a href="https://addons.mozilla.org/en-US/firefox/addon/4429">Secure Login</a>. It's nice and unobtrusive.

Comment 20

[Serra Labán, Eloy Alejandro Lautaro]

I think the best way is add a floating icon only when has the focus. If we click in the icon, it expands a list of usernames. I don't like popups, are obtrusive. In many cases, I have several usernames, but I use only one.

Comment 21

[Serra Labán, Eloy Alejandro Lautaro]

I say something like:

  Username: [___________________]    <---- Username is not focused
  Password: [___________________]

  Username: [___________________][@] <---- only when Username is focused
  Password: [___________________]

  Username: [___________________][@]__________
  Password: [___________________] |Username1  | <-- like a menu
                                  |Username2  |
                                   ───────────

Comment 22

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: proposed interface

>uiwanted

Here is a mockup of what I think we should go with.  The user name field is changed to a native and editable drop down control, and pre-populated with the most commonly used account.  Clicking anywhere to place a cursor insertion in the text area should select everything, similar to the location bar, typing clears the password field.

Sxipper has a really cool UI, but the overlay thing is something that an extension can get away with, but is likely to invasive for a Web browser to actively do to other people's content.  Also, their indicator for multiple accounts is used from branding, where here our interest is not to display a brand or metaphor, but just to convey that multiple options are available.  A platoform native drop is the most direct way to convey that.

Comment 23

[(no longer active)]

How well will this play with theming the input text box which is converted to a drop-down box?  Many websites may try to theme their input boxes in special ways which may look strange when that text box is converted to a drop-down box (especially since styling that won't work because of the nativeness).

Comment 24

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm not really sure, can we apply any visual styling to the drop down itself, like change the background color?  In general trying to style form elements doesn't work that well, since every platform has different types of widgets.

Comment 25

[Ivan Ičin]

Maybe it would be better if you used arrow like the one on the location bar, it is likely to cause less ambiguity across different styles. It could also have white outline so that it is visible on the dark backgrounds.

Comment 26

[(no longer active)]

Changing the text box to a drop-down box may ruin the design altogether.  For an example of a design which may hurt (these are just demos, not necessarily user name fields), see: http://www.games.com/

Comment 27

[B.J. Herbison]

Re:   Ehsan Akhgari Comment #26

Could you explain what you mean by "ruin" the deign, and what part of http://www.games.com/ would be ruined if one box looked different?

In my opinion, having one field look different is not a serious problem (especially a unique field that the user needs to focus on if they want to log in).  If there are multiple saved passwords then I would expect the user to typically log in and the design change goes away (and users without multiple saved passwords won't be affected).

Comment 28

[Robert Kaiser]

Please read comment #8 and the following comments before arguing about what problems this design has, we have had this discussion before.

I think the current proposal is something we can go with, I'd even like better if the dropdown (of the first such field in a doc) would be opened and focused though to make this even more discoverable.

Comment 29

[(no longer active)]

Attachment: Change in the layout when using <select> instead of <input>

(In reply to comment #27)
> Could you explain what you mean by "ruin" the deign, and what part of
> http://www.games.com/ would be ruined if one box looked different?

Please see this actual screenshot of the search box control when using a normal text box, and using a drop-down box.  I created the second sample in DOM Inspector by removing the <input> element and putting a <select> element in place of it and applying all of the text box CSS styles to it manually.

Comment 30

[Neil Deakin]

I think what we want to do here is just add an arrow inside the input as native anonymous content with the popup attached to it.

Note also that in Alex's screenshot, it looks like he's using an editable <menulist> which currently doesn't actually use the native theme properly -- it's actually just a normal mennulist with an edit field superimposed onto it. It should actually be rendered just as a textbox with an single-down arrow in it, like it in Date/time control panel. So making this change and similar on other platforms may be what is needed.

Comment 31

[Philip Chee]

> I think what we want to do here is just add an arrow inside the input as native
> anonymous content with the popup attached to it.

Instead of an arrow, how about a different graphic, for example a lock or a key icon (or what does the Opera Wand widget use anyway?).

Comment 32

[Alex Faaborg [:faaborg] (Firefox UX)]

>Note also that in Alex's screenshot, it looks like he's using an editable
><menulist> which currently doesn't actually use the native theme properly

Yep, after posting that screenshot I was thinking that control looked odd, the bug to use the native OS theme properly on OS X is bug 469631.

Another option is to use the native control if the page hasn't modified the appearance of the field.  If they have only changed the background color, we could try to do that as well to the drop down control, and if they have removed the border, we could then fall back to our our own artwork for the drop down arrow that will work on top of a full range of background colors.  However if they haven't made any changes to the appearance of the field, I think using the native drop down is ideal.

Comment 33

[Robert Kaiser]

I think getting the functionality figured our is more important than the details of the used graphics. I like the functionality described by Alex in comment #22 and wonder what the plans are for implementing this? 3.1 timeframe? 3.2?

Comment 34

[Alex Faaborg [:faaborg] (Firefox UX)]

Probably 3.2 at the earliest, and hopefully as part of a larger set of changes related to managing online identity (but all of those brainstorming sessions haven't started yet).

Comment 36

[Erik Rose [:erik][:erikrose]]

I would love to see some motion on this. I've thought for *years* that FF was just bad at remembering passwords, when really the problem was just undiscoverability. When I mentioned this to another webdev, sitting next to me, he was equally surprised. A friend of his used to use FF just because it remembered his WiFi interstitial login, and he switched away when it inexplicably stopped.

Comment 37

[Fred Wenzel [:wenzel]]

Real-life data point: I recently encountered a long-time, non-geek Firefox user who loved Firefox for the fact that it would automatically typed in his password into his company's silly interstitial page whenever opening a browser. One day it stopped working. After not figuring out how to get it back, he considered switching browsers to IE or Firefox, because this vital piece of daily interaction was seemingly unrecoverably gone.

After I heard about it, I told him how to get to the saved passwords list and remove the duplicate credentials that somehow got in there, and ta-dah, it works again.

If it takes people like Erik years to find out what's going on, then I am not surprised regular users give up on Firefox altogether because they can't figure it out.

Comment 38

[Fred Wenzel [:wenzel]]

(In reply to Fred Wenzel [:wenzel] from comment #37)
> After not figuring out how to get it
> back, he considered switching browsers to IE or Firefox

IE or Chrome, I mean.

Comment 40

[Robert Kaiser]

We could team this UX improvement up with the bug 759860 work, which would fix a long-standing security issue (which we recently aggravated due to storing more passwords in sensitive sites, see bug 956906).

Comment 41

[Matthew N. [:MattN]]

Safari implemented #2 and #3 from comment 8.

Comment 42

[Tanvi Vyas[:tanvi]]

The minimum we could do here to get the biggest user benefit quickly is number 3 in comment 8.  When the user focuses the password field, show the autocomplete UI (instead of requiring them to start typing before showing the drop down).

Comment 43

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d216530e212

Comment 44

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87cfc9561af3

Comment 45

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #42)
> The minimum we could do here to get the biggest user benefit quickly is
> number 3 in comment 8.  When the user focuses the password field, show the
> autocomplete UI (instead of requiring them to start typing before showing
> the drop down).

Hi Matt,

Jonathan is working on implementing what is described above.  Please let us know if this is not sufficient to close this bug.

Thanks!

Comment 46

[Matthew N. [:MattN]]

If by "password field" you meant "username or password field" then yes, that's what I had in mind for now. It's a step towards what Safari has (minus the field indicator). We can always improve the UX from there.

Comment 47

[Jonathan Kingston [:jkt] he/him]

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

Comment 48

[Jonathan Kingston [:jkt] he/him]

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

Comment 49

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 376668 - making login fields show autocomplete on focus

Review commit: https://reviewboard.mozilla.org/r/86444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/86444/

Comment 50

[Matthew N. [:MattN]]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review85248

::: toolkit/components/satchel/nsFormFillController.cpp:277
(Diff revision 1)
> +  // Check if the element has already been focused prior to form fill being attached
> +  if (!userTriggered) {
> +    nsFocusManager *fm = nsFocusManager::GetFocusManager();

Hey Jonathan, if you want to handle this case in a separate bug (or separate commit) I'd prefer that. The advantage of that would be landing the code to handle focus after marking sooner (which is more common). Then we can get more Nightly testing and have more focused review/tests.

Comment 51

[Jonathan Kingston [:jkt] he/him]

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

Comment 52

[Jonathan Kingston [:jkt] he/him]

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

Comment 53

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86444/diff/1-2/

Comment 54

[Jonathan Kingston [:jkt] he/him]

Hey Matt,
Ignore that review request, I'm not sure if at this moment is makes sense to seperate as more tests fail without the code you mention.
Thanks

Comment 55

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20cf40195eec

Comment 56

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86444/diff/2-3/

Comment 57

[Jonathan Kingston [:jkt] he/him]

Hey Matt,
I think the latest code doesn't have any of the failures now, would you be able to check it over and let me know if there are any issues?
Thanks

Comment 58

[Tanvi Vyas[:tanvi]]

Marking as P1 because this needs to land in Firefox 52.

Comment 59

[Matthew N. [:MattN]]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review86894

Hey Jonathan,

::: toolkit/components/satchel/nsFormFillController.h:55
(Diff revision 3)
>    NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsFormFillController, nsIFormFillController)
>  
>    nsresult Focus(nsIDOMEvent* aEvent);
>    nsresult KeyPress(nsIDOMEvent* aKeyEvent);
>    nsresult MouseDown(nsIDOMEvent* aMouseEvent);
> +  nsresult ShowPopup();

Why is this public instead of protected?

::: toolkit/components/satchel/nsFormFillController.cpp:937
(Diff revision 3)
> +  if (!isTrusted && mPwmgrInputs.Get(mFocusedInputNode)) {
> +    ShowPopup();
> +  }

Why are you negating `isTrusted`? Isn't that the opposite of what we want?

I confirmed with manual testing that this is incorrect at for my understanding of what we're doing.

::: toolkit/components/satchel/nsFormFillController.cpp:938
(Diff revision 3)
> +  // if we have a password manager field auto show the popup
> +  bool isTrusted = false;
> +  nsresult rv = aEvent->GetIsTrusted(&isTrusted);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (!isTrusted && mPwmgrInputs.Get(mFocusedInputNode)) {
> +    ShowPopup();

If fixing isTrusted doesn't require fixing any tests then please add a task to test_basic_form_autocomplete.html that ensure that the popup shows up a trusted focus event. I'm not sure but I think you may be able to use synthesizeMouseAtCenter(…) fot this.

Comment 60

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review86894

> Why are you negating `isTrusted`? Isn't that the opposite of what we want?
> 
> I confirmed with manual testing that this is incorrect at for my understanding of what we're doing.

This is to prevent trusted events from reopening the popup after the user has selected a password, I was getting a focus event which was reshowing the autocomplete popup. I will add more of a comment about why it is there in the code and ensure it is tested.

Comment 61

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review86894

> This is to prevent trusted events from reopening the popup after the user has selected a password, I was getting a focus event which was reshowing the autocomplete popup. I will add more of a comment about why it is there in the code and ensure it is tested.

I was able to prevent default on the firing of that selection event (I think it was in LoginManagerContent.jsm but that seemed more drastic than this) Will try and find the code that is causing the need for this, perhaps there is a different approach.

Comment 62

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d28585cb38

Comment 63

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review86894

> I was able to prevent default on the firing of that selection event (I think it was in LoginManagerContent.jsm but that seemed more drastic than this) Will try and find the code that is causing the need for this, perhaps there is a different approach.

For some reason this was no longer needed, I suspect it was there as part of the other code I removed. Anyway I added the test as asked after removing :).

Comment 64

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86444/diff/3-4/

Comment 65

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b2fbb077a57

Comment 66

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ccd743ac8a2

Comment 67

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86444/diff/4-5/

Comment 68

[Jonathan Kingston [:jkt] he/him]

After extensive testing I can't reproduce the time outs produced in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b2fbb077a57&selectedJob=29785473
However removing the isTrusted check causes the issue to happen.

I added the test you requested in toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html which checks that trusted triggered events still work the same.

Comment 69

[Matthew N. [:MattN]]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review87132

::: toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html:807
(Diff revision 4)
>    yield processedPromise;
>    checkACForm("testuser", "testpass");
>  });
> +
> +//Check a isTrusted event opens the autocomplete popup
> +add_task(function* test_form12_trusted() {

"test_form12_trusted" doesn't tell me that this is testing opening the popup upon focus. How about test_form12_open_on_focus? Ideally the test name should remove the need for a code comment.

::: toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html:815
(Diff revision 4)
> +    uname.addEventListener('click', (e) => {
> +      ok(e.isTrusted, 'Ensure event is trusted');

Nit: double quotes

::: toolkit/components/satchel/nsFormFillController.cpp:938
(Diff revisions 4 - 5)
>    // if we have a password manager field auto show the popup
> -  if (mPwmgrInputs.Get(mFocusedInputNode)) {
> +  // Also check we don't have a contextmenu triggered event
> +  bool isTrusted = false;
> +  nsresult rv = aEvent->GetIsTrusted(&isTrusted);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (!isTrusted && mPwmgrInputs.Get(mFocusedInputNode)) {

If anything we want `isTrusted` or to not check isTrusted at all. Only showing the popup on untrusted focus events just doesn't make sense to me. I thought this bug was about the user focusing the field (trusted) and bug 1311301 was about the page focusing (untrusted).

Comment 70

[Matthew N. [:MattN]]

I'm concerned about us making form autocomplete stealing attacks easier by allowing untrusted focus events to open the login manager autocomplete popup as it's one less keypress than an attacker needs to trick the user into pressing. Dan, what do you think?

We should also see how Safari handles this. In my quick test the other day it seemed like they don't auto-open for an untrusted focus event.

Comment 71

[Jonathan Kingston [:jkt] he/him]

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

Comment 72

[Jonathan Kingston [:jkt] he/him]

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

Comment 73

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57e976ee5a3d

Comment 74

[Daniel Veditz [:dveditz]]

(In reply to Matthew N. [:MattN] (PM me if I'm slow responding) from comment #70)
> I'm concerned about us making form autocomplete stealing attacks easier by
> allowing untrusted focus events to open the login manager autocomplete popup
> as it's one less keypress than an attacker needs to trick the user into
> pressing. Dan, what do you think?

I agree! We should only show this prompt on genuine user interaction.

Comment 75

[Jonathan Kingston [:jkt] he/him]

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

Comment 76

[Daniel Veditz [:dveditz]]

(In reply to Daniel Veditz [:dveditz] from comment #74)
> We should only show this prompt on genuine user interaction.

Tanvi has helped me work through scenarios and come to the conclusion that 
==> as long as this is only passwords and does not infect form-fill autocomplete <==
all of my concerns are general password manager concerns (e.g. bug 610997, bug 786276) and that this doesn't make them any worse.

Comment 77

[Tanvi Vyas[:tanvi]]

Hi Jonathan,

When we spoke yesterday, I believe you said that your test failure had to do with IsTrusted.  Since we know longer need to worry about the value of IsTrusted, have all the test failures been resolved?

Even if they haven't been, can you put your latest patch up for review?

Thanks!

Comment 78

[Jonathan Kingston [:jkt] he/him]

Attachment: Latest WIP tests still fail

This attached WIP works other than that try tests are still failing.

The changes in toolkit/components/passwordmgr/test/browser/browser_context_menu.js are to prevent CPOW fails when running ./mach mochitest --run-until-failure however they don't prevent the issues in the try failing which I was unable to replicate locally.

The timeout fails are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57e976ee5a3d&selectedJob=30029959

As you can see from the debugging the failures happen waiting for: https://hg.mozilla.org/try/rev/57e976ee5a3db19b188deae2f37eece2b6aee3bb#l1.76

The patch also includes a test for isTrusted events too: https://hg.mozilla.org/try/rev/57e976ee5a3db19b188deae2f37eece2b6aee3bb#l2.14

The isTrusted check was removed from the code: https://hg.mozilla.org/try/rev/57e976ee5a3db19b188deae2f37eece2b6aee3bb#l3.14

/cc tanvi - I will rebase my other patches then continue working on this as I am mostly well again

Comment 79

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7821ecdf4a02

Comment 80

[Jonathan Kingston [:jkt] he/him]

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

Comment 81

[Tanvi Vyas[:tanvi]]

Clearing my needinfo.  Jonathan, please put up your current patch for review, even if the context menu test is still failing.

Comment 82

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86444/diff/5-6/

Comment 83

[Jonathan Kingston [:jkt] he/him]

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

Comment 84

[Jonathan Kingston [:jkt] he/him]

So I have pushed to try a broken test that I was working on to test the overlapping of both the autocomplete and the context menu on the input field. However for some reason it still fails with the latest change which fixes it locally:

    if (mFocusedInput)
      StopControllingInput();


I think this test is broken because the simulated click isn't like a normal right click, however adding type "contextmenu" doesn't simulate the issue either.

Dale, I will be mostly out today and tomorrow so I'm not sure if you needed this update etc. Feel free to ping me but I won't be very available.

The test that is still failing is the timeouts mentioned above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc74f9481a7dc6a46c19bf2dce8979502807a8d8&selectedJob=30783285

Comment 85

[Jonathan Kingston [:jkt] he/him]

Hey MattN, just keeping you in the loop of the above.
Thanks

Comment 86

[Dale Harvey (:daleharvey)]

So this is a hard blocker for https://bugzilla.mozilla.org/show_bug.cgi?id=1311301, (it cant land before this) and Jonathan is mostly out and this needs to land tonight / tomorrow I am stealing, currently debugging the test failure. 

Currently from the logs I am seeing 

12:53:21     INFO - A coding exception was thrown and uncaught in a Task.
12:53:21     INFO - Full message: TypeError: BrowserTestUtils is null
12:53:21     INFO - Full stack: test_context_menu_doesnt_overlap_with_autocomplete/<@chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_context_menu.js:71:11
12:53:21     INFO - test_context_menu_doesnt_overlap_with_autocomplete@chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_context_menu.js:54:9
12:53:21     INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:42
12:53:21     INFO - TaskImpl@resource://gre/modules/Task.jsm:277:3
12:53:21     INFO - createAsyncFunction/asyncFunction@resource://gre/modul

Which seems like a very strange error to be having considering BrowserTestUtils is a pretty core part of the test framework (for some reason that line 71 that it is pointing to is just a comment?), right now doing a try run to figure out if its the changed code or the new test thats causing the error

Comment 87

[Dale Harvey (:daleharvey)]

Also worth mentioning the tests are passing locally on linux and osx fine, the failure seems to be on linux64 on try only ¯\_(ツ)_/¯

Comment 88

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review92138

::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:58
(Diff revision 6)
> +  Services.prefs.setBoolPref("signon.schemeUpgrades", false);
> +  yield BrowserTestUtils.withNewTab({
> +    gBrowser,
> +    url: TEST_HOSTNAME + MULTIPLE_FORMS_PAGE_PATH,
> +  }, function* (browser) {
> +    let passwordInput = browser.contentWindow.document.getElementById("test-password-1");

Even though it's allowed in tests, we must avoid attempting to access the browser contentWindow directly due to e10s (since this goes over the shims, and there are lifetime issues there, since references in the parent don't keep the content window in the child alive).

BrowserTestUtils.synthesizeMouseAtCenter actually takes a CSS selector as an optional first argument. You should probably pass "#test-password-1" to it.

Comment 89

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review92140

::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:61
(Diff revision 6)
> +    url: TEST_HOSTNAME + MULTIPLE_FORMS_PAGE_PATH,
> +  }, function* (browser) {
> +    let passwordInput = browser.contentWindow.document.getElementById("test-password-1");
> +
> +    // Synthesize a right mouse click over the password input element.
> +    let contextMenuShownPromise = BrowserTestUtils.waitForEvent(window, "popupshown");

We should set this event listener on the context menu popup itself directly instead of the browser window (which is what `window` is bound to) - that popup has the ID `contentAreaContextMenu` and can be safely gotten from within the parent process, since it exists in the browser UI (in fact, this test does that on line 66 - so just move that above where you're setting the popupshown event handler)

Comment 90

[Jonathan Kingston [:jkt] he/him]

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

Comment 91

[Jonathan Kingston [:jkt] he/him]

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

Comment 92

[Dale Harvey (:daleharvey)]

So a quick update from what I have found debugging, the issues with the tests may need looked at but dont seem to be the problems with the failures. The code

     aEvent->InternalDOMEvent()->GetTarget());
   MaybeStartControllingInput(input);
+
+  // if we have a password manager field auto show the popup
+  if (mPwmgrInputs.Get(mFocusedInputNode)) {
+    ShowPopup();
+  }
+

alone causes the tests to go from passing to timing out so I think figuring out why its hanging will need to be done. (I am not getting the one click loaner button on my treeherder for some reason?)

I took a stab in the dark, having the ShowPopup there seemed risky as there are other checks that happen inside maybeStartControllingInput, so doing a quick run to see if moving the showpopup to inside maybestartcontrollinginput helps (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14530380e0670fdc82e059ed1dab5277782b538) although I would be surprised if it did.

Going to bed now though, will get back to this in the morning unless fingers crossed someone figures it out while I am sleeping

Comment 93

[Jonathan Kingston [:jkt] he/him]

Assuming no increase in issues from Dales push I think the change makes sense for clarity.

I have pushed a try to clean up the review from Mike and see if that removes the timeout.

Comment 94

[Matthew N. [:MattN]]

Comment on attachment 8801803 [details]
Bug 376668 - making login fields show autocomplete on focus

https://reviewboard.mozilla.org/r/86444/#review92224

Nothing jumps out as obviously wrong here but it seems like this will change a bit with the patches on try so clearing for now.

Comment 95

[Dale Harvey (:daleharvey)]

Ok my change actually makes things worse, |maybeStartControllingInput| gets called more than |Focus| so this brings the popup in cases that it is not wanted

I have however managed to reproduce locally, took me way to long to realise this is only failing with e10s disabled, so $ ./mach test toolkit/components/passwordmgr/test/browser/browser_context_menu.js --disable-e10s reproduces it

Comment 96

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24d3bb110812

Comment 97

[Jonathan Kingston [:jkt] he/him]

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

Comment 98

[Jonathan Kingston [:jkt] he/him]

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

Comment 99

[Dale Harvey (:daleharvey)]

Just to update on what the problem is (looks like Jonathan and I are debugging the same issue), basically when 

  // if we have a password manager field auto show the popup
  if (mPwmgrInputs.Get(mFocusedInputNode)) {
    ShowPopup();
  }

is added to the end of http://searchfox.org/mozilla-central/source/toolkit/components/satchel/nsFormFillController.cpp#932, the call to that showpopup (from the previous test) interferes with the ability for us to trigger the next contextmenu @ http://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_context_menu.js#241, here we fire a right click, in the previous times this is called we see |nsFormFillController::handleEvent| seeing a 'contextmenu' -> 'focus' series of events, however with that patch applied I am seeing 'mousedown' -> 'focus' events. It seems possible that a previous popup is interferring with the events somehow (and maybe forwarding them incorrectly?) in a way that turns the right click into a normal click

Comment 100

[Dale Harvey (:daleharvey)]

And not the correct fix, but one fix for this is 

+          browser.contentWindow.document.activeElement.blur();
+          return true;

around searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/browse r/browser_context_menu.js#214 blurs the previous element and avoids the right click being modified to a left click

Comment 101

[Dale Harvey (:daleharvey)]

ok, the issue is that the popup that now gets shown when it wouldnt have before, it covers the area we are right clicking which prevents the right click from showing, will have a new patch up shortly

Comment 102

[Dale Harvey (:daleharvey)]

Attachment: Make login fields show autocomplete on focus

So the previous issue was that the previously unshown autocomplete popup was covering the next input we were trying to trigger a context menu on, so now we blur the previous element removing the autocomplete popup.

Pushed to try in https://treeherder.mozilla.org/#/jobs?repo=try&revision=e38a5635e6e64c290c12824892fc6fee8d60d9ae

Comment 103

[Dale Harvey (:daleharvey)]

Comment on attachment 8810361 [details] [diff] [review]
Make login fields show autocomplete on focus

Clearing review since the tests are still failing :(

Comment 104

[Dale Harvey (:daleharvey)]

ok so we receive the contextmenu event before the focus when we right click, which means the popup could get shown when the contextmenu was active, changed the patch to prevent that and pushed to try @ https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcb5e4118f74a7c0a364b919b8955d8f8d19f52f

Comment 105

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=377d15a40fbf

Comment 106

[Dale Harvey (:daleharvey)]

ok so having issues with preventing the popup showing when the contextmenu is triggered, the events look like this:

Processing test-username-2
Send mousedown type=contextmenu to test-username-2
nsFormFillController::HandleEvent::contextmenu
nsFormFillController::HandleEvent::focus
nsFormFillController::ShowPopup called
nsFormFillController::ShowPopup completed
The contextmenu is now shown

I have added the following to ShowPopup

  nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
  if (pm && pm->HasContextMenu(nullptr)) {
    return NS_OK;
  }

Now the contextmenu event is called before the focus however this code does not see the contextmenu and as shown completes showing the popup. Its possible that the code is racy and the event is fired before the code can see the contextmenu, it may be the wrong code? Another possible solution could be if we can figure out that the focus came from a contextmenu click to avoid calling showpopup in that scenario.

Comment 107

[Dale Harvey (:daleharvey)]

Small update, after talking to Mike I used his suggestion, the nsXULPopupManager is going to have race problems since it is in the child process, but AutoCompletePopup.jsm is in the parent and can reliably detect the contextmenu, so:

    // Lets not show the autocomplete when we have a contextmenu
    if (window.gContextMenu) {
      return;
    }

in http://searchfox.org/mozilla-central/source/toolkit/components/satchel/AutoCompletePopup.jsm#139 fixes the original issue reliably as far as I can tell, it does however cause a different test failure which I am looking into now

Comment 108

[Dale Harvey (:daleharvey)]

Another quick update, the above helped a lot but still failing tests (intermittently) so far it looks like that when the contextmenu fills in the input the autocomplete will be shown when we dont want it to, as far as I can tell this isnt coming from nsFormFillController itself but somewhere else (nsAutoCompleteController seems likely) carrying in investigation

Comment 109

[Dale Harvey (:daleharvey)]

Getting there slowly, the issue with having the parent not show the popup when the contextmenu is active is that the autocomplete controller has already by this time started doing (async searches) if it sees the results after the contextmenu has closed then it will still show them. 

We could send something back when the parent decides to not show the popup to try and stop searches etc, but that seems similiarly problematic to the current races. I believe the only reasonable way to fix this is to not trigger the extra ShowPopup(), Mike has said that determining whether the "focus" event resulted from a right click alone is not possible, however the focus event always directly proceeds a contextmenu event so having a flag set on the contextmenu event to ignore the next focus event works great.

This still has further fallout however :( I am now seeing a problem where if I click directly from one autocomplete controller input to another the popup is attached to the previously focused input, the child code in |nsFormFillController::StartControllingInput| looks to prevent that from happening, but maybe the parent is getting the message in time, debugging now

Comment 110

[Dale Harvey (:daleharvey)]

Attachment: Make login fields show autocomplete on focus

Ok found the cause of the swiching inputs, if we went from one input to another directly we never actually closed the previous one so content just itself mixed up.

Will do a try run before asking review, made this mistake before
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c848993528b047ff5a1a83f749b87e4d8ee4d300

Comment 111

[Dale Harvey (:daleharvey)]

Comment on attachment 8811737 [details] [diff] [review]
Make login fields show autocomplete on focus

So |mContextMenuFired| is used as we do not want to call ShowPopup when we receive focus due to a contextmenu being pressed over the input.

+  if (mFocusedPopup) {
+    mFocusedPopup->ClosePopup();
+  }

Is because we did not have code that told the frontend to close the current popup when we went directy from one input to another, |ClosePopup| was called eventually by http://searchfox.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1175 however by the time this was called |mFocusedPopup| was already nulled but not closed here

Comment 112

[Matthew N. [:MattN]]

Comment on attachment 8811737 [details] [diff] [review]
Make login fields show autocomplete on focus

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

Thanks for your persistence and help on this bug Dale! :)

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +68,5 @@
>    mListNode(nullptr),
>    mTimeout(50),
>    mMinResultsForPopup(1),
>    mMaxRows(0),
> +  mContextMenuFired(false),

mContextMenuFiredBeforeFocus to make its purpose more clear? *shrugs*

@@ +934,5 @@
>    nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(
>      aEvent->InternalDOMEvent()->GetTarget());
>    MaybeStartControllingInput(input);
> +
> +  // If this focus doesnt immediately follow a contextmenu event then show

Nit: missing apostrophe in "doesn't"

@@ +936,5 @@
>    MaybeStartControllingInput(input);
> +
> +  // If this focus doesnt immediately follow a contextmenu event then show
> +  // the autocomplete popup
> +  if (!mContextMenuFired && mPwmgrInputs.Get(mFocusedInputNode)) {

An alternate solution that I mentioned to jkt on Wednesday is to check `relatedTarget` of the focus event (aEvent) which tells you the element that was previously focused. Not sure if that would indicate something in the context menu for the problem case or not. The advantage is that if that worked, it wouldn't require any state member variable.

Comment 113

[Dale Harvey (:daleharvey)]

Yeh I did a test for relatedTarget but it doesnt give us any information about whether this came from a contextmenu or not, the behaviour is the same whether we left of right click on an element. I did think there was some idea of |originalEvent| but it seems I was just misremembering from jquery days. 

Thanks for the review, will make those changes and land

Comment 114

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace6ae91b376
Make login fields show autocomplete on focus. r=mattn

Comment 115

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ace6ae91b376

Comment 116

[Jonathan Kingston [:jkt] he/him]

Amazing! So happy this is finally gone, Thanks Dale!

Comment 117

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8811737 [details] [diff] [review]
Make login fields show autocomplete on focus

Approval Request Comment
[Feature/regressing bug #]: Feature is provided by this bug: 376668
[User impact if declined]: Users won't easily see the insecure password warning if they don't get this patch
[Describe test coverage new/current, TreeHerder]: toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html has a new test for trusted focus.
[Risks and why]: Users currently have to double focus the box or start typing to get their logins, this makes the visibility pretty hard of the insecurity.
[String/UUID change made/needed]:

Comment 118

[Julien Cristau [:jcristau]]

Comment on attachment 8811737 [details] [diff] [review]
Make login fields show autocomplete on focus

login field autocomplete fix for aurora52

Comment 119

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f320d5e8e90f

Comment 121

[Matthew N. [:MattN]]

(In reply to Dale Harvey (:daleharvey) from comment #106)
> ok so having issues with preventing the popup showing when the contextmenu
> is triggered, the events look like this:
> 
> Processing test-username-2
> Send mousedown type=contextmenu to test-username-2
> nsFormFillController::HandleEvent::contextmenu
> nsFormFillController::HandleEvent::focus
> nsFormFillController::ShowPopup called
> nsFormFillController::ShowPopup completed
> The contextmenu is now shown

Dale, which OS was this on? I'm seeing focus before contextmenu on macOS and I think this may differ by platform. It may be related to NS_CONTEXT_MENU_IS_MOUSEUP.

Comment 122

[Dale Harvey (:daleharvey)]

This was on OSX 10.11.5

Comment 123

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The issue is verified fixed on 53.0a2 (2017-02-27) and 52.0b9 (20170223185858), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.
Setting the corresponding flags.