1654648 - Credit Card sections not resolved correctly when there are gift card fields and then the credit card fields in a certain order

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P1)

Description

[Daniel Hertenstein]

Attachment: A caputure of the lush.com checkout page with none of the css

Tested on Nightly 80.0a1 on Windows 10

On lush.com, credit card autofill doesn't work correctly I've included a version of the page that won't load css or anything but should still show the problem. The form has a gift card section that is collapsed and then a credit card section. The gift card section still gets picked up by the FieldScanner and has fields classified as cc-name and then cc-number. The credit card fields start with cc-number and then go cc-name, cc-exp-month, and cc-exp-year. When processing of the form finishes, and we go to classify the sections (in FieldScanner._classifySections()) FieldScanner.fieldDetails starts with all of its fields in the default section with the order of those details being cc-name, cc-number, cc-number, cc-name, cc-exp-month, cc-exp-year where the first two are from the gift card and everything else is from the actual credit card information. cc-name and the first cc-number are placed into the first new section. When the second cc-number is being placed into a section, it is placed into the first section because, though cc-number is in seenTypes, the previousType === cc-number. Then cc-name from the credit card fields starts a new section because cc-name is in seenTypes and previousType !== cc-name. The expiration fields are placed in the second section. Because duplicates are not allowed, the second cc-number (from the credit card) is removed from the section that has the gift card information. Once we bubble back up to FormAutofillHandler.collectFormFields() the section with the gift card information is a valid FormAutofillCreditCardSection(), but the section with the credit card information is not valid, because its cc-number field did not make it into the right section.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:MattN, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

I debugged it and I think I understand what's going on.

The cause for this piece of code,:

  _classifySections() {
    let fieldDetails = this._sections[0].fieldDetails;
    this._sections = [];
    let seenTypes = new Set();
    let previousType;
    let sectionCount = 0;

    for (let fieldDetail of fieldDetails) {
      if (!fieldDetail.fieldName) {
        continue;
      }
      if (
        seenTypes.has(fieldDetail.fieldName) &&
        previousType != fieldDetail.fieldName
      ) {
        seenTypes.clear();
        sectionCount++;
      }
      previousType = fieldDetail.fieldName;
      seenTypes.add(fieldDetail.fieldName);
      this._pushToSection(
        DEFAULT_SECTION_NAME + "-" + sectionCount,
        fieldDetail
      );
    }
  }

is to capture multiline fields into a single section. For example, this form: https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/test/unit/heuristics/third_party/test_OfficeDepot.js

has the following structure:

[
  "given-name",
  "family-name",
  "organization",
  "address-line1",
  "address-line2",
  "postal-code",
  "address-level2",
  "address-level2", // <---
  "address-level1",
  "tel-area-code",
  "tel-local-prefix",
  "tel-local-suffix",
  "tel-extension",
  "email",
]

Notice, that address-level2 is found twice. What the author wanted to capture here is that such field is not indicative of two sections, but rather a single section with a single field that is multiline.

So, if we want to make cc-number or cc-name duplicates to cause section break, then we'd have to, I believe, annotate each field with information about whether it can be multiline or not.

To start with, I can build a small set of fields that are meant to be single-line, and if we encounter a duplicate of that type of field, we'd break the section.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1654648 - Fine tune field type duplication heuristics for section splitting. r?abr

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Adam, how does it sound to you?

Comment 5

[Adam Roach [:abr]]

It intuitively feels like the lines that can be coalesced are going to be more the exception than the rule, although I don't have data to back that up. In any case, I'm comfortable asserting that all of the cc fields (the two you have here plus cc-exp, cc-exp-month, and cc-exp-year) should be in the list of "can't appear more than once in a section."

As a tiny nit: I think I'd call these "multi-field" rather than "multi-line," so as to avoid confusion with <textarea> elements.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fe4d982c2c4
Fine tune field type duplication heuristics for section splitting. r=abr

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c2ac92f0aa1
fix whitespace linting failure r=test-only

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1fe4d982c2c4
https://hg.mozilla.org/mozilla-central/rev/3c2ac92f0aa1

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 9168627 [details]
Bug 1654648 - Fine tune field type duplication heuristics for section splitting. r?abr

Beta/Release Uplift Approval Request

• User impact if declined: Slight improvements to heuristics allowing us to capture more forms.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This only affects an A/B study in 80.
• String changes made/needed:

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9168627 [details]
Bug 1654648 - Fine tune field type duplication heuristics for section splitting. r?abr

approved for 80.0b7

Comment 11

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8ad2c073daac
https://hg.mozilla.org/releases/mozilla-beta/rev/8db980ae2ddd

Comment 12

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on affected Nightly build from 2020-08-03.
Can confirm this is now fixed on the latest Nightly 81.0a1 (2020-08-13) (64-bit) and Beta 80.0b7 (64-bit) on Windows 10 and MacOS 10.13