1504960 - CC number field validation applies the "..at least N characters" tooltip instead of the Luhn Algorithm

Status: NEW

(Firefox :: WebPayments UI, defect, P2)

Description

[Timea Cernea [:tbabos][inactive]]

Attachment: Video of the issue

[Affected versions]: 
Nightly 65.0a1 

[Affected platforms]:
Platforms: Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.

[Preconditions]:
1. Set the pref dom.payments.request.enabled to "true"
2. Set browser.search.region to "US" or "CA" if you are outside of the US or Canada


[Steps to reproduce]:
1. Go to "https://rsolomakhin.github.io/pr/single/" page and click on "Buy".
2. Add a new Credit Card
3. Fill in all the fields except CC number
4. Type in the CC field any number and delete it
5. Click out the field to trigger the validation 
5. Click on the field and type in 9 digits number as suggested by the tooltip

[Expected result]:
The field should be considered valid only after the Luhn Algorithm checks the credit card number.

[Actual result]:
The field will lose its invalid state after 9 numbers are typed even if its not a valid credit card number. The "Next" button also becomes enabled. For some reason, the entered number passes the field validation if it respects the tooltip instead of passing the Luhn Algorithm for valid credit cards. Please check the attached file.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 9022867 [details]
Video of the issue

This is the wrong video attachment for this bug.

Comment 2

[Matthew N. [:MattN]]

Hmm… seems like the setCustomValidity code stops working? I guess this is something to fix. I haven't tried to repro yet.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Its also bug that it says please use at least 9 characters. We need at least 12. That's the minlength attribute on the #cc-number field.

Comment 4

[Matthew N. [:MattN]]

Could you upload the correct video please? Jared wasn't able to repro.

Comment 5

[Timea Cernea [:tbabos][inactive]]

Attachment: Video of the issue

Sorry for that! Attached the right file.

Comment 6

[Diego Pino]

I took a look at the bug and tried to figure out what's going on:

