Create a rich-select Custom Element

RESOLVED FIXED in Firefox 59

Status

()

P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [webpayments])

User Story

Markup TBD

Attachments

(1 attachment)

Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8937754 [details]
Bug 1422164 - Create a rich-select, address-option, and basic-card-option custom elements.

https://reviewboard.mozilla.org/r/208466/#review217004

Thanks

::: commit-message-55724:1
(Diff revision 2)
> +Bug 1422164 - WIP, create a rich-select Custom Element.

Remove WIP from the commit message

::: toolkit/components/payments/res/components/rich-select.js:24
(Diff revision 2)
> +
> +/* global ObservedPropertiesMixin */
> +
> +class RichSelect extends ObservedPropertiesMixin(HTMLElement) {
> +  static get observedAttributes() {
> +    return ["open", "disabled", "hidden"];

Nit: one per line with a trailing comma would be better for tracking blame. Apologies if I didn't follow this myself. I thought we actually had an eslint rule for that for the case where the array contains more than one value.

::: toolkit/components/payments/res/components/rich-select.js:76
(Diff revision 2)
> +      if (next) {
> +        next.selected = true;
> +        selectedChild.selected = false;
> +      }

We'll eventually need to distinguish the option that is selected from the option that the user is highlighting while the dropdown is open. At least on macOS I think the selectedness doesn't change until the user hits enter on the highlighted row.

Consider: We wouldn't want to tell the merchant that a new shipping address was chosen each time the arrow key was pressed until the popup closed on the dropdown. We can deal with that when we hook this up to the state object.

::: toolkit/components/payments/res/components/rich-select.js:192
(Diff revision 2)
> +    fragment.appendChild(element);
> +    return element;
> +  }
> +}
> +
> +customElements.define("rich-option", RichOption);

You don't need to register this element if it's only being used for inheritance.

::: toolkit/components/payments/res/components/rich-select.js:194
(Diff revision 2)
> +  }
> +}
> +
> +customElements.define("rich-option", RichOption);
> +
> +class AddressOption extends ObservedPropertiesMixin(RichOption) {

Please move `AddressOption` and `BasicCardOption` each to their own files.

::: toolkit/components/payments/res/components/rich-select.js:212
(Diff revision 2)
> +  constructor() {
> +    super();
> +  }

IIRC the default constructor already does this so this can be omitted.

::: toolkit/components/payments/res/components/rich-select.js:217
(Diff revision 2)
> +    for (let child of this.children) {
> +      child.remove();
> +    }
> +
> +    let fragment = document.createDocumentFragment();

It would be more performant if both of the Option classes only created the elements in the `constructor` or `connectedCallback` and assigned the elements references to a private variable and then have render just set `element.textContent` on the references. That would mean that a render would only affect the `textContent` and wouldn't have the overhead of document fragments and creating elements and setting classes.

For example, see line 78 of https://reviewboard.mozilla.org/r/196014/diff/4#file5723914

It's not strictly necessary at this point since we won't re-render often yet but it may be good to get into the practice of reducing overhead. You can file this as a follow-up if you'd like.

::: toolkit/components/payments/res/components/rich-select.js:245
(Diff revision 2)
> +  constructor() {
> +    super();
> +  }

IIRC the default constructor already does this so this can be omitted.

::: toolkit/components/payments/res/paymentRequest.xhtml:12
(Diff revision 2)
>  <head>
>    <meta http-equiv="Content-Security-Policy" content="default-src 'self'"/>
>    <title></title>
>    <link rel="stylesheet" href="paymentRequest.css"/>
> +  <link rel="stylesheet" href="rich-select.css"/>
> -  <script src="vendor/custom-elements.min.js"></script>
> +    <script src="vendor/custom-elements.min.js"></script>

I think there is accidental indentation here

::: toolkit/components/payments/res/paymentRequest.xhtml:42
(Diff revision 2)
>  </head>
>  <body>
>    <iframe id="debugging-console" hidden="hidden" src="debugging.html"></iframe>
>  
> +  <rich-select>
> +    <address-option email="emzembrano92@email.com"

Nit: please don't use real third-party domains for examples, use RFC2606 TLDs or second level domain names: https://tools.ietf.org/html/rfc2606#section-2

::: toolkit/components/payments/test/mochitest/mochitest.ini:5
(Diff revision 2)
>  [DEFAULT]
>  support-files =
>     ../../../../../testing/modules/sinon-2.3.2.js
>     ../../res/PaymentsStore.js
> +   ../../res/rich-select.css

I think the stylesheet should stay beside the component JS file in the same directory.

::: toolkit/components/payments/test/mochitest/test_rich_select.html:66
(Diff revision 2)
> +function is_visible(element, message) {
> +  let rect = element.getBoundingClientRect();
> +  return !rect.width && !rect.height;
> +}
> +
> +function is_hidden(element, message) {
> +  return !is_visible(element, message);
> +}

You can use `isHidden` from EventUtils.js instead: https://dxr.mozilla.org/mozilla-central/rev/ca379fcca95b1f4a3744242ea8647004b99b3507/testing/mochitest/tests/SimpleTest/EventUtils.js#158

Add ```html
<script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
```
after the SpawnTask.js line.

::: toolkit/components/payments/test/mochitest/test_rich_select.html:170
(Diff revision 2)
> +
> +  dispatchKeyPress("ArrowUp", 38);
> +  ok(option1.selected, "option 1 should remain selected");
> +  ok(!option3.selected, "option 3 should not be selected");
> +
> +  // Wait for any mutation observer notifications to come apply before exiting.

Nit: "to come apply" is confusing to read
Attachment #8937754 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment on attachment 8937754 [details]
Bug 1422164 - Create a rich-select, address-option, and basic-card-option custom elements.

https://reviewboard.mozilla.org/r/208466/#review217004

> Remove WIP from the commit message

It looks like somehow my last revision never got published. I've just published it now (where this is fixed) and the behavior of opening the select is better in the latest revision. Fixing the other issues now.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90da1b24e686
Create a rich-select, address-option, and basic-card-option custom elements. r=MattN
Comment hidden (mozreview-request)

Comment 16

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee57508c6e70
Create a rich-select, address-option, and basic-card-option custom elements. r=MattN

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee57508c6e70
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Whiteboard: [webpayments]
Component: WebPayments UI → WebPayments UI
Product: Toolkit → Firefox
Target Milestone: mozilla59 → Firefox 59
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.