Closed Bug 1411990 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Autofill, defect)

Product:
Toolkit
Component:
Form Autofill
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- verified
firefox59 --- verified

People

(Reporter: bmaris, Assigned: ralin)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Whiteboard: [form autofill])

Attachments

(2 files)

Screencast showing the issue
860.09 KB, video/quicktime
Details
Bug 1411990 - Add consecutive cc-exp-* regex check in form autofill heuristics to enhance expiration date pattern matching.
59 bytes, text/x-review-board-request
lchang
: review+
selee
: review+
gchang
: approval-mozilla-beta+
Details 
[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).
Flags: needinfo?(ralin)
Keywords: regression
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
Flags: needinfo?(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.
Depends on: 1410098
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.
(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?
Flags: needinfo?(ralin)
Component: Form Manager → Form Autofill
(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.
Flags: needinfo?(ralin)
Let's not track this as a regression then. Thanks.
Keywords: regression
Thanks Mike
Assignee: nobody → ralin
Status: NEW → ASSIGNED
hmm... I should have included the fixture in comment 3 in our unit tests as well.
A reduced test case is added in the latest patch. Thanks.
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 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 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.
Attachment #8926928 - Flags: review?(lchang) → review+
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.
Attachment #8926928 - Flags: review?(selee) → review+
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.
Pushed by lchang@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6052d8165c31
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Hi Bogdan, could you help verify that the problem is fixed on aliexpress.com? Thanks
Flags: needinfo?(bogdan.maris)
Verified issue as fixed on 59.0a1 20171117220410 with Win10x64, Ubuntu 14.4 and MacOS 10.12.6.
Flags: needinfo?(bogdan.maris)
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
Attachment #8926928 - Flags: approval-mozilla-beta?
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+.
Attachment #8926928 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Issue is verified as fixed on 58.0b5 20171120142222 with Windows 10x64, Ubuntu 14.4 and MacOS 10.12.6
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: