Closed Bug 1029540 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 1 obsolete file)

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".
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8445222 - Flags: review?(bgrinstead)
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
This is Number.toLocaleString misbehaving. There's nothing I can do about that, like for 0.0001 vs. 0.001.
Attached patch v2Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/bb723b1668ec
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.