- CreditCard validation (luhn algorithm) happens on `onchange` and `oninput` events.
- `handleInput` (https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/content/autofillEditForms.js#459) checks whether the credit card number is valid, and in that case clears any previous error message (setCustomValidity('')).
- `handleChange` (https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/content/autofillEditForms.js#443) checks whether the credit card number is valid when a change event happens (that means when the value of the input gets actually updated).
- The `invalidCardNumberString` message is only set in an `onchange` event, that's why when the minLength constraint is met there's no credit card number validation. When the user presses the Next button or when the credit card field loses its focus, the `onchange` event is triggered and the credit card number gets validated (as shown in the video).

I think the issue described is not exactly a bug, since it's not possible to end in an erroneous situation (I agree it might seem confusing depending on what an user expects). With the current behavior, a wrong cc value is marked as invalid only when an `onchange` event is triggered (I don't know if it would be possible to trigger a cc validation as soon as all the constraints in the input are met, but afaik there's not such thing as `onvalid` event).

That said, there's a situation very similar to the bug described where it's possible to end in an erroneous situation. Steps:

- Insert '000000000' in the credit card number field.
- Press 'Next'. An `onchange` event is triggered, the credit card number is validated and the field is marked as invalid.
- Edit the credit card number, remove one character and insert the same character again (at this point the current value is the same as before ('000000000')). The value is no longer marked as invalid (as it was cleared by `oninput` and the minLength constraint is met).
- Press 'Next'. The `onchange` event won't be triggered since the credit card number didn't change, thus the credit card number won't be validated.
- After pressing 'Next' the following error message is shown: "There was an error saving the payment card.".

It seems doing cc number validation in an `onchange` event is not enough. Perhaps the 'Next' button should validate the cc number, without relying in `onchange`. Perhaps it's possible to find a solution that fixes the original bug described as well as this scenario.

Comment 7

[Diego Pino]

Attachment: Bug-1504960-CC-number-field-validation-applies-the-..patch

I made a patch that attaches an `onkeyup` event to the form. When a key is pressed in ccNumber, if the field is valid, it checks whether the ccNumber value passes the luhn algorithm. The desired result would be that as soon as ccNumber equals the minimum length, its value gets validated.

However, the patch doesn't work as planned. Once ccNumber meets the minimum length, despite setCustomValidity('Please enter a valid card number') is called, the minimum length error message is still shown. I don't understand why this happens.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9025307 [details] [diff] [review]
Bug-1504960-CC-number-field-validation-applies-the-..patch

Review of attachment 9025307 [details] [diff] [review]:
-----------------------------------------------------------------

You are fighting the "input" event handler currently. Input is the better event here as that accounts for cut/paste and other non-keyboard ways of modifying the value. 
There is an existing handleInput method right below the keyupHandler you added. Moving the logic to setCustomValidity(invalidCardNumberString || " ") there should do what you want. 

Please also change the minlength attribute to "12" at https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/content/editCreditCard.xhtml#27 That was my omission, and brings this into line with the range we specify in the CreditCard.jsm's isValidNumber method.

Comment 9

[Diego Pino]

Attachment: Bug-1504960-CC-number-field-validation-applies-the-..patch

I moved the code to `input` method. Once cc-number has a value equals to 12 characters, if the card is not a valid number I can see there's an error (there's a log message), however the input field doesn't prints out the error neither is marked as invalid.

I made a small test that sets a custom validity error when an input field is longer than 9 characters: https://jsfiddle.net/nL0keubj/ . It works as expected in 65.0a1, so it doesn't seem to be an error. What could be the issue why the error message is not popping out? Any ideas?

Note: Now that minLength is updated to 12 characters, an invalid cc-number is: '111111111111' (twelve 0 character is apparently a valid cc-number).

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 9025654 [details] [diff] [review]
Bug-1504960-CC-number-field-validation-applies-the-..patch

Review of attachment 9025654 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/formautofill/content/autofillEditForms.js
@@ +458,5 @@
>  
>    handleInput(event) {
> +    let ccNumberField = this._elements.ccNumber;
> +
> +    if (ccNumberField.validity.valid) {

Shouldn't we still have an `event.target` check?

@@ +459,5 @@
>    handleInput(event) {
> +    let ccNumberField = this._elements.ccNumber;
> +
> +    if (ccNumberField.validity.valid) {
> +      if (!this.isCCNumber(ccNumberField.value)) {

This is an unnecessary negation as you can simple reverse the order of the if/else bodies to avoid it.

Comment 11

[Diego Pino]

Attachment: Bug-1504960-CC-number-field-validation-applies-the-..patch

Addressed comments from last reviews.

The patch is still not working as expected. When introducing an invalid cc-number, `handleInput` detects the cc-number is not correct and marks the fields as invalid, however that's not reflected in the interface. I don't know if this is a CSS issue or what else could be.

https://jsfiddle.net/nL0keubj/8/ shows the behavior intended (as soon as cc-number meets the required minLength, the field is marked as invalid with a custom message).

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 9026687 [details] [diff] [review]
Bug-1504960-CC-number-field-validation-applies-the-..patch

Review of attachment 9026687 [details] [diff] [review]:
-----------------------------------------------------------------

I'll probably only dig into the problem you're having next week. In the meantime I would suggest adding logging in the various event handlers to understand the order they are running. Potentially ones in basic-card-form.js too if this problem doesn't happen in preferences?

::: browser/extensions/formautofill/content/autofillEditForms.js
@@ +458,5 @@
>  
>    handleInput(event) {
> +    if (event.target != this._elements.ccNumber) {
> +      return;
> +    }

Due to the `super.handleInput` at the bottom this isn't good.

Comment 13

[Diego Pino]

Attachment: Bug-1504960-CC-number-field-validation-applies-the-..patch

I made some progress in this bug. The reason why the custom message didn't shown up, despite handlInput marking the field as invalid was that `basic-card-form.js:onInput` always clears out the custom validity message.

So what I've done is to undo the changes in `autofillEditForms.js` and add an extra ccNumber validation in basic-card-form.js:onInput. The validation only takes place if ccNumber meets the minimum length, otherwise "Please enter a valid..." error message would always overwrite minlength error message.

The patch fixes the issue, but I'm not sure whether this is the best approach (ideally having this code in autofillEditForms.js would prevent a similar issue in add/edit cc preference dialog).

Comment 14

[Diego Pino]

Attachment: Bug-1504960-CC-number-field-validation-applies-the-..patch

Here is another approach. I like this one better as it fixes the issue at autofill level (which indeed is affected by this bug).

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 9027162 [details] [diff] [review]
Bug-1504960-CC-number-field-validation-applies-the-..patch

(In reply to Diego Pino from comment #14)
> Here is another approach. I like this one better as it fixes the issue at
> autofill level (which indeed is affected by this bug).

Me too so clearing review on the first one.

Comment 16

[Diego Pino]

Attachment: Bug-1504960-CC-number-field-validation-applies-the-..patch

Fixed nit: replaced hardcoded value "12" for minLength.

I think the patch is ready for review now.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 9029262 [details] [diff] [review]
Bug-1504960-CC-number-field-validation-applies-the-..patch

Review of attachment 9029262 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/formautofill/content/autofillEditForms.js
@@ +468,5 @@
> +
> +    if (ccNumberField.value.length >= ccNumberField.minLength) {
> +      if (!this.isCCNumber(ccNumberField.value)) {
> +        let invalidCardNumberString = this._elements.invalidCardNumberStringElement.textContent;
> +        ccNumberField.setCustomValidity(invalidCardNumberString || " ");

setCustomValidity (input.validity.customError) should be independent of the minLength check (input.validity.tooShort) so I don't think a lot of the complexity you're adding is necessary, and if it is then that seems like a bug in our DOM validity code.

It seems to me (without testing) that the only necessary change is changing the blanket `event.target.setCustomValidity("");` in basic-card-form to have it not clear the custom validity message in this case (assuming no DOM bugs).

What am I missing? If there is a DOM bug can you make a minimal test case?

Comment 18

[Diego Pino]

I tried what happens if only `event.target.setCustomValidity("");` in basic-card-form is removed.

That's enough for fixing the bug, however the Next button is enabled even if the cc-number is not valid under the following scenario:

1) Fill up all the fields except cc-number.
2) Fill up cc-number with a valid number (000000000000). The Next button is enabled.
3) Remove the last digit. The Next button is disabled
3) Insert 1 (cc-number becomes 000000000001). The cc-number is not valid, but isn't marked as invalid. The Next button is enabled.
4) On clicking Next, cc-number is marked as invalid.

I think enabling Next even though cc-number is not valid it might not be Ok, but at least the error is detected on clicking Next.

If it's desired that cc-number is checked and marked as invalid as soon as a character is inserted (and thus Next isn't enabled if cc-number is invalid) it's necessary to set a custom message in onInput. The reason why I checked whether minimumLength is met before checking cc validity is to not overwrite the minimumlength error with an invalid card error message (every too short cc-number is not a valid cc-number). The code can be simplified though, i.e:

```
  handleInput(event) {
    if (!event.target.validity.tooShort) {
        if (event.target == this._elements.ccNumber &&
          this.isCCNumber(this._elements.ccNumber.value)) {
          this._elements.ccNumber.setCustomValidity("");
        } else {
          let invalidCardNumberString = this._elements.invalidCardNumberStringElement.textContent;
          this._elements.ccNumber.setCustomValidity(invalidCardNumberString || " ");
        }
    }
    super.handleInput(event);
  }
```

Comment 19

[Matthew N. [:MattN]]

(In reply to Diego Pino from comment #18)
> I tried what happens if only `event.target.setCustomValidity("");` in
> basic-card-form is removed.
> 
> That's enough for fixing the bug, however the Next button is enabled even if
> the cc-number is not valid 

I think https://phabricator.services.mozilla.com/D14642 will fix that part.

It's possible that the two patches in bug 1474905 will already fix this bug but I'm not sure.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

MattN, can you re-prioritize this as it hasn't been touched in over a year but is marked as P1?