1688209 - [DE][thomann.de] Credit Card Autofill is not toggled on the form fields

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P2)

Description

[Timea Cernea [:tbabos][inactive]]

Attachment: Thomman comparison to Chrome.mov

Affected Versions:*

• All latest Firefox versions using force enable (Nightly 86, Beta RC 85.0, Release 84.0.2)

Tested on:

• MacOS 10.15

Prerequisites:

• browser.search.region DE
• extensions.formautofill.supportedCountries DE
• download latest Firefox Nightly with region locale (switch extensions.formautofill.available to "on" for testing non-Nightly builds)

Steps to Reproduce:

1. Launch Firefox
2. Go to and reach the payment form for thomann.de
3. Click on each field and check id the credit card autofill dropdown is displayed

Expected Results:
Autofill dropdown should be displayed for each eligible field.

Actual Results:
Autofill dropdown is not displayed for any of the fields

Notes:

• Severity: considering S2-S3 since this is working on Chrome, depends if the fix for this would be scalable or not. Capture doorhanger is also not displayed due to this.
• Reproducible on Chrome?: Partially (Autofill is not working for chrome as well as Capture)
• Regression-range: reproducible on all latest versions, this site was never tested before, can look for regression-range if need be

Comment 1

[Tim Giles [:tgiles]]

Okay, this bug seems rather tricky upon investigation.

So the issue first appeared to be that we were not identifying the correct fields on the page, otherwise we would be able to autofill and autocomplete CC and addresses. However, this isn't the whole story. We do correctly identify that there are elements that we are supposed to use for autofill/autocomplete, but there's also duplicate invisible elements that have the same autocomplete attributes. For example, the form handler does correctly parse out that there are four sections on this page and the correct (and visible) autocomplete elements in each of these sections. If you inspect the root element here in _getFormHandler, eventually you will come across these four previously mentioned sections, so we have some of the information we need...but don't seem to be using that information.

When we move along in the code flow, we come across "activeFieldDetail()" which is called from "updateActiveInput" in our case. In "activeFieldDetail", if we have active form details, we end up iterating over these details to see if the active input (the selected field) matches one of the form detail elements. However, since the visibly selected input is not the same as the hidden input, we end up returning undefined and not activating the autocomplete dropdown.

The bug seems to be when we generate the "fieldDetails" of a particular form. Still not sure where this generation of "fieldDetails" is happening, but seems to be stored in the "_formDetails" property of FormAutofillContent. "fieldDetails" appear to come from "getSectionFieldDetails()", if I'm following the code correctly. Okay yeah, the details are generated by "_getFinalDetails" and since the invisible inputs come before the visible inputs, the visible inputs get filtered out when getting the final details.

My initial guess is to ensure we don't mark hidden inputs as viable fields for autofilling, but I'm not sure what assumptions that new behavior will break.

Comment 2

[Tim Giles [:tgiles]]

Attachment: Bug 1688209 - Prevent simple hidden fields from being eligible for autofill. r=dimi,sfoster

Comment 3

[Tim Giles [:tgiles]]

This bug seems to be a specific case of Bug 1247245. I'm not going to mark as a dupe right now, because I think this patch's fix doesn't completely solve the hidden field issue. For example there are many ways to hide an element that are not included in this patch. I believe this patch would be a stop gap for thomann only and further investigation would be needed for the general hidden element case.

Comment 4

[Tim Giles [:tgiles]]

In order to not be completely blocked by Bug 1247245, we might need to extend our current login recipes to also handle form elements on certain sites. This pattern of having duplicate fields, one hidden and one visible, is not uncommon.

Comment 5

[Tim Giles [:tgiles]]

Going to lay out my thoughts here for how we solve this one particular instance of incorrectly autofilling into a hidden field.

As previously mentioned, we could create a "recipes" based system to handle these instances of autofilling incorrectly, but the recipe would be as brittle as usual. If the website changed the ID or other identifier of an element, then the recipe would silently break. This is probably an acceptable solution.

Do we need to hardcode the recipes into mozilla-central, or should we set up a remote settings collection and handle the recipes like we do for login recipes? Hardcoding the recipes into m-c would be much quicker, but would penalize us if we needed to quickly change autofill's behavior on a particular site. Setting up recipes in remote settings would be more robust and have a quicker delivery time when needing to change autofill's behavior on a particular site, but comes with the upfront cost of getting the remote settings team involved and getting all of that related infrastructure set up.

