1411990 - [Form Autofill] Expiration month field is populated with both month and year even though the field supports two caracters

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect)

Description

[Bogdan Maris, Desktop Test Engineering]

Attachment: Screencast showing the issue

[Affected versions]:
- Firefox 57 beta 11
- Latest Nightly 58.0a1 

[Affected platforms]:
- macOS 10.13
- Ubuntu 14.04 32bit
- Windows 10 64bit

[Steps to reproduce]:
1. Start Firefox
2. Save a credit card entry
3. Visit aliexpress.com and log into an account
4. Save a product in basket and proceed to checkout
5. Click use a new card
6. Double click on number or date field (month)
(for some reason Autofill will not be active when clicking inside Year field)
7. Select the saved Credit Card

[Expected result]:
- Autofill credit card fills card number/month/year fields.

[Actual result]:
- Month and Year of credit card is saved in one field only (month field)

[Regression range]:
- Using the build after bug 1398101 was fixed I see that the month field is completed with the month only, but the year is not displayed at all. (clicking inside year field does nothing).
For this behavior we have the following regression range:

Last good revision: d72d02a5f8ac69198e85fe88e0acd7cca9b8c994
First bad revision: 3c25526c75878fafc2ead1741e3957fa32258a91
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d72d02a5f8ac69198e85fe88e0acd7cca9b8c994&tochange=3c25526c75878fafc2ead1741e3957fa32258a91

3c25526c7587	Ray Lin — Bug 1392940 - Add expiration date parser to detect cc-exp family fields. r=MattN,seanlee

[Additional notes]:
- Screencast showing the issue is attached.
- I'm not sure if we need two separate bugs on this but if yes please let me know and I can log a new one (for year field being ignored).

Comment 1

[Ray Lin[:ralin]]

If it's a regression of Bug 1392940, I guess there's a high chance that the issue can be fixed by Bug 1411190. Bug 1411190 is landing now, let's see if the problem is still after landed. Thanks for reporting this :D

Comment 2

[Ray Lin[:ralin]]

Ends up the Bug 1411190 doesn't fix the problem. Comparing the result before/after Bug 1392940, I found that we never detect the cc-exp-year correctly for aliexpress.com, as you mentioned. The difference is after Bug 1392940, we added a mechanism that help to fix unreasonable fields after parsing, such as correcting standalone cc-exp-month to cc-exp for it's irrational to have only a cc-exp-month present in a regular credit card form. I believe once we fix the cc-exp-year prediction, we can make both cc-exp-month and cc-exp-year right.

Comment 3

[Ray Lin[:ralin]]

Below is the structure of cc-exp-* fields on AliExpress, just a note might help to create a reduced case:

<dt><span class="security-code">Expiry date</span><font>Security code</font></dt>
<dd>
  <input class="expire-date" maxlength="2" id="expiry-month" placeholder="MM" name="expireMonth" onpaste="return false;" data-role="card-info" type="text">
  <span class="split">/</span>
  <input id="expiry-year" class="expire-date" placeholder="YY" maxlength="2" name="expireYear" onpaste="return false;" data-role="card-info" type="text">
  <input maxlength="3" name="cvc" data-role="card-info" type="text"><span name="cvcForNewCard"></span>
  <div id="date" class="error-tips" style="top:173px; width:300px; height:30px;">Please fill in month</div>
</dd>

I will look into this and consider either doing more precise regex tests or making placeholder account for part of prediction.

Comment 4

[Mike Taylor [:miketaylr]]

