1477114 - Need an asterisk for in-field label on credit card number and CVV field.

Status: VERIFIED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Jean Gong :jgong]

Need an asterisk for in-field label on credit card number and CVV field.

Comment 1

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

Attachment: Bug 1477114 - Add an asterisk to the required fields of the credit card form as well as the CVV placeholder.

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

Comment 2

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

This bug says to add an asterisk to the CVV placeholder, which I did, but some debit cards such as Maestro don't have a CVV. Are we going to just style this as required but not enforce it? Or should we put the asterisk on there conditionally after we can determine which type of card it is?

Comment 3

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

Comment on attachment 8996112 [details]
Bug 1477114 - Add an asterisk to the required fields of the credit card form as well as the CVV placeholder.

https://reviewboard.mozilla.org/r/260362/#review267902

r=me with the test and refactoring suggestions. I guess the form-autofill.css issue is more a question than an issue.

::: browser/components/payments/res/containers/autofill-form.css:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I can see that it makes sense to move this shared code into a new file for both card and address forms - and the CVV field on the summary page. But why "autofill-form.css" rather than just "form.css" or "form-common.cs" or something?

::: browser/components/payments/res/containers/basic-card-form.js:176
(Diff revision 1)
>        } else {
>          billingAddressSelect.value = Object.keys(addresses)[0];
>        }
>      }
>  
> +    for (let formElement of this.form.elements) {

We may want to toggle required-ness on the fly without having to re-`render()`, so could you move this to its own (internal) method and call that from here?

::: browser/components/payments/res/containers/basic-card-form.js:176
(Diff revision 1)
>        } else {
>          billingAddressSelect.value = Object.keys(addresses)[0];
>        }
>      }
>  
> +    for (let formElement of this.form.elements) {

Can you add a test to test_basic_card_form.html to verify that toggling a required attribute on say the expiry month and year do indeed toggle that attribute on the nearest label?

Comment 4

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

Comment on attachment 8996112 [details]
Bug 1477114 - Add an asterisk to the required fields of the credit card form as well as the CVV placeholder.

https://reviewboard.mozilla.org/r/260362/#review267902

> I can see that it makes sense to move this shared code into a new file for both card and address forms - and the CVV field on the summary page. But why "autofill-form.css" rather than just "form.css" or "form-common.cs" or something?

Naming is hard ;) I switched to form.css.

Comment 5

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

Comment on attachment 8996112 [details]
Bug 1477114 - Add an asterisk to the required fields of the credit card form as well as the CVV placeholder.

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

Comment 6

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

Comment on attachment 8996112 [details]
Bug 1477114 - Add an asterisk to the required fields of the credit card form as well as the CVV placeholder.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d402c7b803c6
Add an asterisk to the required fields of the credit card form as well as the CVV placeholder. r=sfoster

Comment 8

[Cristian Brindusan [:cbrindusan]]

Backed out changeset d402c7b803c6 (Bug 1477114) as requested by jaws.

Backout: https://hg.mozilla.org/integration/autoland/rev/ade87af5847e5ca237895a9fda96fe0867ee3a60

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d402c7b803c69b11b28bfa01a80de034a07690c5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Comment 9

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

Comment on attachment 8996112 [details]
Bug 1477114 - Add an asterisk to the required fields of the credit card form as well as the CVV placeholder.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b2d475dff2c
Add an asterisk to the required fields of the credit card form as well as the CVV placeholder. r=sfoster

Comment 11

[Matthew N. [:MattN]]

After our decision today there should be an asterisk on all credit card fields like the UI spec had at one point. We won't support cards not associated with a credit card network.

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8b2d475dff2c

Comment 13

[Timea Cernea [:tbabos][inactive]]

Hi Matthew,

Based on the Invision doc (https://mozilla.invisionapp.com/share/SDFY4PA4EQ7#/screens/310857527), there is an asterisk for the month and year of expiration date as well. 
Since there is a bug submitted for the expiration date (to be required or not) I can close this issue as verified - fixed given that the CC-number, Name and CVV code fields have an asterisk. 

Does it sound good on your side or a different approach is needed?

Comment 14

[Matthew N. [:MattN]]

Yes, the asterisk should get added in bug 1480719

Comment 15

[Timea Cernea [:tbabos][inactive]]

Thanks for the response!
Marking this issue as Verified - Fixed based on Comment 13 and Comment 14.