The bigger question, and the one that is more relevant to Bug 1247245, is there a way to determine which hidden fields we should fill in and which hidden fields we should not fill in? Currently it seems our logic will be a contradiction...given some form, on one site we want to fill in hidden fields, while that same form on another site, we don't want to fill in hidden fields. Are there some clues in the form or on the site we can use to improve our current heuristics?

Comment 6

[Tim Giles [:tgiles]]

Something else I noticed: Chrome's behavior is that, if there were multiple duplicate visible fields that have the same autocomplete attribute, then it would autofill the duplicate fields. In the Thomann case, the duplicate fields are visibly hidden so it doesn't exactly apply, but helps give us some insight if we want to change our behavior for visibly hidden fields.

Comment 7

[Tim Giles [:tgiles]]

Changing this line of code from formHandler.collectFormFields() to formHandler.collectFormFields(true), which allows duplicate fields when determining form fields for autofill, causes the Thomann site to autofill as expected...at least for credit card name and number, since there's no autocomplete attributes on the expiration month and year selectors.

I don't know the far reaching implications of allowing duplicate form fields though. For example, given this new behavior, the Thomann site autofills as expected...but also autofills into the hidden fields with that have the same attribute as the visible field. This does match Chrome's behavior on the site however, so maybe allowing these duplicates would be the way to go? That way we don't need to have custom handling for each site that has some different behavior/markup.

Comment 8

[Tim Giles [:tgiles]]

I was trying to find some historical context as to why we defaulted to not including duplicate fields, but the trail, at first, ended at Bug 1339731 for me. The refactoring work was stopping me from tracing back farther, to determine what the initial implementation bug was. This function "collectFormFields" seems to have existed since 2017...err 2016 now that I'm tracing around.

Bug 1300988, as I understand it, copies the relevant code from FormAutofillContentService into FormAutofillHandler but still doesn't provide the context that I'm looking for.

Aha, think I found it. Bug 1020607, I believe, is the initial implementation of "collectFormFields" or, at least, where the behavior of throwing out duplicate identifiers is implemented. Maybe Bug 1020602 is the initial implementation of "collectFormFields", not 100% sure. Regardless, I can't find any solid point as to why we disregard fields with the same identifier. Only thing I can piece together is this comment from :MattN about eventually adding some functionality to not fill input type=hidden fields. I guess the rationale is that a W3C spec says not to include duplicate fields when trying to autofill/autocomplete.

Brings me back to my crux, will adding formHandler.collectFormFields(true) have far reaching effects? I'm still not 100% sure but considering Chrome's behavior, might be worth a shot as opposed to implementing a whole new recipes system.

Comment 9

[Tim Giles [:tgiles]]

Thanks to :dimi for finding Bug 1383058, I think this is the missing link for the implementation details about collectFormFields allowing duplicates for the first time. According to those details, looks like allowDuplicates was originally a test only parameter. So I'm not sure if the changes for this bug should use collectFormFields(true) or if some other changes should be present in FormAutofillHeuristics because "...the heuristics should be verified that some duplicated elements still can be predicted correctly" from Bug 1383058.

Comment 10

[Tim Giles [:tgiles]]

Attachment: test page with hidden cc fields

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/330618351bf0
Prevent simple hidden fields from being eligible for autofill. r=dimi

Comment 12

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/330618351bf0

Comment 13

[Timea Cernea [:tbabos][inactive]]

Hey Tim,
I just checked autofill on this site and can confirm that we do fill the Name and CC number fields. However, the Month and Year fields are still not autofilled on Firefox but it is fixed and works correctly on Chrome now.
I feel like this bug needs a follow-up to handle the case for the Month and Year dropdowns as it was not handled here.
Let me know if I should file an issue and I will also mark this one as verified after.
Thanks!

Comment 14

[Tim Giles [:tgiles]]

Hey Timea, please go ahead and file a follow-up issue so that we can keep this part verified. Thanks!

Comment 15

[Timea Cernea [:tbabos][inactive]]

Thanks Tim! Closing this issue as verified-fixed and the follow-up issues were also submitted.