1406771 - Use placeholder text as an indicator for form autofill format

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Jonathan Kingston [:jkt] he/him]

When using a shopify website I was presented with the following form:


<form>
      
        <label for="number" class="visually-hidden" aria-hidden="true">Credit Card Number</label>

        
          <input autocomplete="cc-number" id="number" name="number" class="visually-hidden" aria-hidden="true" data-honeypot-field="" tabindex="-1" type="tel">
        
      
...
        
      
        <label for="expiry" class="visually-hidden" aria-hidden="true">MM / YY</label>

        
          <input autocomplete="cc-exp" id="expiry" name="expiry" aria-describedby="error-for-expiry" data-current-field="expiry" class="input-placeholder-color--lvl-34" style="color: rgb(51, 51, 51); font-family: Roboto, sans-serif; padding: 0.94em 0.8em; transition: padding 0.2s ease-out 0s;" placeholder="MM / YY" type="tel">
          <span id="error-for-expiry" class="input-error-message visually-hidden"></span>
        
      
        <label for="verification_value" class="visually-hidden" aria-hidden="true">CVV</label>

        
...
    </form>

I clicked autofill in the CC number input and the expiry filled in with the wrong format: yyyy-mm instead of mm / yy

Comment 1

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

Hi Jonathan,

I found the same issue at [1] which is using Shopify as well. May I know the URL of the website that you tried? This is useful to verify the issue by myself. Thanks.

I also found [1] will correct the cc-exp value when I click on the cc-exp field to show the popup, but the cc-exp value won't be fixed when the popup shows on other fields (e.g. cc-number). I guess this is related to the `change` or `blur` event for the web site to correct the value. (See bug 1395928)

[1] https://www.deathwishcoffee.com

Comment 2

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

Attachment: Bug 1406771 - Use input's placeholder to adjust the format of cc-exp.

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

Comment 3

[Jonathan Kingston [:jkt] he/him]

Sean, I responded with the URL in Slack, I would prefer to keep this off Bugzilla as I rarely pay with CC online. Not that security by obscurity is a good tactic. Thanks for the quick patch.

Comment 4

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

Comment on attachment 8916558 [details]
Bug 1406771 - Use input's placeholder to adjust the format of cc-exp.

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

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8916558 [details]
Bug 1406771 - Use input's placeholder to adjust the format of cc-exp.

https://reviewboard.mozilla.org/r/187706/#review195292

::: browser/extensions/formautofill/FormAutofillHandler.jsm:292
(Diff revision 2)
> +    if (/mm\s*\/\s*yyyy/gi.test(element.placeholder)) {
> +      profile["cc-exp"] = String(profile["cc-exp-month"]).padStart(2, "0") + "\/" +
> +        profile["cc-exp-year"];
> +    } else if (/mm\s*\/\s*yy/gi.test(element.placeholder)) {
> +      profile["cc-exp"] = String(profile["cc-exp-month"]).padStart(2, "0") + "\/" +
> +        String(profile["cc-exp-year"]).substr(-2);
> +    }

"g" is unnecessary. BTW, I think we should handle more cases such as "mmyy", "yyyy/mm", etc if it's not too complicated. What I'm thinking of is something like:

```js
let result;

result = /(?:[^m]|\b)(m{1,2})([^my]*)(y{2,4})(?!y)/i.exec(placeholder);
if (result) {
  ccExp = String(ccExpMonth).padStart(result[1].length, "0") +
          result[2].trim() +
          String(ccExpYear).substr(-1 * result[3].length);
  return;
}
  
result = /(?:[^y]|\b)(y{2,4})([^my]*)(m{1,2})(?!m)/i.exec(placeholder);
if (result) {
  ccExp = String(ccExpYear).substr(-1 * result[1].length) +
          result[2].trim() +
          String(ccExpMonth).padStart(result[3].length, "0");
  return;
}
```

What do you think?

Comment 6

[Matthew N. [:MattN]]

Please remember to assign a priority if a bug is being worked on.

Comment 7

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

Comment on attachment 8916558 [details]
Bug 1406771 - Use input's placeholder to adjust the format of cc-exp.

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

Comment 8

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

Comment on attachment 8916558 [details]
Bug 1406771 - Use input's placeholder to adjust the format of cc-exp.

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

Comment 9

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

Comment on attachment 8916558 [details]
Bug 1406771 - Use input's placeholder to adjust the format of cc-exp.

https://reviewboard.mozilla.org/r/187706/#review195292

> "g" is unnecessary. BTW, I think we should handle more cases such as "mmyy", "yyyy/mm", etc if it's not too complicated. What I'm thinking of is something like:
> 
> ```js
> let result;
> 
> result = /(?:[^m]|\b)(m{1,2})([^my]*)(y{2,4})(?!y)/i.exec(placeholder);
> if (result) {
>   ccExp = String(ccExpMonth).padStart(result[1].length, "0") +
>           result[2].trim() +
>           String(ccExpYear).substr(-1 * result[3].length);
>   return;
> }
>   
> result = /(?:[^y]|\b)(y{2,4})([^my]*)(m{1,2})(?!m)/i.exec(placeholder);
> if (result) {
>   ccExp = String(ccExpYear).substr(-1 * result[1].length) +
>           result[2].trim() +
>           String(ccExpMonth).padStart(result[3].length, "0");
>   return;
> }
> ```
> 
> What do you think?

Thanks for addressing this. I tweak it a little because the second argument (result[2]) seems loose. Do you agree with this change?
([^my]*)   ==>>   \s*([-/\\]*)\s

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8916558 [details]
Bug 1406771 - Use input's placeholder to adjust the format of cc-exp.

https://reviewboard.mozilla.org/r/187706/#review195292

> Thanks for addressing this. I tweak it a little because the second argument (result[2]) seems loose. Do you agree with this change?
> ([^my]*)   ==>>   \s*([-/\\]*)\s

It makes sense. Thanks.

Comment 11

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8916558 [details]
Bug 1406771 - Use input's placeholder to adjust the format of cc-exp.

https://reviewboard.mozilla.org/r/187706/#review196162

Comment 12

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

Thanks for the review!

Now we can handle the following cases:
mmyyyy
mmyy
mm-yy
mm-yyyy
yyyymm
yymm
yy-mm
yyyy-mm

"-" can be replaced with "/" as well.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8843cf82310
Use input's placeholder to adjust the format of cc-exp. r=lchang

Comment 14

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

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