Closed Bug 1421806 Opened 7 years ago Closed 7 years ago

Use a custom element for the payment request dialog contents

Categories

(Firefox :: WebPayments UI, enhancement)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: MattN, Assigned: MattN)

Details

Attachments

(2 files)

Bug 1421806 - Create a mixin to subscribe to payment store changes.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1421806 - Use a custom element for the payment request dialog contents.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
For the top-level container element of the dialog, switch it to use a custom element which listens to state changes of the store.
Comment on attachment 8933200 [details]
Bug 1421806 - Create a mixin to subscribe to payment store changes.

https://reviewboard.mozilla.org/r/204140/#review209900

::: toolkit/components/payments/res/mixins/PaymentStateSubscriberMixin.js:14
(Diff revision 1)
> +/**
> + * A mixin for a custom element to observe store changes to information about a payment request.
> + */
> +
> +/**
> + * State of the payment request dialod.

s/dialod/dialog/

::: toolkit/components/payments/res/mixins/PaymentStateSubscriberMixin.js:60
(Diff revision 1)
> +      super();
> +      this.requestStore = requestStore;
> +    }
> +
> +    connectedCallback() {
> +      this.requestStore.subscribe(this);

From my previous review, I asked that we remove the side effect that subscribing would also trigger a render.

Here we could call this.render() after calling .subscribe()

::: toolkit/components/payments/test/mochitest/test_PaymentStateSubscriberMixin.html:54
(Diff revision 1)
> +  el1.requestStore.setState({a: 1});
> +  el1.requestStore.setState({b: 2});

I think it would be helpful if this was setting the savedAddresses or savedBasicCards instead of 'a' or 'b', since others may refer to the tests for documentation.

::: toolkit/components/payments/test/mochitest/test_PaymentStateSubscriberMixin.html:68
(Diff revision 1)
> +});
> +
> +add_task(async function test_disconnect() {
> +  el1.stateChangeCallback.reset();
> +  el1.render.reset();
> +  el1.remove();

Can you spy on disconnectedCallback to confirm that it was calledOnce?
Attachment #8933200 - Flags: review?(jaws) → review+
Comment on attachment 8933200 [details]
Bug 1421806 - Create a mixin to subscribe to payment store changes.

https://reviewboard.mozilla.org/r/204140/#review209900

> I think it would be helpful if this was setting the savedAddresses or savedBasicCards instead of 'a' or 'b', since others may refer to the tests for documentation.

The reason I intentionally didn't do that is because this test is trying to focus on testing the mixin, not the payments store. Given that, this test shouldn't rely on the shape of the store IMO
Comment on attachment 8933201 [details]
Bug 1421806 - Use a custom element for the payment request dialog contents.

https://reviewboard.mozilla.org/r/204142/#review210724

::: toolkit/components/payments/res/containers/payment-dialog.js:13
(Diff revision 3)
> +
> +/**
> + * <payment-dialog></payment-dialog>
> + */
> +
> +class PaymentDialog extends PaymentStateSubscriberMixin(HTMLElement) {

Have you tried using subclasses of HTMLElement? This could use HTMLDialogElement for example. I tried it (with HTMLOptionElement and HTMLSelectElement) and I was getting an exception about an illegal constructor.

::: toolkit/components/payments/res/debugging.js:118
(Diff revision 3)
>  let buttonActions = {
> +  logState() {
> +    let state = requestStore.getState();
> +    // eslint-disable-next-line no-console
> +    console.log(state);
> +    dump(`${JSON.stringify(state, null, 2)}\n`);

Please use console.log here as well, and then you can just log the `state` object and let the Console do the formatting. I've had issues with getting dump output on Windows.
Attachment #8933201 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> > +    console.log(state);
> > +    dump(`${JSON.stringify(state, null, 2)}\n`);
> 
> Please use console.log here as well, and then you can just log the `state`
> object and let the Console do the formatting. I've had issues with getting
> dump output on Windows.

Sorry, I missed that the console.log() already there is also dumping `state`. I didn't notice that they were dumping/logging the same thing.
Comment on attachment 8933201 [details]
Bug 1421806 - Use a custom element for the payment request dialog contents.

https://reviewboard.mozilla.org/r/204142/#review210724

> Have you tried using subclasses of HTMLElement? This could use HTMLDialogElement for example. I tried it (with HTMLOptionElement and HTMLSelectElement) and I was getting an exception about an illegal constructor.

I think this may partly be due to naming confusion which I've been wanting to clean up but didn't have great ideas. This is used in the inner frame of the "dialog" so doesn't represent the top-level container of the visual "dialog". See https://firefox-source-docs.mozilla.org/toolkit/components/payments/docs/index.html#dialog-architecture. The outer dialog is currently using `window.openDialog` until we have a proper tab-modal widget and I believe that actual outer dialog should already provide the correct accessible role. Once we create that final widget we'll need to make sure it's accessible.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/562dbac7f65c
Create a mixin to subscribe to payment store changes. r=jaws
https://hg.mozilla.org/integration/autoland/rev/bd96153b3e7c
Use a custom element for the payment request dialog contents. r=jaws
https://hg.mozilla.org/mozilla-central/rev/562dbac7f65c
https://hg.mozilla.org/mozilla-central/rev/bd96153b3e7c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Toolkit → Firefox
Target Milestone: mozilla59 → Firefox 59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: