Closed Bug 376668 Opened 17 years ago Closed 8 years ago

Improve discoverability of login autocompletion (used with multiple accounts)

Categories

(Toolkit :: Password Manager, enhancement, P1)

Product:
Toolkit
Component:
Password Manager
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- verified
firefox53 --- verified

People

(Reporter: Dolske, Assigned: daleharvey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [passwords:fill-UI] PRD:PASS-001d)

Attachments

(3 files, 4 obsolete files)

proposed interface
36.21 KB, image/png
Details
Change in the layout when using <select> instead of <input>
3.42 KB, image/png
Details
Make login fields show autocomplete on focus
6.09 KB, patch
MattN
: review+
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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.
Blocks: 371000
Whiteboard: PRD:PASS-001d
Depends on: 222653
Flags: blocking-firefox3?
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).
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.
In that case It would be nice to have the login list sorted by usage order.
Flags: blocking-firefox3? → blocking-firefox3-
Keywords: uiwanted
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.
Whiteboard: PRD:PASS-001d → PRD:PASS-001d [wanted-firefox3]
Depends on: 227632
Depends on: 234174
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.
#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?
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.
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.
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.
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.
(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.

(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.]
Attached patch Mostly works, WIP (obsolete) — — Splinter Review 
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]
************************************************************
Flags: wanted-firefox3+
Whiteboard: PRD:PASS-001d [wanted-firefox3] → PRD:PASS-001d
Assignee: dolske → nobody
Target Milestone: Firefox 3 → ---
I use <a href="https://addons.mozilla.org/en-US/firefox/addon/4429">Secure Login</a>. It's nice and unobtrusive.
Flags: in-testsuite-
Flags: in-litmus?
Flags: in-testsuite-
Flags: in-litmus?
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.
I say something like:

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

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

  Username: [___________________][@]__________
  Password: [___________________] |Username1  | <-- like a menu
                                  |Username2  |
                                   ───────────
Product: Firefox → Toolkit
Attached image 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.
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).
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.
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.
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/
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).
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.
(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.
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.
> 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?).
>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.
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?
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).
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.
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.
(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.
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).
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog-
Depends on: 456904
Safari implemented #2 and #3 from comment 8.
No longer depends on: 456904
Flags: firefox-backlog- → qe-verify+
Summary: Improve discoverability of PW autofill with multiple accounts → Improve discoverability of login autocompletion (used with multiple accounts)
Whiteboard: PRD:PASS-001d → [passwords:fill-UI] PRD:PASS-001d
Blocks: 1217152
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).
Blocks: 1304224
Assignee: nobody → jkt
(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!
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
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.
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
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
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Marking as P1 because this needs to land in Firefox 52.
Priority: -- → P1
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.
Attachment #8801803 - Flags: review?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
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 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 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 :).
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.
Flags: needinfo?(MattN+bmo)
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).
Attachment #8801803 - Flags: review?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(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.
Flags: needinfo?(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.
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!
Flags: needinfo?(jkt)
Attached patch Latest WIP tests still fail (obsolete) — — Splinter Review 
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
Flags: needinfo?(jkt) → needinfo?(tanvi)
Clearing my needinfo.  Jonathan, please put up your current patch for review, even if the context menu test is still failing.
Flags: needinfo?(tanvi)
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
Flags: needinfo?(dale)
Hey MattN, just keeping you in the loop of the above.
Thanks
Flags: needinfo?(MattN+bmo)
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
Assignee: jkt → dale
Flags: needinfo?(dale)
Also worth mentioning the tests are passing locally on linux and osx fine, the failure seems to be on linux64 on try only ¯\_(ツ)_/¯
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 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)
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
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 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.
Attachment #8801803 - Flags: review?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
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
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
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
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
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
Attachment #8810361 - Flags: review?(MattN+bmo)
Comment on attachment 8810361 [details] [diff] [review]
Make login fields show autocomplete on focus

Clearing review since the tests are still failing :(
Attachment #8810361 - Flags: review?(MattN+bmo)
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
Attachment #279884 - Attachment is obsolete: true
Attachment #8801803 - Attachment is obsolete: true
Attachment #8807603 - Attachment is obsolete: true
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.
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
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
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
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
Attachment #8810361 - Attachment is obsolete: true
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
Attachment #8811737 - Flags: review?(MattN+bmo)
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.
Attachment #8811737 - Flags: review?(MattN+bmo) → review+
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
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ace6ae91b376
Make login fields show autocomplete on focus. r=mattn
https://hg.mozilla.org/mozilla-central/rev/ace6ae91b376
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1318915
Amazing! So happy this is finally gone, Thanks Dale!
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]:
Attachment #8811737 - Flags: approval-mozilla-aurora?
No longer depends on: 227632
See Also: → 227632
Comment on attachment 8811737 [details] [diff] [review]
Make login fields show autocomplete on focus

login field autocomplete fix for aurora52
Attachment #8811737 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1330111
(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.
Flags: needinfo?(dale)
This was on OSX 10.11.5
Flags: needinfo?(dale)
Depends on: 1337259
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.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: