[Form Autofill] Misdetect birthday fields as cc-exp-year and cc-exp-month

VERIFIED FIXED in Firefox 58

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: selee, Assigned: ralin)

Tracking

(Blocks 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 unaffected, firefox58 verified, firefox59 verified)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 attachment, 1 obsolete attachment)

The heuristics should be fixed to prevent detecting the birthday fields as cc-exp-year and cc-exp-month. This happens in QVC and Sears.
Summary: Misdetect birthday fields as cc-exp-year and cc-exp-month → [Form Autofill] Misdetect birthday fields as cc-exp-year and cc-exp-month
Since this issue is not in the web site already, this bug is removed from [form autofill:MVP].
Whiteboard: [form autofill:MVP] → [form autofill]
We went over the top websites in these days, and the problem is still unsolved on QVC. I'll investigate how we can improve this, maybe either adding matching pattern for BOD or enhancing the accuracy of cc-exp-* parser.
Whiteboard: [form autofill] → [form autofill:MVP]
wups... s/BOD/DOB/
I just had a quick look to chromium field types, it doesn't define any related type to birthday fields. And it make sense as there's no fields in preferences ask you to fill out birthday information. Therefore, I guess the proper way would be enhancing the prediction for cc-exp field,  which would rather abandon uncertain field than fallback to cc-exp when we're not confident enough to tell the field set is exact cc-exp or cc-exp-* [0]. We need to take the risk in terms of breaking some false positive tests, however, for long term I think it's worthy.

[0] http://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/browser/extensions/formautofill/FormAutofillHeuristics.jsm#516-520
Assignee: nobody → ralin
Status: NEW → ASSIGNED
I would start from cribbing some useful helper functions from Chromium, or somehow make our flow easier to adapt its logic.
Component: Form Manager → Form Autofill
Even those patterns are added, I would expect some subtle differences will be spotted comparing with Chromium's implementation, our rules would be a bit looser. The reason is that if we're going to do some fundamental re-design or refactoring, I think it's preferable not to bring too much changes here, like input type filtering or some stateful variable.
Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

https://reviewboard.mozilla.org/r/199706/#review205926

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:799
(Diff revision 2)
> +   * @param {RegExp} regexp
> +   *
> +   * @returns {boolean}
> +   */
> +  _matchRegexp(element, regexp) {
> +    const elemStrigns = this._getElementStrings(element);

Looks like `_getElementStrings` will be called more than once during single `getFormInfo` process. Is it possible to cache the result of `extractLabelStrings` in `_getElementStrings` so that we won't access the same DOM elements multiple times?
Comment on attachment 8929974 [details]
Bug 1392947 - (experimental) Use memoize function to help caching previous extracted label result.

In this patch, I just want to introduce the memoize concept. I think it would be useful in some contexts, for example, remember the previous computed results in certain scope which lives along with its life cycle . Hence, we can reduce some unnecessary declarations and if-condition for caching, which looks bad somehow. Feel free give me some feedback about this.
Attachment #8929974 - Flags: feedback?(selee)
Attachment #8929974 - Flags: feedback?(lchang)
Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

https://reviewboard.mozilla.org/r/199706/#review206270

The patch looks good! However, I would like to read the patch again with the changes I addressed. Thank you.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:572
(Diff revision 3)
> +    }
> +    fieldScanner.parsingIndex = savedIndex;
> +
> +    // Set current field name to null as it failed to match any patterns.
> +    fieldScanner.updateFieldName(fieldScanner.parsingIndex, null);
> +    return false;

Since `_parseCreditCardExpirationDateFields` only cares about the interesting field types only, `return true` would be reasonable to finish the parsing for this element even there is no matching rule.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:805
(Diff revision 3)
> +   * @param {HTMLElement} element
> +   * @param {RegExp} regexp
> +   *
> +   * @returns {boolean}
> +   */
> +  _matchRegexp(element, regexp) {

`element` here can be replaced with the result of `this._getElementStrings` which is invoked by the caller e.g. `_parseCreditCardExpirationDateFields`. In addition,  `FormAutofillHeuristics.extractedLabelStrings` cache can be removed as well. The result of `_getElementStrings` would be cached in the caller of `_matchRegexp`.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:806
(Diff revision 3)
> +   * @param {RegExp} regexp
> +   *
> +   * @returns {boolean}
> +   */
> +  _matchRegexp(element, regexp) {
> +    const elemStrigns = this._getElementStrings(element);

nit: elemStrigns > elemStrings
Attachment #8928457 - Flags: review?(selee)
Attachment #8929974 - Attachment is obsolete: true
Attachment #8929974 - Flags: feedback?(selee)
Attachment #8929974 - Flags: feedback?(lchang)
Thanks for review! The issues are fixed in latest patch. I ended up persisting the cached label strings within the return object, and theoretically it will be destroyed once the caller popping out the call stack, so we are free from the difficulty managing the cached results in Heuristic singleton.
Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

https://reviewboard.mozilla.org/r/199706/#review206300

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:747
(Diff revision 4)
>     * @param {HTMLElement} element
>     * @returns {ElementStrings}
>     */
>    _getElementStrings(element) {
>      return {
> +      extractedLabelStrings: null,

nit: `extractedLabelStrings` is only used in `_getElementStrings` in the current patch.
Do you expect it will be used by the caller? If not, give it an underscore to mark its private scope.
Attachment #8928457 - Flags: review?(selee) → review+
LGTM! Thank you.
Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

https://reviewboard.mozilla.org/r/199706/#review206318
Attachment #8928457 - Flags: review?(lchang) → review+
Thanks!
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48d015dec45a
Add more credit card expiration date matching patterns to enhance prediction. r=lchang,seanlee
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/48d015dec45a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Hi Gabi,

Could you help us verify credit card expiration date can be filled correctly on QVC.com? 

STR:
1. create a credit card profile in Preferences
2. go to QVC checkout page
3. choose "Enter New Card" as payment method
4. use form autofill to fill the credit card data
5. verify if "Expiration Date" field has been filled with correct data

Thank you!
Flags: needinfo?(gasofie)
Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Approval Request Comment
[Feature/Bug causing the regression]: Form Autofill Heuristics 
[User impact if declined]: Birthday fields will be recognized as a credit card expiration fields on QVC and Sears which are our target websites.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, in comment 23
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: only enhance the accuracy of fields prediction, doesn't break any existing expected result in our unit tests.
[String changes made/needed]: none

Thanks!
Attachment #8928457 - Flags: approval-mozilla-beta?
Issue was verified as fixed on 59.0a1 20171121100129 on Ubuntu 14.4, Windows 10x64 and MacOS 10.12.6.
Flags: needinfo?(gasofie)
Comment on attachment 8928457 [details]
Bug 1392947 - Add more credit card expiration date matching patterns to enhance prediction.

Take this to support shield study in beta. Beta58+.
Attachment #8928457 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Contact: gasofie
Verified as fixed with 58.0b6 20171123161455 on Win 10 x64, Ubuntu 14.4 and MacOS 10.12.6
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.