Autofill not dismissed after second username selection

VERIFIED FIXED in Firefox 52

Status

()

defect
P1
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: adrian_sv, Assigned: MattN)

Tracking

(Blocks 1 bug)

Trunk
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52+ verified, firefox53 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 attachments, 1 obsolete attachment)

[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?
Reporter

Updated

3 years ago
Whiteboard: [fxprivacy]
Hey Dale, any chance you have time to look into this? I can repro on OS X too.
Flags: needinfo?(dale)
Will take a look
Assignee: nobody → dale
Flags: needinfo?(dale)
Dale, we need to get this into 53 and uplift to aurora/52 asap.
Priority: -- → P1
Track password autofill issue for 52.
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.
Attachment #8825855 - Flags: feedback?(MattN+bmo)
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"
Assignee

Comment 9

3 years ago
mozreview-review
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 on attachment 8825855 [details] [diff] [review]
Bug 1329631 - Dont showpopup if called after user click

Clearing since alternate patch
Attachment #8825855 - Flags: feedback?(MattN+bmo)
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.
Attachment #8826019 - Flags: review?(dale) → review+
Comment on attachment 8825855 [details] [diff] [review]
Bug 1329631 - Dont showpopup if called after user click

Thanks
Attachment #8825855 - Attachment is obsolete: true
Dale: needs review + in mozreview so that we can use autoland for this checkin.
Flags: needinfo?(dale)
Keywords: checkin-needed

Comment 14

3 years ago
mozreview-review
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
Flags: needinfo?(dale)

Comment 15

3 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/e86a5e67f82e
Only automatically open login manager autocomplete upon first marking. r=daleharvey

Comment 16

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14f20d0a83a0
Fix ESLint failures in test_autofocus_js.html on a CLOSED TREE.
Assignee: dale → MattN+bmo
Status: NEW → ASSIGNED

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e86a5e67f82e
https://hg.mozilla.org/mozilla-central/rev/14f20d0a83a0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Verified as fixed on Nightly 53.0a1 20170116030326.
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
Attachment #8826019 - Flags: approval-mozilla-aurora?
Comment on attachment 8826019 [details]
Bug 1329631 - Only automatically open login manager autocomplete upon first marking.

don't show password autofill twice, aurora52+
Attachment #8826019 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on 52.0a2 20170122004006 on Ubuntu 16.04, Windows 10 and Mac OSX 10.10.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.