1329631 - Autofill not dismissed after second username selection

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Adrian Florinescu [:aflorinescu]]

Attachment: slect user and dissmiss.mp4

[OS:]
All.

[Versions tested upon:]
52.0a2 20170109004008
53.0a1 20170108030212

[Description:]
Autocomplete form not dismissed after second username selection.


Steps:
1. Open a login form (http/https)
2. Save 2 passoword manager entries.
3. Refresh the login form.
4. Select an username.
5. Click username field and re-select the username.

[Actual Result:]
After the 2nd username selection, the autofill is opened again. (see screencast.) - each new username selection will reopen the autofill.
In case of using http, selecting the username or clicking on the contextual warning has the same result: the username autofill is opened again.

[Expected Result:]
When the username is selected the username selection autofill is dismissed and the username/ password are populated.

Note: 
I believe this is a bug introduced by the recent autocomplete changes, maybe by bug 1311301?

Comment 1

[Matthew N. [:MattN]]

Hey Dale, any chance you have time to look into this? I can repro on OS X too.

Comment 2

[Dale Harvey (:daleharvey)]

Will take a look

Comment 3

[Tanvi Vyas[:tanvi]]

https://irccloud.mozilla.com/file/EcQVa7rH/fake.mov

Comment 4

[Tanvi Vyas[:tanvi]]

Dale, we need to get this into 53 and uplift to aurora/52 asap.

Comment 5

[Julien Cristau [:jcristau]]

Track password autofill issue for 52.

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Bug 1329631 - Dont showpopup if called after user click

So yup this is caused by https://bugzilla.mozilla.org/show_bug.cgi?id=1311301, nsFormFillController::MarkAsLoginManagerField is being called when the autocomplete entry is clicked which it doesnt expect to be so, so its just opening the popup as if it were being called on page load.

This was part of Jonathans original patch but it wasnt said what it was needed for, if this seems good ill get a test written up for it.

Comment 7

[Matthew N. [:MattN]]

Attachment: Bug 1329631 - Only automatically open login manager autocomplete upon first marking.

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

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8825855 [details] [diff] [review]
Bug 1329631 - Dont showpopup if called after user click

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

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +277,5 @@
>    mPwmgrInputs.Put(node, true);
>    node->AddMutationObserverUnlessExists(this);
>  
>    nsFocusManager *fm = nsFocusManager::GetFocusManager();
> +  if (fm && !aUserTriggered) {

The point of the ShowPopup code is to handle the case where the field is focused before the first MarkAsLoginManagerField occurs. Did you consider only running this code if the field is not already marked instead?

::: toolkit/components/satchel/nsIFormFillController.idl
@@ +40,5 @@
>     * Autocomplete requests will be handed off to the password manager, and will
>     * not be stored in form history.
>     *
>     * @param aInput - The HTML <input> element to tag
> +   * @param aUserTriggered - a bool if the user tiggered the filling

Nit: typo on "triggered"

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8826019 [details]
Bug 1329631 - Only automatically open login manager autocomplete upon first marking.

https://reviewboard.mozilla.org/r/104060/#review104788

::: toolkit/components/passwordmgr/test/mochitest/test_autofocus_js.html:74
(Diff revision 1)
> +  let formFillController = SpecialPowers.Cc["@mozilla.org/satchel/form-fill-controller;1"].
> +                           getService(SpecialPowers.Ci.nsIFormFillController);
> +let usernameField = iframeDoc.getElementById("form-basic-username");
> +  listenForUnexpectedPopupShown()
> +  formFillController.markAsLoginManagerField(usernameField);

I couldn't get the bug to happen using doKey or synthesizeKey for some reason (only when I manually hit enter) so this ended up being a unit test which I figured is better than nothing.

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 8825855 [details] [diff] [review]
Bug 1329631 - Dont showpopup if called after user click

Clearing since alternate patch

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 8826019 [details]
Bug 1329631 - Only automatically open login manager autocomplete upon first marking.

This is much cleaner thanks, I should have thought of that first time round.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8825855 [details] [diff] [review]
Bug 1329631 - Dont showpopup if called after user click

Thanks

Comment 13

[Carsten Book [:Tomcat]]

Dale: needs review + in mozreview so that we can use autoland for this checkin.

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 8826019 [details]
Bug 1329631 - Only automatically open login manager autocomplete upon first marking.

https://reviewboard.mozilla.org/r/104060/#review104926

Looks great, thanks

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e86a5e67f82e
Only automatically open login manager autocomplete upon first marking. r=daleharvey

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14f20d0a83a0
Fix ESLint failures in test_autofocus_js.html on a CLOSED TREE.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/e86a5e67f82e
https://hg.mozilla.org/mozilla-central/rev/14f20d0a83a0

Comment 18

[Adrian Florinescu [:aflorinescu]]

Verified as fixed on Nightly 53.0a1 20170116030326.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8826019 [details]
Bug 1329631 - Only automatically open login manager autocomplete upon first marking.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1311301
[User impact if declined]: login autocomplettion re-open when it shouldn't
[Is this code covered by automated tests?]: YEs
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Done
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple early return added
[String changes made/needed]: No

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8826019 [details]
Bug 1329631 - Only automatically open login manager autocomplete upon first marking.

don't show password autofill twice, aurora52+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/832fbf29c8a4

Comment 22

[Adrian Florinescu [:aflorinescu]]

Verified as fixed on 52.0a2 20170122004006 on Ubuntu 16.04, Windows 10 and Mac OSX 10.10.