ViewHelpers.L10N.numberWithDecimals doesn't properly handle NaN and numbers that can't be localized

RESOLVED FIXED in Firefox 33

Status

()

Firefox
Developer Tools
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
numberWithDecimals(0.0001, 2) // "00"

It should be "0" or "0.0" or "0,0" instead, but since we don't know the localization settings ("," or "."?), we should simply return "0".
(Assignee)

Comment 1

3 years ago
Created attachment 8445222 [details] [diff] [review]
v1
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8445222 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
Blocks: 879008
Comment on attachment 8445222 [details] [diff] [review]
v1

Review of attachment 8445222 [details] [diff] [review]:
-----------------------------------------------------------------

I think browser.ini needs to be rebased.  Looks good besides that, just a couple of questions about the early return.

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +375,5 @@
>      let localized = aNumber.toLocaleString(); // localize
> +
> +    // If no grouping or decimal characters are available, bail out, because
> +    // padding with zeros at the end of the string won't make sense anymore.
> +    if (!localized.contains(".") && !localized.contains(",")) {

I'm actually not sure what case this is catching.  Is this just an early return to prevent some computation or is this actually changing the output of the function?  For instance, if I run l10n.numberWithDecimals(123, 10); with or without the patch, I get the same result.

Also, do all locales use , for grouping?
Attachment #8445222 - Flags: review?(bgrinstead)
> I'm actually not sure what case this is catching.  Is this just an early
> return to prevent some computation or is this actually changing the output
> of the function?  For instance, if I run l10n.numberWithDecimals(123, 10);
> with or without the patch, I get the same result.

Scratch that, I see the case in the first comment
With the patch applied:

l10n.numberWithDecimals(0.0001, 2); // "0"
l10n.numberWithDecimals(0.001, 2);  // "0.00"
l10n.numberWithDecimals(1.00001, 2) // "1"

I'm not really sure what the right result here is, but it is a bit odd that some decimals become NUM and some become NUM.00.  Luckily this also fixed what looks like a really bad bug without the patch applied:

l10n.numberWithDecimals(1.00001, 2) // "10" ? huh
(Assignee)

Comment 5

3 years ago
This is Number.toLocaleString misbehaving. There's nothing I can do about that, like for 0.0001 vs. 0.001.
(Assignee)

Updated

3 years ago
Attachment #8445222 - Flags: review?(bgrinstead)
(Assignee)

Comment 6

3 years ago
Created attachment 8445262 [details] [diff] [review]
v2

Addressed comments from the discussion on IRC.
Attachment #8445222 - Attachment is obsolete: true
Attachment #8445222 - Flags: review?(bgrinstead)
Attachment #8445262 - Flags: review?(bgrinstead)
Comment on attachment 8445262 [details] [diff] [review]
v2

Review of attachment 8445262 [details] [diff] [review]:
-----------------------------------------------------------------

Changes LGTM
Attachment #8445262 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/bb723b1668ec
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bb723b1668ec
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.