1645922 - Fields are not saved after choosing to save the credit card from the doorhanger

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P1)

Description

[chsiang]

Steps to reproduce:

1. Visit the product page on Etsy
   https://www.etsy.com/listing/799631062/soft-organic-cotton-crew-socks-short?ref=shop_home_active_20
2. Put 1 pair of socks to the shopping cart
3. Proceed to Guest Checkout
4. Fill in the shipping information
5. Proceed to review order

Expected results:

1. Save card doorhanger dropped
2. Choose save and have a complete card profile in autofill

Actual results:

1. Save card doorhanger dropped
2. Choose save but the "name on card" and "card type" were not saved

Comment 1

[Matthew N. [:MattN]]

I mentioned this in bug 1642060 comment 1 but I don't know if it made its way into its own bug… it would be good to update the regexes from Chromium as a starting point before digging deeper as that could fix it. I would do that as a first commit then figure out if there is still a problem.

Comment 2

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

Attachment: Bug 1645922 - Update Regexps used by heuristics to latest master.

Comment 3

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

Updated heuristics to the latest source.

It did not fix the bug, but I guess updating them is worth it anyway, so I'll keep this patch at the base of the patchset.

Comment 4

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

In the next patch, I'll extend FormAutofillHeuristics.jsm to handle more logic.

Comment 5

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

I decided it is not the best use of my time now to extend FormAutofillHeuristics unless it directly relates to this bug now.

So instead I dove into the issues at hand, and if either of them requires heuristics logic, then I'll do that.

The two issues we have are:

1. Name field is not recognized
2. Card Type field is not recognized

Matt indicated that the type field will require heuristics update, so it's still likely I'll get there.

Comment 6

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

I was able to find the root of the (1) issue.

We identify the name field as name type, rather than cc-name, and since name type is part of address, and not credit card, we skip it when serializing credit card entry.

My naive interpretation is that we should treat the name field as part of Credit Card entry if there is not separate cc-name field.

Matt, does it sound reasonable or was there a reason you didn't go for such solution?

Comment 7

[Matthew N. [:MattN]]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #5)

2. Card Type field is not recognized

Matt indicated that the type field will require heuristics update, so it's still likely I'll get there.

Right, card type heuristics are covered by bug 1642060. The same heuristics are used for filling and saving.

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #6)

We identify the name field as name type, rather than cc-name, and since name type is part of address, and not credit card, we skip it when serializing credit card entry.

My naive interpretation is that we should treat the name field as part of Credit Card entry if there is not separate cc-name field.

Matt, does it sound reasonable or was there a reason you didn't go for such solution?

That doesn't seem to account for having address and cc fields in the same form/document so doesn't sound good as-is. I know we have sectioning code that tries to separate parts of the form but I wasn't involved it in much so I'm not sure how well it works or how it works. If we have already determined that the name field isn't in an address section (not sure we do it this way or if sectioning is only to separate billing vs. shipping address) then it could work. Basically I think you need to flesh out the idea more to handle address fields nearby.

Comment 8

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

Ok, so the problem is that if you have a single field name, it creates a separate section for it with type address:

[
  {
    "type": "address",
    "fieldDetails": [
      {
        "section": "",
        "addressType": "",
        "contactType": "",
        "fieldName": "name",
        "elementWeakRef": {},
        "_reason": "autocomplete"
      }
    ]
  },
  {
    "type": "creditCard",
    "fieldDetails": [
      {
        "section": "",
        "addressType": "",
        "contactType": "",
        "fieldName": "cc-number",
        "elementWeakRef": {},
        "_reason": "autocomplete"
      },
      {
        "section": "",
        "addressType": "",
        "contactType": "",
        "fieldName": "cc-exp-month",
        "elementWeakRef": {},
        "_reason": "autocomplete"
      },
      {
        "section": "",
        "addressType": "",
        "contactType": "",
        "fieldName": "cc-exp-year",
        "elementWeakRef": {},
        "_reason": "autocomplete"
      }
    ]
  }
]

I'm digging into how to resolve that.

Comment 9

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

[edited] Previously I assumed that our heuristics struggle with field of id name. Since then I verified that the name of the field on Etsy is cc-name, but it gets classified as address#name. Debugging why.

So, the issue seems to be that the way we approach sections is that we just take a field, identify its section and then move on.

https://searchfox.org/mozilla-central/rev/5e6c7717255ca9638b2856c2b2058919aec1d21d/browser/extensions/formautofill/FormAutofillHeuristics.jsm#195-216

So, on Etsy, we take the cc-name field and classify it as name, and part of address.

