Closed Bug 1392528 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P1)

x86
macOS
enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: selee, Assigned: selee)

References

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

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file)

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: nobody → selee
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+
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 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 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.
Keywords: checkin-needed
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Does this ticket needs manual QA verification and if yes advise on what should we check for? Thanks
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.