Create a currency-amount Custom Element

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: MattN, Assigned: MattN)

Tracking

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

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

Example:
> <currency-amount value="7.5" currency="USD"></currency-amount>
would render
> $7.50
Priority: -- → P1
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"
Attachment #8928789 - Flags: review?(mcaceres) → review-
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 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 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 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 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 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.
Attachment #8928789 - Flags: review- → review+
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 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!
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/84f6ddc49882
Create a currency-amount Custom Element. r=marcosc
https://hg.mozilla.org/mozilla-central/rev/84f6ddc49882
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
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.