[Form Autofill] Ignore autocomplete=off attr for the credit card fields

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Form Manager
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: seanlee, Assigned: seanlee)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla57
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [form autofill:MVP])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
We already know that there are some web sites with autocomplete=off attr for the credit card fields, and Chromium won't respect autocomplete=off.
This bug is for the discussion that how we handle this case of these tags:
<form autocomplete="off"></form>
<input autocomplete="off"/>
<select autocomplete="off"></select>
(Assignee)

Updated

3 months ago
Assignee: nobody → selee
Comment hidden (mozreview-request)
Comment on attachment 8905433 [details]
Bug 1392528 - Ignore autocomplete="off" attribute for Credit Card related fields.

https://reviewboard.mozilla.org/r/177248/#review182624

Thanks

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:470
(Diff revision 1)
>          addressType: "",
>          contactType: "",
>        };
>      }
>  
> -    let regexps = Object.keys(this.RULES);
> +    const MUST_CHECKING_FIELDNAMES = [

Maybe FIELDNAMES_IGNORING_AUTOCOMPLETE_OFF is more clear?

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:517
(Diff revision 1)
>            };
>          }
>        }
>      }
>  
>      return null;

IIUC we will end up returning null instead of an "off" fieldName after this change in some cases? I wonder if that that will cause problems. It may be better to not change that?

::: browser/extensions/formautofill/FormAutofillUtils.jsm
(Diff revision 1)
> -    if (element.autocomplete == "off") {
> -      return false;
> -    }

Please do a talos comparison before landing since we will do more work on some pages that were using autocomplete=off
Attachment #8905433 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 3

2 months ago
mozreview-review-reply
Comment on attachment 8905433 [details]
Bug 1392528 - Ignore autocomplete="off" attribute for Credit Card related fields.

https://reviewboard.mozilla.org/r/177248/#review182624

> Maybe FIELDNAMES_IGNORING_AUTOCOMPLETE_OFF is more clear?

Thanks! That's way better.

> IIUC we will end up returning null instead of an "off" fieldName after this change in some cases? I wonder if that that will cause problems. It may be better to not change that?

[1] is the only caller, and I do not see any case that it needs to deal with fieldName="off" element. IMHO, `pushDetail` can handle `null` even it's from an `autocomplete="off"` element.

May I change the code once we want to handle the `off` case in these parsers?

[1] http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/extensions/formautofill/FormAutofillHeuristics.jsm#116

> Please do a talos comparison before landing since we will do more work on some pages that were using autocomplete=off

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0b71c292a474e955ccac6dcd8b67ea24009330af&newProject=try&newRevision=657bde047e7b218f3234327d2243aa34705f514f&framework=1&filter=tps&showOnlyImportant=0

Comment 4

2 months ago
mozreview-review-reply
Comment on attachment 8905433 [details]
Bug 1392528 - Ignore autocomplete="off" attribute for Credit Card related fields.

https://reviewboard.mozilla.org/r/177248/#review182624

> [1] is the only caller, and I do not see any case that it needs to deal with fieldName="off" element. IMHO, `pushDetail` can handle `null` even it's from an `autocomplete="off"` element.
> 
> May I change the code once we want to handle the `off` case in these parsers?
> 
> [1] http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/browser/extensions/formautofill/FormAutofillHeuristics.jsm#116

Sure, now that I know you've considered the potential issue.
Comment hidden (obsolete)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 months ago
mozreview-review-reply
Comment on attachment 8905433 [details]
Bug 1392528 - Ignore autocomplete="off" attribute for Credit Card related fields.

https://reviewboard.mozilla.org/r/177248/#review182624

> https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0b71c292a474e955ccac6dcd8b67ea24009330af&newProject=try&newRevision=657bde047e7b218f3234327d2243aa34705f514f&framework=1&filter=tps&showOnlyImportant=0

There is only one item regressed "tsvg_static opt e10s", and I do not think it's related to this change much.
So I would like to land this patch. Thanks.
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 8

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1c952f67044f
Ignore autocomplete="off" attribute for Credit Card related fields. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1c952f67044f
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 10

2 months ago
Does this ticket needs manual QA verification and if yes advise on what should we check for? Thanks
(Assignee)

Comment 11

2 months ago
Hi Gabi,

There are some Credit Card related fields with autocomplete="off" in the targeting websites, e.g. OfficeDepot, Amazon, Macys, QVC, and Sears, etc.
Unlike the address autofill, the credit card fields with autocomplete="off" are still able to be filled by autofill feature.
Please help to check it while verifying the targeting websites.

BTW, please apply the patch in bug 1398101 to verify the autocomplete="off" fields. It also affects the result. Thank you.
You need to log in before you can comment on or make changes to this bug.