cubic-bezier() computed style serialization outputs locale-dependent floats (test_value_storage.html fails in some locales)

RESOLVED FIXED in mozilla8

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: arno renevier, Assigned: arno renevier)

Tracking

Trunk
mozilla8
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Hi,
I'm using french locale, and have 94 failing tests in test_value_storage.html, all related to -moz-animation-timing-function.
Example of a failing test:
failed | parse+compute+serialize(-moz-animation-timing-function) should be idempotent for '-moz-animation: bounce 1s linear 2s' - got "cubic-bezier(0,250000, 0,100000, 0,250000, 1,000000)", expected "cubic-bezier(0,000000, 0,000000, 1,000000, 1,000000)"

This happens because getComputedStyle uses printf with floats 
http://hg.mozilla.org/mozilla-central/file/320e5abc6f5a/layout/style/nsComputedDOMStyle.cpp#l3986
This gives me, with my locale, computed -moz-animation-timing-function which look like:

cubic-bezier(0,250000, 0,100000, 0,250000, 1,000000)

Which is wrong (notice the comma instead of dots for decimal separators).
Yeah, that's just wrong.  Arno, do you want to fix, or should I?
Summary: test_value_storage.html fails in some locales → cubic-bezier() computed style serialization outputs locale-dependent floats (test_value_storage.html fails in some locales)
(Assignee)

Comment 2

6 years ago
Created attachment 543919 [details] [diff] [review]
patch v1

I'm unsure if this is the right way. Should I instead reset LC_NUMERIC locale before calling nsPrintfCString (and set it back after) ?
Assignee: nobody → arno

Comment 3

6 years ago
You could use nsAutoString and AppendPrintf e.g. AppendPrintf("%f,%f", x, y) to build up a string. Or alternatively nsTextFormatter::ssprintf e.g.

    nsTextFormatter::ssprintf(aValue,
                              NS_LITERAL_STRING("%g,%g").get(),
                              x, y);
Or just use AppendFloat().

You do NOT want to mess with LC_NUMERIC.
(Assignee)

Comment 5

6 years ago
Created attachment 543970 [details] [diff] [review]
patch v2
Attachment #543919 - Attachment is obsolete: true
Attachment #543970 - Flags: review?(bzbarsky)
Comment on attachment 543970 [details] [diff] [review]
patch v2

r=me, but please add a useful commit message!
Attachment #543970 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 544162 [details] [diff] [review]
reviewed patch with a commit message
Attachment #543970 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
That commit message needs to list the bug number (and reviewer, typically).

I recommend: "Bug 669220.  Use location-independent float-to-string conversions for timing function computed styles.  r=bzbarsky".
(Assignee)

Comment 9

6 years ago
Created attachment 544262 [details] [diff] [review]
reviewed patch with a commit message (2)
Attachment #544162 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Created attachment 544264 [details] [diff] [review]
reviewed patch with a commit message (2)
Attachment #544262 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9b83b5cfcab

(I changed "location-independent" in the commit message to "locale-independent".)
http://hg.mozilla.org/mozilla-central/rev/a9b83b5cfcab
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8

Updated

6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.