Closed
Bug 1330111
Opened 7 years ago
Closed 7 years ago
Username autocomplete pops up on secure forms with only one saved login (and are therefore autofilled)
Categories
(Toolkit :: Password Manager, defect, P1)
Tracking
()
People
(Reporter: MattN, Assigned: MattN)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [FxPrivacy])
User Story
When to *automatically* open autocomplete upon focus: === Focusing a username field (only applies if signon.rememberSignons=true) === U1) If the username and password fields match the last one that pwmgr filled in, don't open the autocomplete popup. U2) Otherwise, attempt to open the autocomplete popup (it won't open if there are no rows to show) === Focusing a password field (existing behaviour) === P1) If the field is empty, attempt to automatically open. P2) Otherwise, do not open. == Notes == * The above is regardless of whether the focus was caused by the user or page * The above is regardless of whether the page is secure or not… Differences between secure and insecure are caused indirectly by signon.autofillForms.http=false which means that we don't autofill insecure forms and therefore U1 will never be true when the form first appears.
Attachments
(9 files, 2 obsolete files)
59 bytes,
text/x-review-board-request
|
daleharvey
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
johannh
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
johannh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
johannh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
johannh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
MattN
:
review+
johannh
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
johannh
:
review+
|
Details |
Prerequisites: * signon.autofillForms=True (default) * Exactly one login saved for the secure form STR: 1) Visit a SECURE login page that auto-focuses the username field e.g. https://myhealthonline.sutterhealth.org/mho/ 2) Notice the form is auto-filled. Expected result: Since there was only one saved login and the form is secure, the username and password were auto-filled. The autocomplete popup doesn't open since it doesn't add any value for the user. Actual result: The form is auto-filled but autocomplete also appears on the auto-focused username field even though the only saved username and password were autofilled. I think we should implement bug 1294194 to track auto-fills and then perhaps don't show the autocomplete upon focus of the non-empty username field if it was already autofilled.
Updated•7 years ago
|
Priority: -- → P1
Updated•7 years ago
|
Assignee: nobody → MattN+bmo
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Attached are WIP patches. [Tracking Requested - why for this release]: Username autocompletion is too annoying and pops up when it doesn't add value because the password field is already filled.
Updated•7 years ago
|
Iteration: --- → 54.1 - Feb 6
Flags: qe-verify?
Whiteboard: [FxPrivacy]
Assignee | ||
Comment 7•7 years ago
|
||
Ryan, does the user story make sense? I tried to consider all of the cases you mentioned in https://docs.google.com/document/d/1ZQUSkzdlCcPjRf71P6Z_S43WuhpsayinNvBTsvEhe-Q/edit and I think these simple rules will work.
User Story: (updated)
Flags: needinfo?(rfeeley)
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Three of the commits are still in progress (hence no review requests) and I will add/update tests when I have those done.
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8832395 [details] Bug 1330111 - Replace recordAutofillResult with a method-scoped variable and .add in `finally`. https://reviewboard.mozilla.org/r/108712/#review109880 Looks reasonable :)
Attachment #8832395 -
Flags: review?(jhofmann) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8832394 [details] Bug 1330111 - Add FormLikeFactory.findRootForField API. https://reviewboard.mozilla.org/r/108710/#review109882 Makes sense, thanks!
Attachment #8832394 -
Flags: review?(jhofmann) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8830642 [details] Bug 1330111 - Convert the getFocusedInput method on nsIFormFillController to an attribute. https://reviewboard.mozilla.org/r/107382/#review109958
Attachment #8830642 -
Flags: review?(felipc) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8833235 [details] Bug 1330111 - Don't open username autocomplete upon focus if a contextmenu is opening. https://reviewboard.mozilla.org/r/109470/#review110526 ::: toolkit/components/passwordmgr/test/browser/browser_context_menu_autocomplete_interaction.js:42 (Diff revision 1) > +add_task(function* test_context_menu_password() { > + gUnexpectedIsTODO = true; I need to file a follow-up bug to fix the context menu and autocomplete interaction on password fields since it seems like the solution from bug 376668 comment 121 doesn't work now for me on OS X.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8832396 [details] Bug 1330111 - Keep track of the username and password last filled by password manager. https://reviewboard.mozilla.org/r/108714/#review110640
Attachment #8832396 -
Flags: review?(jhofmann) → review+
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8832393 [details] Bug 1330111 - Expose nsFormFillController's showPopup via nsIFormFillController. https://reviewboard.mozilla.org/r/108708/#review110642
Attachment #8832393 -
Flags: review?(felipc) → review+
Assignee | ||
Updated•7 years ago
|
Attachment #8832393 -
Flags: review?(dale)
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8830641 [details] Bug 1330111 - Always attempt to autocomplete on type=password fields upon focus. https://reviewboard.mozilla.org/r/107380/#review110728 ::: toolkit/components/satchel/nsFormFillController.cpp:1025 (Diff revision 3) > - (mPwmgrInputs.Get(mFocusedInputNode) > + && formControl->GetType() == NS_FORM_INPUT_PASSWORD) { > -#ifndef ANDROID > - || formControl->GetType() == NS_FORM_INPUT_PASSWORD > -#endif > - )) { > ShowPopup(); Aren't we breaking Fennec here? They'll never call ShowPopup now.
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8830643 [details] Bug 1330111 - Automatically open login autocomplete from username fields if the form isn't filled. https://reviewboard.mozilla.org/r/107384/#review110588 Apart from the changes from line 1532 this LGTM. :) ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:123 (Diff revision 4) > + if (!gEnabled) { > + return; > + } > + > + switch (aEvent.type) { > + // Only used for username fields Nit: add a period after the comment ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:552 (Diff revision 4) > - * onUsernameInput > - * > + * Focus event handler for username fields to decide whether to show autocomplete. > + * @param {FocusEvent} event > + */ > + _onUsernameFocus(event) { > + let focusedField = event.target; > + if (!focusedField.mozIsTextField(false) || focusedField.readOnly) { It probably doesn't matter, I just wanted to note that we could probably also pass true to this function and it would ignore password fields, right? ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1542 (Diff revision 4) > * @throws Error if aForm isn't an HTMLFormElement > */ > createFromForm(aForm) { > - let formLike = FormLikeFactory.createFromForm(aForm); > - formLike.action = LoginUtils._getActionOrigin(aForm); > + let formLike = LoginManagerContent._formLikeByRootElement.get(aForm) || {}; > + if (Object.keys(formLike).length > 0) { > + log("reusing LoginForm for", aForm, formLike); As mentioned on IRC, I'm not sure how these changes work with the rest of the patch. You wanted to revert them :)
Attachment #8830643 -
Flags: review?(jhofmann)
Assignee | ||
Comment 38•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8830641 [details] Bug 1330111 - Always attempt to autocomplete on type=password fields upon focus. https://reviewboard.mozilla.org/r/107380/#review110728 > Aren't we breaking Fennec here? They'll never call ShowPopup now. Sorry, I meant to explain that I was trying to revert Feneec to the previous behaviour. I don't think we want ShowPopup for Fennec in this case since we're not implementing the feature for Fennec yet.
Assignee | ||
Comment 39•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8830643 [details] Bug 1330111 - Automatically open login autocomplete from username fields if the form isn't filled. https://reviewboard.mozilla.org/r/107384/#review110588 > Nit: add a period after the comment Fixed. > It probably doesn't matter, I just wanted to note that we could probably also pass true to this function and it would ignore password fields, right? Good idea. At one point I was going to handle the username and password fields in this JSM so that was leftover. > As mentioned on IRC, I'm not sure how these changes work with the rest of the patch. You wanted to revert them :) Reverted. They were leftover from an older approach.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 47•7 years ago
|
||
mozreview-review |
Comment on attachment 8830643 [details] Bug 1330111 - Automatically open login autocomplete from username fields if the form isn't filled. https://reviewboard.mozilla.org/r/107384/#review110796 Thanks!
Attachment #8830643 -
Flags: review?(jhofmann) → review+
Assignee | ||
Comment 48•7 years ago
|
||
Comment on attachment 8830643 [details] Bug 1330111 - Automatically open login autocomplete from username fields if the form isn't filled. Approval Request Comment [Feature/Bug causing the regression]: Bug 376668 making the insecure field warning more visible [User impact if declined]: Autocomplete will popup on login fields more than necessary if the user has a saved password and become annoying. The autocomplete visually covers parts of the page making it annoying. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Not yet. I was hoping to land this on the weekend so it can go to Beta shortly after Aurora. [Needs manual test from QE? If yes, steps to reproduce]: Yes, it's part of the test plan. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: I think mostly the C++ changes [Why is the change risky/not risky?]: Potential for crashes or breaking Android [String changes made/needed]: None
Attachment #8830643 -
Flags: approval-mozilla-aurora?
Comment 49•7 years ago
|
||
mozreview-review |
Comment on attachment 8833236 [details] Bug 1330111 - Test changes for opening username autocomplete. https://reviewboard.mozilla.org/r/109472/#review110802 Seems good, I'd just like a couple of comments for clarity. Also, I don't remember seeing a test for "only password pre-filled" (test that only the username field is considered), is that a scenario we should care about? ::: toolkit/components/passwordmgr/test/mochitest/test_autofocus_js.html:74 (Diff revision 3) > is(iframeDoc.getElementById("form-basic-password").value, "pass", "Check first password filled"); > let popupState = yield getPopupState(); > is(popupState.open, false, "Check popup is now closed"); > }); > > +add_task(function* test_not_reopened_if_filled() { This test seems to be dependent on the test above running before it. I'm not sure if this is actually something we try to avoid (there are a lot of old tests that do this) but I'd say you should at least add a short heads-up comment. ::: toolkit/components/passwordmgr/test/mochitest/test_autofocus_js.html:78 (Diff revision 3) > > +add_task(function* test_not_reopened_if_filled() { > + listenForUnexpectedPopupShown(); > + let usernameField = iframeDoc.getElementById("form-basic-username"); > + usernameField.focus(); > + yield new Promise(resolve => setTimeout(resolve, 1000)); Even though it's pretty obvious, it would probably be good style to add a comment on what we're waiting for here. ::: toolkit/components/passwordmgr/test/mochitest/test_autofocus_js.html:80 (Diff revision 3) > + listenForUnexpectedPopupShown(); > + let usernameField = iframeDoc.getElementById("form-basic-username"); > + usernameField.focus(); > + yield new Promise(resolve => setTimeout(resolve, 1000)); > + > + // cleanup Nit: Capitalize ::: toolkit/components/passwordmgr/test/mochitest/test_autofocus_js.html:102 (Diff revision 3) > + usernameField.value = ""; > + iframeDoc.getElementById("form-basic-password").value = ""; > listenForUnexpectedPopupShown(); > formFillController.markAsLoginManagerField(usernameField); > - SimpleTest.requestFlakyTimeout("Giving a chance for the unexpected popupshown to occur"); > yield new Promise(resolve => setTimeout(resolve, 1000)); See above, this might need a comment.
Attachment #8833236 -
Flags: review?(jhofmann) → review+
Assignee | ||
Comment 50•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8833236 [details] Bug 1330111 - Test changes for opening username autocomplete. https://reviewboard.mozilla.org/r/109472/#review110802 Did you mean "pre-filled" i.e. in the @value or do you mean "auto-filled" by Fx?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 69•7 years ago
|
||
mozreview-review |
Comment on attachment 8830641 [details] Bug 1330111 - Always attempt to autocomplete on type=password fields upon focus. https://reviewboard.mozilla.org/r/107380/#review110824 ::: mobile/android/app/mobile.js:175 (Diff revision 5) > /* download helper */ > pref("browser.helperApps.deleteTempFileOnExit", false); > > /* password manager */ > pref("signon.rememberSignons", true); > +pref("signon.autofillForms.http", true); This returns Android to its pre-52 value since we didn't implement the insecure warning there. ::: toolkit/components/satchel/nsFormFillController.cpp:721 (Diff revision 5) > + if (NS_WARN_IF(!mLoginManager)) { > + return NS_ERROR_FAILURE; > + } This is to fix the crash in bug 1336391
Comment 70•7 years ago
|
||
mozreview-review |
Comment on attachment 8833235 [details] Bug 1330111 - Don't open username autocomplete upon focus if a contextmenu is opening. https://reviewboard.mozilla.org/r/109470/#review110854 Seems fine. I can't come up with a better way to solve the problem, so this will have to do :) ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:596 (Diff revision 5) > + log("maybeOpenAutocompleteAfterFocus: Opening the autocomplete popup"); > - this._formFillService.showPopup(); > + this._formFillService.showPopup(); > - } else { > + } else { > - log("_onUsernameFocus: FormFillController has a different focused input"); > + log("maybeOpenAutocompleteAfterFocus: FormFillController has a different focused input"); > - } > + } > + }.bind(this), 0); Any particular reason for using .bind(this) instead of an arrow function? ::: toolkit/components/passwordmgr/test/browser/browser_context_menu_autocomplete_interaction.js:18 (Diff revision 5) > + * Initialize logins needed for the tests and disable autofill > + * for login forms for easier testing of manual fill. > + */ > +add_task(function* test_initialize() { > + let autocompletePopup = document.getElementById("PopupAutoComplete"); > + Services.prefs.setBoolPref("signon.autofillForms", false); Nit: This should use pushPrefEnv instead I think. ::: toolkit/components/passwordmgr/test/browser/browser_context_menu_autocomplete_interaction.js:71 (Diff revision 5) > + * Synthesize mouse clicks to open the context menu popup > + * for a target login input element. > + * > + * assertCallback should return true if we should continue or else false. > + */ > +function* openContextMenu(browser, loginInput, assertCallback = null) { I don't think you're ever using the third parameter
Attachment #8833235 -
Flags: review?(jhofmann) → review+
Updated•7 years ago
|
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
Comment 71•7 years ago
|
||
Comment on attachment 8830641 [details] Bug 1330111 - Always attempt to autocomplete on type=password fields upon focus. Apologies for the delay, took a look at these patches together and they make sense + look good
Attachment #8830641 -
Flags: review?(dale) → review+
Comment 72•7 years ago
|
||
Comment on attachment 8833235 [details] Bug 1330111 - Don't open username autocomplete upon focus if a contextmenu is opening. Apologies for the delay, took a look at these patches together and they make sense + look goodApologies for the delay, took a look at these patches together and they make sense + look good
Attachment #8833235 -
Flags: review?(dale) → review+
Comment 73•7 years ago
|
||
> Did you mean "pre-filled" i.e. in the @value or do you mean "auto-filled" by Fx?
Sorry, missed that comment. I did mean pre-filled IIRC.
Assignee | ||
Comment 74•7 years ago
|
||
I did some sanity checks on Android with my Try build using http://htmlpad.org/password-autofocus/ and https://htmlpad.org/password-autofocus/ (ignore the invalid cert issue) and it seems to behave normally (I didn't compare to prior versions directly but the experience seemed like it should be).
Assignee | ||
Comment 75•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8833235 [details] Bug 1330111 - Don't open username autocomplete upon focus if a contextmenu is opening. https://reviewboard.mozilla.org/r/109470/#review110854 > Any particular reason for using .bind(this) instead of an arrow function? Yes, to get a function name in the stack > Nit: This should use pushPrefEnv instead I think. Ideally, yes, but I copied this from an existing test and I need the registerCleanupFunction anyways so I'll leave it as-is for now so I don't have to do another try push and can get this baking. > I don't think you're ever using the third parameter Correct. I copied this from another test file. I removed that arg. now.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 78•7 years ago
|
||
(In reply to comment #76) > Attachment #8833235 [details] - Flags: review+ → review?(dale@mozilla.com) Dale, in the future you should do your review in mozreview or else it doesn't know that you reviewed it which means I also can't use autoland.
Assignee | ||
Updated•7 years ago
|
Attachment #8833235 -
Flags: review?(dale) → review+
Comment 79•7 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/mozilla-inbound/rev/271035a2bc64 Always attempt to autocomplete on type=password fields upon focus. r=daleharvey https://hg.mozilla.org/integration/mozilla-inbound/rev/e208e0b51349 Convert the getFocusedInput method on nsIFormFillController to an attribute. r=felipe https://hg.mozilla.org/integration/mozilla-inbound/rev/09c59f5c572a Expose nsFormFillController's showPopup via nsIFormFillController. r=felipe https://hg.mozilla.org/integration/mozilla-inbound/rev/23180cae4a46 Add FormLikeFactory.findRootForField API. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/d94d72846952 Replace recordAutofillResult with a method-scoped variable and .add in `finally`. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e782d1c944 Keep track of the username and password last filled by password manager. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/7abc377ef809 Automatically open login autocomplete from username fields if the form isn't filled. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/9e2e44a7a159 Don't open username autocomplete upon focus if a contextmenu is opening. r=johannh,daleharvey https://hg.mozilla.org/integration/mozilla-inbound/rev/e87c86970a29 Test changes for opening username autocomplete. r=johannh
Assignee | ||
Comment 80•7 years ago
|
||
Comment on attachment 8830643 [details] Bug 1330111 - Automatically open login autocomplete from username fields if the form isn't filled. See comment 48. Note that all of the patches need approval but I didn't want to flag all 9.
Attachment #8830643 -
Flags: approval-mozilla-beta?
I had to back these out of inbound for various browser-chrome test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/955dd973d5b7ed2426e87e805a080902d7098e3d https://treeherder.mozilla.org/logviewer.html#?job_id=74967416&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=74967412&repo=mozilla-inbound And an xpcshell failure for good measure: https://treeherder.mozilla.org/logviewer.html#?job_id=74967440&repo=mozilla-inbound
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 82•7 years ago
|
||
Sorry, didn't realize that the one change would invalidate my try push. New one is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e23e46ff6a2abf8f51a79d03e1e575b50b4a120
Flags: needinfo?(MattN+bmo)
Comment 83•7 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7069ed30e0 Always attempt to autocomplete on type=password fields upon focus. r=daleharvey https://hg.mozilla.org/integration/mozilla-inbound/rev/dee4bf1753cd Convert the getFocusedInput method on nsIFormFillController to an attribute. r=felipe https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f1b7028872 Expose nsFormFillController's showPopup via nsIFormFillController. r=felipe https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a695d7f3df Add FormLikeFactory.findRootForField API. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/678b0803aa1c Replace recordAutofillResult with a method-scoped variable and .add in `finally`. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/089d4531be56 Keep track of the username and password last filled by password manager. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/2dee1d713c1d Automatically open login autocomplete from username fields if the form isn't filled. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/9c3763aa4bed Don't open username autocomplete upon focus if a contextmenu is opening. r=johannh,daleharvey https://hg.mozilla.org/integration/mozilla-inbound/rev/d71bc4a09493 Test changes for opening username autocomplete. r=johannh
Comment 84•7 years ago
|
||
backed out for causing frequent timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=75048677&repo=mozilla-inbound
Flags: needinfo?(MattN+bmo)
Comment 85•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/566eedfbb5bf Backed out changeset d71bc4a09493 https://hg.mozilla.org/integration/mozilla-inbound/rev/b3243521586b Backed out changeset 9c3763aa4bed https://hg.mozilla.org/integration/mozilla-inbound/rev/5776c72e7359 Backed out changeset 2dee1d713c1d https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a1171cca7a Backed out changeset 089d4531be56 https://hg.mozilla.org/integration/mozilla-inbound/rev/11e702637d4d Backed out changeset 678b0803aa1c https://hg.mozilla.org/integration/mozilla-inbound/rev/22ec133c5af9 Backed out changeset a0a695d7f3df https://hg.mozilla.org/integration/mozilla-inbound/rev/adb8f7d507cb Backed out changeset e0f1b7028872 https://hg.mozilla.org/integration/mozilla-inbound/rev/e29f86bd5608 Backed out changeset dee4bf1753cd https://hg.mozilla.org/integration/mozilla-inbound/rev/73401a58d048 Backed out changeset 6b7069ed30e0 for causing frequent timeouts in browser_autocomplete_insecure_warning.js
Assignee | ||
Comment 86•7 years ago
|
||
Try push with small potential fixes and debug logging enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8c9d0ecf9eed8d96c4a22858fce2171bc39cc7d
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 87•7 years ago
|
||
(In reply to Matthew N. [:MattN] (Meetings In Taipei) from comment #86) > Try push with small potential fixes and debug logging enabled: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=c8c9d0ecf9eed8d96c4a22858fce2171bc39cc7d bc6 is all green here with 22 runs so I'll try land this again
Comment 88•7 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/mozilla-inbound/rev/3cd5d86ac3aa Always attempt to autocomplete on type=password fields upon focus. r=daleharvey https://hg.mozilla.org/integration/mozilla-inbound/rev/b37b62c09b05 Convert the getFocusedInput method on nsIFormFillController to an attribute. r=felipe https://hg.mozilla.org/integration/mozilla-inbound/rev/47421c871ea2 Expose nsFormFillController's showPopup via nsIFormFillController. r=felipe https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe0bd22fa42 Add FormLikeFactory.findRootForField API. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/ad413db36e46 Replace recordAutofillResult with a method-scoped variable and .add in `finally`. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ca0a0795bd Keep track of the username and password last filled by password manager. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8fc16c5541 Automatically open login autocomplete from username fields if the form isn't filled. r=johannh https://hg.mozilla.org/integration/mozilla-inbound/rev/5f1db4a78ca1 Don't open username autocomplete upon focus if a contextmenu is opening. r=johannh,daleharvey https://hg.mozilla.org/integration/mozilla-inbound/rev/7538950ebcc9 Test changes for opening username autocomplete. r=johannh
Comment 89•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3cd5d86ac3aa https://hg.mozilla.org/mozilla-central/rev/b37b62c09b05 https://hg.mozilla.org/mozilla-central/rev/47421c871ea2 https://hg.mozilla.org/mozilla-central/rev/bfe0bd22fa42 https://hg.mozilla.org/mozilla-central/rev/ad413db36e46 https://hg.mozilla.org/mozilla-central/rev/c0ca0a0795bd https://hg.mozilla.org/mozilla-central/rev/fb8fc16c5541 https://hg.mozilla.org/mozilla-central/rev/5f1db4a78ca1 https://hg.mozilla.org/mozilla-central/rev/7538950ebcc9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 90•7 years ago
|
||
Comment on attachment 8830643 [details] Bug 1330111 - Automatically open login autocomplete from username fields if the form isn't filled. Let's get this set in aurora (all patches in this bug).
Attachment #8830643 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Hi Florin, Andrei, we would like to uplift these changes to 52.0b6 (or sooner). Would it be possible for SV to do a smoke test on this feature on latest Aurora53 build? Thanks!
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Comment 94•7 years ago
|
||
Needs rebasing for Aurora (or approval requests on conflicting bugs).
Flags: needinfo?(MattN+bmo)
Comment 95•7 years ago
|
||
Matt, will you be able to rebase? Or can Johann help?
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 96•7 years ago
|
||
I will. Just starting the work day and will land in the next few hours on Aurora.
Flags: needinfo?(jhofmann)
Assignee | ||
Comment 97•7 years ago
|
||
(In reply to George Pîrlea from comment #91) > Running the latest Nightly, the insecure login warning appears for me on > facebook.com > > I think it might be a regression introduced in this patch. Can anyone verify? I can't reproduce this. I filed bug 1338010.
Assignee | ||
Comment 98•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/83a5402b5472 https://hg.mozilla.org/releases/mozilla-aurora/rev/e22cc104ea97 https://hg.mozilla.org/releases/mozilla-aurora/rev/8898738f895e https://hg.mozilla.org/releases/mozilla-aurora/rev/aa053c7497f4 https://hg.mozilla.org/releases/mozilla-aurora/rev/f64a17520775 https://hg.mozilla.org/releases/mozilla-aurora/rev/c75ab99e17b5 https://hg.mozilla.org/releases/mozilla-aurora/rev/b56cf0210d77 https://hg.mozilla.org/releases/mozilla-aurora/rev/57d720b33354 https://hg.mozilla.org/releases/mozilla-aurora/rev/ee5b1dfbf401
Flags: in-testsuite+
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(rfeeley)
Flags: needinfo?(MattN+bmo)
Comment hidden (obsolete) |
Assignee | ||
Comment 100•7 years ago
|
||
Rebased beta try push v2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34544281b93e19af235589158e9689bb5c04413b Please land the following commits from the try push for "Bug 1334026" and "Bug 1330111" once approved if Try is green: https://hg.mozilla.org/try/shortlog?rev=7ebc860b2307%3A%3A43884f68cd61
Comment 101•7 years ago
|
||
I verified this issue across platforms using latest Developer Edition 53.0a2. I also logged two new issues discovered during testing, bug 1338187 and bug 1338201.
Comment 102•7 years ago
|
||
Also forgot to mention one additional possible issue here. STR: 1. Enter a secure website and remember credentials (I used vk.com or twitter.com) 2. Open private window and visit the same website as before Actual: The autocomplete pops up right from the start, but the credentials are not populated in the input fields. Not sure if this is intended or not. Matt?
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 103•7 years ago
|
||
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #101) > I verified this issue across platforms using latest Developer Edition > 53.0a2. Thanks Bogdan! > I also logged two new issues discovered during testing, bug 1338187 > and bug 1338201. Ok, neither of those seem like regressions from the insecure password warning work in 52+. (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #102) > STR: > 1. Enter a secure website and remember credentials (I used vk.com or > twitter.com) > 2. Open private window and visit the same website as before > > Actual: The autocomplete pops up right from the start, but the credentials > are not populated in the input fields. Not sure if this is intended or not. > Matt? That is intended. The cases where we don't autofill: 51 and earlier: private windows 52+: private windows AND insecure forms (the warning should be the first row when that happens)
Flags: needinfo?(MattN+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8834970 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8834971 -
Attachment is obsolete: true
Comment 104•7 years ago
|
||
Comment on attachment 8830643 [details] Bug 1330111 - Automatically open login autocomplete from username fields if the form isn't filled. let's get this set in 52.0b6. Comment 100 has the commits to land, combining bugs 1334026 and 1330111.
Attachment #8830643 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 105•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/235bab6aab65 https://hg.mozilla.org/releases/mozilla-beta/rev/6f2c68f6ba5e https://hg.mozilla.org/releases/mozilla-beta/rev/182607df53cd https://hg.mozilla.org/releases/mozilla-beta/rev/2194e38b422d https://hg.mozilla.org/releases/mozilla-beta/rev/9999cfe192db https://hg.mozilla.org/releases/mozilla-beta/rev/38747b063e23 https://hg.mozilla.org/releases/mozilla-beta/rev/36273b6ec5db https://hg.mozilla.org/releases/mozilla-beta/rev/6af8a64f8759 https://hg.mozilla.org/releases/mozilla-beta/rev/f984c74df66c
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
Comment 106•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/235bab6aab65 https://hg.mozilla.org/releases/mozilla-esr52/rev/6f2c68f6ba5e https://hg.mozilla.org/releases/mozilla-esr52/rev/182607df53cd https://hg.mozilla.org/releases/mozilla-esr52/rev/2194e38b422d https://hg.mozilla.org/releases/mozilla-esr52/rev/9999cfe192db https://hg.mozilla.org/releases/mozilla-esr52/rev/38747b063e23 https://hg.mozilla.org/releases/mozilla-esr52/rev/36273b6ec5db https://hg.mozilla.org/releases/mozilla-esr52/rev/6af8a64f8759 https://hg.mozilla.org/releases/mozilla-esr52/rev/f984c74df66c
status-firefox-esr52:
--- → fixed
Comment 107•7 years ago
|
||
Verified fixed FX 52b6, 54.0a1 (2017-02-15) Win 10.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment 108•7 years ago
|
||
Username autocomplete pops up on insecure forms (http://www.webs.com/s/login/relogin) with 2 saved logins. Is this expected?
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 109•7 years ago
|
||
Right, that's correct and has nothing to do with this bug since this bug is about SECURE with ONE login. I would hope that Adrian and the test plan would have explained the case you're talking about: Whenever the insecure warning exists the pop up should auto-open so the user actually sees the warning before typing.
Flags: needinfo?(MattN+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•