Closed Bug 1329631 Opened 8 years ago Closed 8 years ago

Autofill not dismissed after second username selection

Categories

(Toolkit :: Password Manager, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox52 + verified
firefox53 --- verified

People

(Reporter: aflorinescu, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files, 1 obsolete file)

[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?
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"
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
Keywords: checkin-needed
Dale: needs review + in mozreview so that we can use autoland for this checkin.
Flags: needinfo?(dale)
Keywords: checkin-needed
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)
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/e86a5e67f82e Only automatically open login manager autocomplete upon first marking. r=daleharvey
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
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.

Attachment

General

Created:
Updated:
Size: