LoginManagerContent.onFormPasword calls _fillForm passing a hard-coded false for ignoreAutocomplete, _fillForm then does not autofill ____________________________________ if (!ignoreAutocomplete && (this._isAutocompleteDisabled(form) || this._isAutocompleteDisabled(usernameField) || this._isAutocompleteDisabled(passwordField))) { isFormDisabled = true; log("form not filled, has autocomplete=off"); } ____________________ Why is this still not user overridable? If a form has just a password, then a user triggered autofill can't work as it's tied to the username field. And even if there is a username field, the user should still be in control of password manager behavior, not the site.

See bug 956906 comment 29 and the discussion that follows.

I think there are some good reasons to let sites avoid us auto-filling logins. We should address these use cases by fixing bug 348941 and bug 433238.

> I think there are some good reasons to let sites avoid us auto-filling logins. What are those reasons? Consensus among Team Passwords 2015 is that we that should start disrespecting autocomplete=off when filling login forms, which is consistent with the broader trend among others browsers.

I honestly don't recall the reasons, but one use case I do recall is sites who use password fields for non-password data that they want masked (http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Oct/thread.html#msg34). Not a particularly convincing one, since it is rare and there are alternative ways to do this. Distinguishing "valid use cases" from what we consider to be "invalid use cases" for autofill=false is probably impossible to do consistently, which probably suggests not obeying it (or at least only using it as part of a heuristic), and ensuring that our filling isn't "too aggressive" via other means.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #5) > I honestly don't recall the reasons, but one use case I do recall is sites > who use password fields for non-password data that they want masked > (http://lists.w3.org/Archives/Public/public-whatwg-archive/2014Oct/thread. > html#msg34). Not a particularly convincing one, since it is rare and there > are alternative ways to do this. This is something we could support with a recipe -- basically a site-specific setting to honor autocomplete=off. Or, more generally, with other improved detect/fill mechanisms we can avoid treating it as a login in the first place. That would allow ignoring @autocomplete as a default behavior, while still providing a way to make the password manager operate as expected on these rare sites.

Ignoring autocomplete as the default case appears to be this simple. Recipes can simply pass in different values for ignoreAutoComplete to _fillForms

Comment on attachment 8554840 [details] [diff] [review] autocompleteoff Review of attachment 8554840 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty sure you'll need to deal with tests in the tree for this feature which will fail with this change.

Comment on attachment 8554840 [details] [diff] [review] autocompleteoff Review of attachment 8554840 [details] [diff] [review]: ----------------------------------------------------------------- We may also want to make this a pref for now (like we did with ignoring @autocomplete=off for saving) so that we can easily revert it if we run into issues in the wild. That may also make it easier to fix the tests as the specific tests can just flip the pref for now.

FYI in the improved passwords UI, the user should have the opportunity to click the key icon in the address bar and manually bring up all logins for the domain. https://www.lucidchart.com/documents/view/87ab1cc8-e708-49d3-8b91-6e2e6da346fb/2

well, let's see what we've got in the way of broken tests, since I'll expire of old age before the local runs finish: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68419710deec

looks like mochitest 5 & I need to have a heart to heart.

(In reply to Ryan Feeley from comment #10) > FYI in the improved passwords UI, the user should have the opportunity to > click the key icon in the address bar and manually bring up all logins for > the domain. > https://www.lucidchart.com/documents/view/87ab1cc8-e708-49d3-8b91- > 6e2e6da346fb/2 Do we have a bug on file for that yet? I don't recall seeing it come up in triage.

> Do we have a bug on file for that yet? I don't recall seeing it come up in triage. :rfeeley should start filing UX bugs this week now that his design is fairly stable.

All password tests pass locally. Waiting on try to see if linux mochitest-5 agrees. https://treeherder.mozilla.org/#/jobs?repo=try&revision=13d0cd7364ff feel free to drop feedback in the interim.

green mochitest-5 https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d258839fb9d

Related bug? Provide an indication to the user when auto-complete is suggesting saved logins https://bugzilla.mozilla.org/show_bug.cgi?id=1129629

(In reply to Ryan Feeley from comment #18) > Related bug? > > Provide an indication to the user when auto-complete is suggesting saved > logins > https://bugzilla.mozilla.org/show_bug.cgi?id=1129629 In that they both concern passwords, but the work is not related.

Comment on attachment 8561484 [details] [diff] [review] autocompleteoff3 Review of attachment 8561484 [details] [diff] [review]: ----------------------------------------------------------------- Passing this over to dolske so I can wrap up some patches of my own.

Comment on attachment 8561484 [details] [diff] [review] autocompleteoff3 Review of attachment 8561484 [details] [diff] [review]: ----------------------------------------------------------------- Preemptive r+, but I think the two subtest_notifications*.html changes shouldn't be needed. Can you verify they're not, or say why they are? ::: toolkit/components/passwordmgr/test/subtst_notifications_2.html @@ +7,2 @@ > <form id="form" action="formsubmit.sjs"> > + <input id="user" name="user"> Is this actually needed? As I read test_notifications.html / case 9, which uses this subtest_notification_2.html, it's checking to see that we offer to save the password even when autocomplete=off is present... And that behavior isn't changing with this patch. ::: toolkit/components/passwordmgr/test/subtst_notifications_3.html @@ +7,3 @@ > <form id="form" action="formsubmit.sjs"> > <input id="user" name="user"> > + <input id="pass" name="pass" type="password"> Ditto.

Comment on attachment 8561484 [details] [diff] [review] autocompleteoff3 Review of attachment 8561484 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/passwordmgr/LoginManagerContent.jsm @@ +569,5 @@ > * - foundLogins is an array of nsILoginInfo for optimization > */ > + _fillForm : function (form, autofillForm, clobberPassword, > + userTriggered, foundLogins) { > + let ignoreAutocomplete = true; Drive-by: Instead of creating a local variable here, shouldn't we just get rid of this whole block of logic here? http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#653

(In reply to :Margaret Leibovic from comment #22) > Comment on attachment 8561484 [details] [diff] [review] > autocompleteoff3 > > Review of attachment 8561484 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/passwordmgr/LoginManagerContent.jsm > @@ +569,5 @@ > > * - foundLogins is an array of nsILoginInfo for optimization > > */ > > + _fillForm : function (form, autofillForm, clobberPassword, > > + userTriggered, foundLogins) { > > + let ignoreAutocomplete = true; > > Drive-by: Instead of creating a local variable here, shouldn't we just get > rid of this whole block of logic here? > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/ > LoginManagerContent.jsm#653 No, because we'll want to be able to override it later for recipes if we find particular sites that this breaks on, so we're not ready to rip it out yet.

https://hg.mozilla.org/integration/fx-team/rev/bce595d4ed1c note self: after r+, actually _land_ patch

Release Note Request [Why is this notable]: Firefox already ignored autocomplete=off for prompting to remember logins since bug 956906 but now we also ignore "off" for the purpose of auto-filling login forms with saved credentials. The behaviour of non-login forms doesn't change with this bug. This change puts the user back in control of the login experience and aligns with the trend in other browsers. [Suggested wording]: autocomplete=off is no longer supported for username/password fields [Links (documentation, blog post, etc)]: None yet This change should probably get an intent-to-ship email.

Is manual QA verification needed for this fix? I see it's already covered by automated testing.

I don't think so

I've added this to the upcoming Aurora/DevEd 38 release notes as: autocomplete=off is no longer supported for username/password fields

MattN, doc updates: https://developer.mozilla.org/en-US/docs/Web/Security/Securing_your_site/Turning_off_form_autocompletion https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#attr-autocomplete - added a paragraph pointing to the previous doc https://developer.mozilla.org/en-US/docs/Web/HTML/Element/form#attr-autocomplete - added a similar paragraph Please let me know if this looks good.

LGTM. Thanks!