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)

52 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla54
Iteration:
54.2 - Feb 20
Tracking Status
firefox51 --- unaffected
firefox52 + verified
firefox-esr52 --- fixed
firefox53 --- verified
firefox54 --- verified

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+
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.
Priority: -- → P1
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Depends on: 1333996
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.
Tracking for 52.
Iteration: --- → 54.1 - Feb 6
Flags: qe-verify?
Whiteboard: [FxPrivacy]
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)
Blocks: 1294194
No longer depends on: 1294194
Three of the commits are still in progress (hence no review requests) and I will add/update tests when I have those done.
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 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 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 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 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 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+
Attachment #8832393 - Flags: review?(dale)
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 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)
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.
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 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+
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 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+
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 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
Blocks: 1336391
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+
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
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 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+
> 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.
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).
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.
(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.
Attachment #8833235 - Flags: review?(dale) → review+
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
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?
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)
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
Depends on: 1337259
backed out for causing frequent timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=75048677&repo=mozilla-inbound
Flags: needinfo?(MattN+bmo)
Try push with small potential fixes and debug logging enabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8c9d0ecf9eed8d96c4a22858fce2171bc39cc7d
Flags: needinfo?(MattN+bmo)
(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
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 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+
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)
Needs rebasing for Aurora (or approval requests on conflicting bugs).
Flags: needinfo?(MattN+bmo)
Matt, will you be able to rebase?  Or can Johann help?
Flags: needinfo?(jhofmann)
I will. Just starting the work day and will land in the next few hours on Aurora.
Flags: needinfo?(jhofmann)
(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.
Flags: needinfo?(rfeeley)
Flags: needinfo?(MattN+bmo)
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
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.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
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)
(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)
Depends on: 1338396
Attachment #8834970 - Attachment is obsolete: true
Attachment #8834971 - Attachment is obsolete: true
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+
Flags: qe-verify? → qe-verify+
Verified fixed FX 52b6, 54.0a1 (2017-02-15) Win 10.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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)
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)
Blocks: 1325437
You need to log in before you can comment on or make changes to this bug.