1417749 - Create a currency-amount Custom Element

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

Example:
> <currency-amount value="7.5" currency="USD"></currency-amount>
would render
> $7.50

Comment 1

[Matthew N. [:MattN]]

This will be inspired by PaymentCurrencyAmount: https://w3c.github.io/payment-request/#dom-paymentcurrencyamount

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1417749 - Create a currency-amount Custom Element.

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

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

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

Comment 4

[Marcos Caceres [:marcosc]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205782

Looking great! Like the `ObservedPropertyMixin`. 

However, unless I've misundertood, the value validity check is not correct - hence my r-.

::: toolkit/components/payments/res/components/currency-amount.js:21
(Diff revisions 1 - 2)
>    render() {
> -    console.log("render");
> +    let output = "";
> +    try {
> -    if (this.value && this.currency) {
> +      if (this.value && this.currency) {
> +        let number = Number.parseFloat(this.value);
> +        if (Number.isNaN(number) || !Number.isFinite(number)) {

I don't think this matchs the spec (we assume it's a string of digits possibly with an optional ".")... but there is no requirement that it be a finite number.

See https://www.w3.org/TR/payment-request/#validity-checkers

The regex is "^-?[0-9]+(\.[0-9]+)?$"

::: toolkit/components/payments/test/mochitest/test_ObservedPropertiesMixin.html:58
(Diff revision 2)
> +  is(el1.one, "a", "Check .one value");
> +  is(el1.getAttribute("one"), "a", "Check @one");
> +  is(el1.twoWord, "b", "Check .twoWord value");
> +  is(el1.getAttribute("two-word"), "b", "Check @two-word");
> +  let expected = `{"one":"a","twoWord":"b"}`;
> +  await Promise.resolve();

nit: missing "// Wait for the async render"

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205782

> I don't think this matchs the spec (we assume it's a string of digits possibly with an optional ".")... but there is no requirement that it be a finite number.
> 
> See https://www.w3.org/TR/payment-request/#validity-checkers
> 
> The regex is "^-?[0-9]+(\.[0-9]+)?$"

Hmm… I wasn't try to match the spec, mostly trying to avoid NaN. I originally didn't have this line at all but didn't really like non-numeric values turning into `NaN`.  Does the DOM code be doing the validation you linked to already? If so, then I don't think we need the same here though I can if you really want… it just makes the element more specific to PaymentRequest.

> nit: missing "// Wait for the async render"

I left it out since the reason should have been learned from the earlier uses. I was thinking it may be easier to have a one-line helper like `await asyncElementRendered()` which simply returns Promise.resolve() so that I don't need to explain it in every test. Thoughts?

Comment 6

[Marcos Caceres [:marcosc]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205782

> Hmm… I wasn't try to match the spec, mostly trying to avoid NaN. I originally didn't have this line at all but didn't really like non-numeric values turning into `NaN`.  Does the DOM code be doing the validation you linked to already? If so, then I don't think we need the same here though I can if you really want… it just makes the element more specific to PaymentRequest.

Yep, the DOM implementation does the check so we are assured to always get back a valid currency value :)

> I left it out since the reason should have been learned from the earlier uses. I was thinking it may be easier to have a one-line helper like `await asyncElementRendered()` which simply returns Promise.resolve() so that I don't need to explain it in every test. Thoughts?

I see, I guess you could even just do:

```
// Waits for next tick/render
const tick = Promise.resolve(); 
```

and then just add:

```
await tick;
```

as needed. No biggy if you don't change this tho, it's pretty clear what the test is doing.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205782

> Yep, the DOM implementation does the check so we are assured to always get back a valid currency value :)

So which option do you prefer? Remove the check so we render NaN, leave it as-is, or duplicate the regex check?

> I see, I guess you could even just do:
> 
> ```
> // Waits for next tick/render
> const tick = Promise.resolve(); 
> ```
> 
> and then just add:
> 
> ```
> await tick;
> ```
> 
> as needed. No biggy if you don't change this tho, it's pretty clear what the test is doing.

True

Comment 8

[Marcos Caceres [:marcosc]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205782

> So which option do you prefer? Remove the check so we render NaN, leave it as-is, or duplicate the regex check?

I'd prefer we remove the check, as it's not needed. We should never treat the value as an number (it's always a string), so we should never (in theory) end up with a NaN situation. If we do, that's a DOM bug.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205782

> I'd prefer we remove the check, as it's not needed. We should never treat the value as an number (it's always a string), so we should never (in theory) end up with a NaN situation. If we do, that's a DOM bug.

okay, but it's possible that a front-end bug ends up setting @value to "null", "undefined", or some other string which would cause NaN. In some ways it's good for pre-release so we get a bug report (less likely with an empty string) but it's somewhat more embarassing for release.

Comment 10

[Marcos Caceres [:marcosc]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205816

r+ from me, given discussion about error handling.

Comment 11

[Marcos Caceres [:marcosc]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205782

> okay, but it's possible that a front-end bug ends up setting @value to "null", "undefined", or some other string which would cause NaN. In some ways it's good for pre-release so we get a bug report (less likely with an empty string) but it's somewhat more embarassing for release.

Yeah, if the value is not going to be coming from the DOM implementation, then I agree - we need the check. I'll flip to r+ from me.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

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

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8928789 [details]
Bug 1417749 - Create a currency-amount Custom Element.

https://reviewboard.mozilla.org/r/200056/#review205782

> Yeah, if the value is not going to be coming from the DOM implementation, then I agree - we need the check. I'll flip to r+ from me.

Thanks for the quick review!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84f6ddc49882
Create a currency-amount Custom Element. r=marcosc

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/84f6ddc49882