Then go to cc-number and classify it as creditCard and then go on with other fields. All other fields end up in carditCard while name stays in address.

I see two ways to resolve it:

1. Find a way to not classify cc-name as name.
2. Manually one-off name as a field that if it remains the only address section field, and the form has cc section with no cc-name, gets moved into cc section as cc-name.

Chromium does handle this differently - https://source.chromium.org/chromium/chromium/src/+/master:components/autofill/core/browser/form_parsing/form_field.cc;l=100-103;bpv=0;bpt=1?originalUrl=https:%2F%2Fcs.chromium.org%2F

What they do is that they scan in order:

• Address
• Credit Card
• Name

So, the field name gets classified by the cc-name matcher first in Credit Card loop, then since cc-number and cc-exp are also present, the whole section is returned (otherwise, they clear the markers and let the fields be classified by the Name and later added to Address).

Imho, that's the right approach that will pay off long term, but it's a non-trivial rewrite of the heuristics, and it diverges from the concept of sections. It doesn't necessarily erase them, but it does shuffle algorithms around because instead of going a O(n) and attaching sections, we'd be going O(3n) and first classifying all fields, and only then checking continuous sections.
At that point, I'm not sure if sections will still bring any value.

I believe there's little value in debugging the Card Type saving part of this bug, unless we decide to fix the Name classifying bug first either via dirty hack, or algorithmic rewrite.

Comment 10

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

I found what is happening.

Minimized testcase that reproduces it is:

  <form class="alignedLabels">
    <label for="cc-name">Name on card <input type="text" id="cc-name"></label>

    <label>Card Number: <input autocomplete="cc-number"></label>
    <label>Expiration month: <input autocomplete="cc-exp-month"></label>
    <label>Expiration year: <input autocomplete="cc-exp-year"></label>
    <label>CSC: <input autocomplete="cc-csc"></label>

    <p>
      <input type="submit" value="Submit">
      <button type="reset">Reset</button>
    </p>
  </form>

The core of the issue happens in [0].

This method collects all tokens that may be relevant for the field, without prioritizing them. It means that our heuristics capture cc-name and Name on card and loop over regexps to find if those terms match to any of them.

Since regexp name is before regexp cc-name, we first test for all fields matching name regexp [1] before we test for cc-name regexp [2] and match against the id.

If I'm not mistaken that heuristic is inherently flawed and the order should be reverted in [0] and we should first check all regexps for id, name before we get to labels.

There may be a shortcut tho, that should handle the name vs cc-name in particular - since cc-name is more specific, we could reorder the regexps and first attempt to match against cc-name, before we try for the more generic name.

That would emulate the logic I described in comment 9 until we get a chance to clean up the heuristics. I'll try that next.

[0] https://searchfox.org/mozilla-central/rev/5e6c7717255ca9638b2856c2b2058919aec1d21d/browser/extensions/formautofill/FormAutofillHeuristics.jsm#1063
[1] https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/content/heuristicsRegexp.js#158
[2] https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/content/heuristicsRegexp.js#201

Comment 11

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

Attachment: Bug 1645922 - Reshuffle heuristic regexps to prioritize cc-name over generic name.

Comment 12

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

With bug 1647043 patch and the patches in this bug, I was able to successfully fix the bug described in comment 0!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fd3a3846d59
Update Regexps used by heuristics to latest master. r=abr
https://hg.mozilla.org/integration/autoland/rev/70a0f7fd1803
Reshuffle heuristic regexps to prioritize cc-name over generic name. r=abr

Comment 14

[Cristian Brindusan [:cbrindusan]]

Backed out 2 changesets (bug 1645922) for xpcshell failures at test_getInfo.js.
https://hg.mozilla.org/integration/autoland/rev/6033b2c854f37e7a523748d785fc110a97d394c6

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=70a0f7fd1803d814072439bedd70160da691cc7a&selectedTaskRun=MHRwPu1_TomvT1s7CxU1Tw.0

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307140807&repo=autoland&lineNumber=2983

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/806575a46680
Update Regexps used by heuristics to latest master. r=abr
https://hg.mozilla.org/integration/autoland/rev/e65c8bb7ce6c
Reshuffle heuristic regexps to prioritize cc-name over generic name. r=abr

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/806575a46680
https://hg.mozilla.org/mozilla-central/rev/e65c8bb7ce6c

Comment 17

[Gabi Cheta [:Gabi] Desktop QA]

Verified as fixed with Firefox 79.0b4 and Firefox 80.0a1 on Windows 10x64, Ubuntu 16.4 and macOS 10.13.