Ignore autocomplete="off" for filling login forms

RESOLVED FIXED in Firefox 38

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: al_9x, Assigned: ally)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

31 Branch
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox38 fixed, relnote-firefox 38+, relnote-b2g ?)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
OS: Windows XP → All
Hardware: x86 → All
See Also: → 956906
Summary: autocomplete="off" still can't be fully ignored → Ignore autocomplete="off" for filling forms
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
> 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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Summary: Ignore autocomplete="off" for filling forms → Ignore autocomplete="off" for filling login forms
Priority: -- → P1
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → ally
(Assignee)

Comment 7

4 years ago
Posted patch autocompleteoff (obsolete) — Splinter Review
Ignoring autocomplete as the default case appears to be this simple.

Recipes can simply pass in different values for ignoreAutoComplete to _fillForms
Attachment #8554840 - Flags: review?(MattN+bmo)
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.
Attachment #8554840 - Flags: review?(MattN+bmo) → feedback+
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.
Status: REOPENED → ASSIGNED
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
(Assignee)

Comment 11

4 years ago
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
(Assignee)

Updated

4 years ago
Whiteboard: [waiting on try run]
(Assignee)

Comment 12

4 years ago
looks like mochitest 5 & I need to have a heart to heart.
Whiteboard: [waiting on try run] → [wip]
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 15

4 years ago
Posted patch autocompleteoff2 (obsolete) — Splinter Review
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.
Attachment #8554840 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Attachment #8559416 - Attachment is obsolete: true
Attachment #8561484 - Flags: review?(MattN+bmo)
Related bug?

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

Comment 19

4 years ago
(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.
Attachment #8561484 - Flags: review?(MattN+bmo) → review?(dolske)
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.
Attachment #8561484 - Flags: review?(dolske) → review+
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
(Assignee)

Comment 23

4 years ago
(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.
(Assignee)

Comment 24

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/bce595d4ed1c

note self: after r+, actually _land_ patch
https://hg.mozilla.org/mozilla-central/rev/bce595d4ed1c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
relnote-firefox: --- → ?
relnote-b2g: --- → ?
Flags: needinfo?(MattN+bmo)
Keywords: dev-doc-needed
Flags: needinfo?(MattN+bmo)
Whiteboard: [wip]
Flags: needinfo?(MattN+bmo)
Is manual QA verification needed for this fix? I see it's already covered by automated testing.
Flags: qe-verify?
Flags: needinfo?(ally)
(Assignee)

Comment 28

4 years ago
I don't think so
Flags: needinfo?(ally)
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
I've added this to the upcoming Aurora/DevEd 38 release notes as:

autocomplete=off is no longer supported for username/password fields
Flags: needinfo?(MattN+bmo)
LGTM. Thanks!
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.