(In reply to Ray Lin[:ralin] from comment #3)
> I will look into this and consider either doing more precise regex tests or
> making placeholder account for part of prediction.

any updates here?

Comment 5

[Ray Lin[:ralin]]

(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #4)
> (In reply to Ray Lin[:ralin] from comment #3)
> > I will look into this and consider either doing more precise regex tests or
> > making placeholder account for part of prediction.
> 
> any updates here?

I'm working on bug 1392947 which dealing with very similar type of question. Comparing with our target websites, aliexpress.com is relatively low priority in our queue. Plus, though the result looks like it's, I wouldn't 100% say this is a regression, since from the aspect of our heuristic, the previous good build is kind of false positive detection for aliexpress.com.

Comment 6

[Ray Lin[:ralin]]

Attachment: Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

This WIP is able to fix the aliexpress.com. Let's make the code cleaner,
and maybe add a test case.

Review commit: https://reviewboard.mozilla.org/r/198166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198166/

Comment 7

[Mike Taylor [:miketaylr]]

Let's not track this as a regression then. Thanks.

Comment 8

[Ray Lin[:ralin]]

Thanks Mike

Comment 9

[Ray Lin[:ralin]]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198166/diff/1-2/

Comment 10

[Ray Lin[:ralin]]

hmm... I should have included the fixture in comment 3 in our unit tests as well.

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198166/diff/2-3/

Comment 12

[Ray Lin[:ralin]]

A reduced test case is added in the latest patch. Thanks.

Comment 13

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

https://reviewboard.mozilla.org/r/198166/#review204734

Overall looks good.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:710
(Diff revision 3)
> +  _getElementStrings(element) {
> +    return [...{
> +      * [Symbol.iterator]() {
> -      yield element.id;
> +        yield element.id;
> -      yield element.name;
> +        yield element.name;
> -      if (!labelStrings) {
> -        labelStrings = [];
> +
> +        const labels = LabelUtils.findLabelElements(element);
> -        let labels = LabelUtils.findLabelElements(element);
>          for (let label of labels) {
> -          labelStrings.push(...LabelUtils.extractLabelStrings(label));
> +          yield *LabelUtils.extractLabelStrings(label);
>          }
> -      }
> -      yield *labelStrings;
> -    };
> +      },
> +    }];
> +  },

`getElementStrings` was designed not to expose all strings at once so that we can probably avoid unnecessary access to DOM elements via `LabelUtils`. Did you change to return an array on purpose?

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198166/diff/3-4/

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

https://reviewboard.mozilla.org/r/198166/#review204734

> `getElementStrings` was designed not to expose all strings at once so that we can probably avoid unnecessary access to DOM elements via `LabelUtils`. Did you change to return an array on purpose?

Thanks for pointing out. I removed the spread that evaluates all value at once, and return only an iterable object instead.

Comment 16

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

https://reviewboard.mozilla.org/r/198166/#review204810

Thanks.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:708
(Diff revision 4)
> +
> +  /**
> +   * Extract all the signature strings of an element.
> +   *
> +   * @param {HTMLElement} element
> +   * @returns {Array<string>} Extracted strings

The comment should be modified accordingly.

Comment 17

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

https://reviewboard.mozilla.org/r/198166/#review205200

LGTM! Thank you.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:529
(Diff revision 4)
> +        const nextDetail = fieldScanner.getFieldDetailByIndex(fieldScanner.parsingIndex);
> +        const nextElement = nextDetail.elementWeakRef.get();
> +        if (this._findMatchedFieldName(nextElement, ["cc-exp-year"])) {
> +          fieldScanner.updateFieldName(fieldScanner.parsingIndex, "cc-exp-year");
> +          fieldScanner.parsingIndex++;
> +

nit: remove this newline?

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:539
(Diff revision 4)
>      fieldScanner.parsingIndex = savedIndex;
>  
>      // If no possible regular expiration fields are detected in current parsing window
>      // fallback to "cc-exp" as there's no such case that cc-exp-month or cc-exp-year
>      // presents alone.
> +    // TODO: We should eventually remove this fallback, since we don't want to mess up

Can this TODO be done in bug 1392947?
If so, please add the bug number for this TODO.
My suggestion is to remove this once bug 1392947 can fix the birthday misdetection without any regression.

Comment 18

[Ray Lin[:ralin]]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198166/diff/4-5/

Comment 19

[Ray Lin[:ralin]]

Thanks for your review :)

> The comment should be modified accordingly.
updated.

> Can this TODO be done in bug 1392947?
> If so, please add the bug number for this TODO.
bug number added.
> My suggestion is to remove this once bug 1392947 can fix the birthday misdetection without any regression.
yeah, those lines are removed in bug 1392947 WIP patch.

Comment 20

[Ray Lin[:ralin]]

looks good on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be280f41ef33d4ea31832bac41dc6d958f62b5ce

Thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6052d8165c31
Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching. r=lchang,seanlee

Comment 22

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/6052d8165c31

Comment 23

[Ray Lin[:ralin]]

Hi Bogdan, could you help verify that the problem is fixed on aliexpress.com? Thanks

Comment 24

[Gabi Cheta [:Gabi] Desktop QA]

Verified issue as fixed on 59.0a1 20171117220410 with Win10x64, Ubuntu 14.4 and MacOS 10.12.6.

Comment 25

[Ray Lin[:ralin]]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

Approval Request Comment
[Feature/Bug causing the regression]: Form autofill heuristics 
[User impact if declined]: couldn't auto-fill credit card expiration fields on some websites
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: none
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: original heuristics cases are all covered by our unit tests
[String changes made/needed]: none

Thanks

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8926928 [details]
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.

Fix a form autofill issue and was verified. Beta58+.

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d9dd73c061c

Comment 28

[Gabi Cheta [:Gabi] Desktop QA]

Issue is verified as fixed on 58.0b5 20171120142222 with Windows 10x64, Ubuntu 14.4 and MacOS 10.12.6