1400760 - [Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Luke Chang [:lchang] (inactive)]

We currently define a valid credit-card form as the one with "cc-number" field no matter other fields are present or not. However, there's a chance that we might misdetect other fields to be "cc-number" (e.g. bug 1392950).

An additional condition may be able to alleviate this situation. After discussing with UX, we agreed to only popup credit card dropdown in a form that contains both "cc-number" and "cc-exp" (or its components, "cc-exp-month" and "cc-exp-year").

Since this rule is for reducing misdetection, we won't apply it to a form whose "cc-number" is defined explicitly by the @autocomplete attribute.

Comment 1

[Ray Lin[:ralin]]

I'd like to add bug 1392940 as dep, since at least cc-exp family should be correctly identified to say we can mitigate this case with a confident basis.

Comment 2

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1400760 - [Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete.

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

Comment 3

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

Comment on attachment 8916531 [details]
Bug 1400760 - [Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete.

https://reviewboard.mozilla.org/r/187670/#review193944

The patch looks good! I am just curious if we should apply the same behavior to an address form.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:194
(Diff revision 1)
> +          hasExpiryDate = true;
> +          break;
> +      }
> +    }
> +
> +    return hasCCNumber && (ccNumberReason == "autocomplete" || hasExpiryDate);

If we care about cc-number with autocomplete or not, should we use the same idea to an address form?

::: browser/extensions/formautofill/test/unit/test_collectFormFields.js:215
(Diff revision 1)
> +    ],
> +  },
> +  {
> +    description: "A valid credit card form with non-autocomplete-attr cc-number and cc-exp-month/cc-exp-year.",
> +    document: `<form>
> +               <input id="cc-number" name="card-number">

According this line:
`return hasCCNumber && (ccNumberReason == "autocomplete" || hasExpiryDate);`

I think this case will have one item in `validFieldDetails`
<form>
<input id="cc-number" autocomplete="cc-number">
</form>

Do we have the test already?

Comment 4

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

Comment on attachment 8916531 [details]
Bug 1400760 - [Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete.

https://reviewboard.mozilla.org/r/187670/#review193944

> According this line:
> `return hasCCNumber && (ccNumberReason == "autocomplete" || hasExpiryDate);`
> 
> I think this case will have one item in `validFieldDetails`
> <form>
> <input id="cc-number" autocomplete="cc-number">
> </form>
> 
> Do we have the test already?

In addition, this case will have an empty `validFieldDetails`, right?
<form>
<input id="cc-number">
</form>

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8916531 [details]
Bug 1400760 - [Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete.

https://reviewboard.mozilla.org/r/187670/#review193944

> If we care about cc-number with autocomplete or not, should we use the same idea to an address form?

I can't think of any case of an address form that needs this. Do you have any idea?

> In addition, this case will have an empty `validFieldDetails`, right?
> <form>
> <input id="cc-number">
> </form>

Added the test. Thanks.

And right. The second case gets an empty result.

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8916531 [details]
Bug 1400760 - [Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete.

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

Comment 7

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

Comment on attachment 8916531 [details]
Bug 1400760 - [Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete.

https://reviewboard.mozilla.org/r/187670/#review193944

> I can't think of any case of an address form that needs this. Do you have any idea?

Yeah, we can discuss this later unitl we have the requiremenet to the similiar strategy for the address field based on autocomplete attr. Thanks.

Comment 8

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

Comment on attachment 8916531 [details]
Bug 1400760 - [Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete.

https://reviewboard.mozilla.org/r/187670/#review195794

LGTM! Thanks.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c05495fc2b51
[Form Autofill] Don't popup credit card dropdown if only cc-number field is identified and without @autocomplete. r=seanlee

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c05495fc2b51