1475080 - Cleanup the pickers to match the spec

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[:prathiksha]

Link to spec: https://mozilla.invisionapp.com/share/SDFY4PA4EQ7#/screens/304878909

Comment 1

[Matthew N. [:MattN]]

Note that the pickers aren't only on the summary page e.g. this applies to the billing address picker too.

Comment 2

[:prathiksha]

Attachment: Bug 1475080 - Clean up the pickers on the payment summary page to match the spec.

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

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8992773 [details]
Bug 1475080 - Clean up the pickers on the payment summary page to match the spec.

https://reviewboard.mozilla.org/r/257622/#review264536

I think you got the right idea visually but I think we could share some of the logic/styles more.

::: browser/components/payments/res/components/rich-select.css:6
(Diff revision 1)
> -  display: inline-block;
> +  margin: 14px 0;
> +  display: block;
>    position: relative;
> +  border: 1px solid #0C0C0D33;

Nit: generally I like to keep properties sorted alphabetically… it's not always followed unfortunately.

::: browser/components/payments/res/containers/address-picker.js:21
(Diff revision 1)
>    constructor() {
>      super();
> +    this.pickerLabel = document.createElement("label");
> +    this.pickerLabel.className = "picker-label";
>      this.dropdown = new RichSelect();
>      this.dropdown.addEventListener("change", this);
>      this.dropdown.setAttribute("option-type", "address-option");

Not that the pickers are becoming more complex and sharing more logic I think we should make a shared/common base class for them e.g. `RichPicker`

::: browser/components/payments/res/containers/address-picker.js:23
(Diff revision 1)
> +    this.pickerLabel = document.createElement("label");
> +    this.pickerLabel.className = "picker-label";

Nit: Since this is in a Picker class, the `picker` prefix here is redundant. Please use `labelElement` instead.

::: browser/components/payments/res/containers/address-picker.js:40
(Diff revision 1)
>    connectedCallback() {
> -    this.appendChild(this.dropdown);
> +    this.appendChild(this.pickerLabel);
>      this.appendChild(this.addLink);
> -    this.append(" ");
>      this.appendChild(this.editLink);
> +    this.appendChild(this.dropdown);

In order for a label to actually work at labelling the field you need to either nest the contents to label inside the label's body or you need to use the @for attribute which required an ID:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label#Attributes

I can see that using the nesting may cause problems for our CSS so I think we should generate a random ID and use that after the `this.pickerLabel = …` line in the constructor.

::: browser/components/payments/res/containers/payment-dialog.js
(Diff revision 1)
> -    this._shippingTypeLabel.querySelector("label").textContent =
> -      this._shippingTypeLabel.dataset[shippingType + "AddressLabel"];

I think this logic should stay here and set a .label observed property on the shipping address picker element. Pickers would always use `this.label` to render the label text content and outside consumers are responsible for updating that `label` observed attribute to be correct in the context.

::: browser/components/payments/res/containers/rich-picker.css:10
(Diff revision 1)
> +a {
> +  float: right;
> +  padding: 5px;
> +}

This will apply to all <a> in the document which wouldn't be good

::: browser/components/payments/res/containers/rich-picker.css:10
(Diff revision 1)
> +a {
> +  float: right;
> +  padding: 5px;

Was this padding defined in the spec? If not, please ask Eric for the value or ask if it should just be a normal space on each side of the separator.

Comment 4

[:prathiksha]

Comment on attachment 8992773 [details]
Bug 1475080 - Clean up the pickers on the payment summary page to match the spec.

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

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8992773 [details]
Bug 1475080 - Clean up the pickers on the payment summary page to match the spec.

https://reviewboard.mozilla.org/r/257622/#review264928

I still need to manually test this and review the CSS but otherwise this looks great!

::: browser/components/payments/res/containers/address-picker.js:27
(Diff revision 2)
>      if (oldValue !== newValue) {
>        this.render(this.requestStore.getState());
>      }

You should probably add a check of `name` here now that it runs for other attributes.

::: browser/components/payments/res/containers/payment-dialog.js:254
(Diff revision 2)
> +    /* eslint-disable-next-line max-len */
> +    this._shippingAddressPicker.setAttribute("label", this._shippingAddressPicker.dataset[shippingType + "AddressLabel"]);

Nit: use a temporary variable instead of disbling the eslint rule.

::: browser/components/payments/res/containers/rich-picker.js:17
(Diff revision 2)
> +
> +  constructor() {
> +    super();
> +    this.dropdown = new RichSelect();
> +    this.dropdown.addEventListener("change", this);
> +    this.dropdown.id = Math.floor(Math.random() * 1000);

Nit: A 1/1000 chance of collision isn't that low… maybe bump this up a bit… maybe 1 million?

::: browser/components/payments/res/paymentRequest.xhtml:108
(Diff revision 2)
> -          <div class="shipping-related"><label>&shippingOptionsLabel;</label></div>
> -          <shipping-option-picker class="shipping-related"></shipping-option-picker>
> +          <shipping-option-picker class="shipping-related"
> +                                  label="&shippingOptionsLabel;"></shipping-option-picker>

Hmm… I just realized that this string isn't changing based upon the shippingType which is wrong. Can you file a follow-up (ideally blocking the bug that should have done the work) for this please?

Comment 6

[:prathiksha]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3)
> Comment on attachment 8992773 [details]
> Bug 1475080 - Clean up the pickers on the payment summary page to match the
> spec.

> ::: browser/components/payments/res/containers/rich-picker.css:10
> (Diff revision 1)
> > +a {
> > +  float: right;
> > +  padding: 5px;
> 
> Was this padding defined in the spec? If not, please ask Eric for the value
> or ask if it should just be a normal space on each side of the separator.


I have now set the padding as 8px after talking to Eric about this.

Comment 7

[Matthew N. [:MattN]]

(In reply to :prathiksha from comment #6)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #3)
> > Comment on attachment 8992773 [details]
> > Bug 1475080 - Clean up the pickers on the payment summary page to match the
> > spec.
> 
> > ::: browser/components/payments/res/containers/rich-picker.css:10
> > (Diff revision 1)
> > > +a {
> > > +  float: right;
> > > +  padding: 5px;
> > 
> > Was this padding defined in the spec? If not, please ask Eric for the value
> > or ask if it should just be a normal space on each side of the separator.
> 
> 
> I have now set the padding as 8px after talking to Eric about this.

OK, in the future you should probably try to ask for for "em" units so it adjusts with the font-size, at least offer that as an option. We can leave it as px for now though.

Comment 8

[:prathiksha]

Comment on attachment 8992773 [details]
Bug 1475080 - Clean up the pickers on the payment summary page to match the spec.

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

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8992773 [details]
Bug 1475080 - Clean up the pickers on the payment summary page to match the spec.

https://reviewboard.mozilla.org/r/257622/#review265250

::: browser/components/payments/res/containers/address-picker.js:26
(Diff revisions 2 - 3)
> +    if (name == "label") {
> -    super.attributeChangedCallback(name, oldValue, newValue);
> +      super.attributeChangedCallback(name, oldValue, newValue);
> +    }
>      if (oldValue !== newValue) {
>        this.render(this.requestStore.getState());
>      }

Sorry, I guess there was a misunderstand/miscommunication… I meant to check the `name == "address-fields" && oldValue !== newValue` since that's what the .render was needed for. The changing the label directly changes the textContent so we don't need a render.

::: browser/components/payments/res/containers/payment-dialog.js:254
(Diff revisions 2 - 3)
>      let shippingType = state.request.paymentOptions.shippingType || "shipping";
>      this._shippingAddressPicker.dataset.addAddressTitle =
>        this.dataset[shippingType + "AddressTitleAdd"];
>      this._shippingAddressPicker.dataset.editAddressTitle =
>        this.dataset[shippingType + "AddressTitleEdit"];
> -    /* eslint-disable-next-line max-len */
> +    let addressPickerLabel =  this._shippingAddressPicker.dataset[shippingType + "AddressLabel"];

Nit: there is a double-space after `=`

Comment 10

[:prathiksha]

Comment on attachment 8992773 [details]
Bug 1475080 - Clean up the pickers on the payment summary page to match the spec.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d59d8dcaf84e
Clean up the pickers on the payment summary page to match the spec. r=MattN

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8992773 [details]
Bug 1475080 - Clean up the pickers on the payment summary page to match the spec.

https://reviewboard.mozilla.org/r/257622/#review265280

I've made these changes and pushed to inbound. So exciting how much nicer the dialog looks after this!

::: browser/components/payments/res/containers/rich-picker.css:20
(Diff revision 4)
> +payment-method-picker a,
> +address-picker a {
> +  float: right;
> +  padding: 0 8px;
> +}

I don't think the picker CSS needs to know that the shipping-option-picker has the add/edit hidden, the @hidden will take care of that. Can you make this rule use only the selectors
```css
.rich-picker > .add-link,
.rich-picker > .edit-link {…
```

which means adding:
```js
this.classList.add("rich-picker");
```
to the RichPicker constructor.

::: browser/components/payments/res/containers/rich-picker.css:28
(Diff revision 4)
> +  padding: 0 8px;
> +}
> +
> +payment-method-picker {
> +  display: grid;
> +  grid-template-columns: 5fr 1fr auto auto;

20fr for the first position seems more in the ballpark of the spec

::: browser/components/payments/res/containers/rich-picker.css:30
(Diff revision 4)
> +    "label    spacer edit add"
> +    "cc-number cvv   cvv cvv";

Nit: vertically line up `spacer` and the first `cvv`

::: browser/components/payments/res/containers/rich-picker.css:34
(Diff revision 4)
> +payment-method-picker rich-select {
> +  grid-area: cc-number;
> +}
> +
> +payment-method-picker input {

Nit: for both of these rules, use a child (>) selector instead of a descendant selector for better performance:

e.g. `payment-method-picker > rich-select`

::: browser/components/payments/res/containers/rich-picker.js:17
(Diff revision 4)
> +
> +  constructor() {
> +    super();
> +    this.dropdown = new RichSelect();
> +    this.dropdown.addEventListener("change", this);
> +    this.dropdown.id = Math.floor(Math.random() * 1000000);

I guess you didn't test the labelling… the id needs to be on the <select> as it's the thing that gets focus.

::: browser/components/payments/res/containers/rich-picker.js:20
(Diff revision 4)
> +    this.dropdown = new RichSelect();
> +    this.dropdown.addEventListener("change", this);
> +    this.dropdown.id = Math.floor(Math.random() * 1000000);
> +    this.labelElement = document.createElement("label");
> +    this.labelElement.className = "picker-label";
> +    this.labelElement.setAttribute("for", this.dropdown.id);

And then this line has to be updated to match the change above.

You will know it works if clicking on the label focuses the <select>. You can also see the label on the <select> in the accessibility inspector.

::: browser/components/payments/res/containers/rich-picker.js:35
(Diff revision 4)
> +    this.appendChild(this.addLink);
> +    this.appendChild(this.editLink);

Please invert the order of these two lines so that the tab order is correct for the links. You may have to fix testa that were using :nth-of-type link selectors and make them use the .add-link and .edit-link classes (but I may have already cleaned those up before). You should also move them to append after appending this.dropdown so that edit and add come after the dropdown in the tab order. With all of these changes the float approach you use for non-payment pickers won't work so it's probably easiest to switch to using grid for all pickers.

Comment 13

[Cosmin Sabou [:CosminS]]

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

Comment 14

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #5)
> ::: browser/components/payments/res/paymentRequest.xhtml:108
> (Diff revision 2)
> > -          <div class="shipping-related"><label>&shippingOptionsLabel;</label></div>
> > -          <shipping-option-picker class="shipping-related"></shipping-option-picker>
> > +          <shipping-option-picker class="shipping-related"
> > +                                  label="&shippingOptionsLabel;"></shipping-option-picker>
> 
> Hmm… I just realized that this string isn't changing based upon the
> shippingType which is wrong. Can you file a follow-up (ideally blocking the
> bug that should have done the work) for this please?

Reminder to file this when you have a chance.

Comment 15

[:prathiksha]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #14)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #5)
> > ::: browser/components/payments/res/paymentRequest.xhtml:108
> > (Diff revision 2)
> > > -          <div class="shipping-related"><label>&shippingOptionsLabel;</label></div>
> > > -          <shipping-option-picker class="shipping-related"></shipping-option-picker>
> > > +          <shipping-option-picker class="shipping-related"
> > > +                                  label="&shippingOptionsLabel;"></shipping-option-picker>
> > 
> > Hmm… I just realized that this string isn't changing based upon the
> > shippingType which is wrong. Can you file a follow-up (ideally blocking the
> > bug that should have done the work) for this please?
> 
> Reminder to file this when you have a chance.

Filed Bug 1477534. :)