Bug 218880 - Serialize CSS <number> values with rounding, limited decimal precision, and no exponents per-spec
Summary: Serialize CSS <number> values with rounding, limited decimal precision, and n...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar, WPTImpact
Depends on:
Blocks:
 
Reported: 2020-11-12 18:13 PST by Tyler Wilcock
Modified: 2022-01-11 20:10 PST (History)
19 users (show)

See Also:


Attachments
WIP (277.92 KB, patch)
2020-11-12 18:38 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIPv2 (327.80 KB, patch)
2020-11-12 20:21 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (336.03 KB, patch)
2020-11-15 18:10 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (335.84 KB, patch)
2020-11-15 18:49 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (338.64 KB, patch)
2020-11-15 21:15 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (338.64 KB, patch)
2020-11-15 22:15 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (339.34 KB, patch)
2020-11-16 07:35 PST, Tyler Wilcock
darin: review+
darin: commit-queue-
Details | Formatted Diff | Diff
Possible alternative approach (339.27 KB, patch)
2021-09-11 16:51 PDT, Simon Fraser (smfr)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (182.42 KB, patch)
2021-11-18 12:34 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (243.75 KB, patch)
2021-11-18 20:14 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (308.40 KB, patch)
2021-11-21 15:36 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (323.87 KB, patch)
2021-11-22 10:59 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (322.98 KB, patch)
2021-11-22 14:46 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (328.51 KB, patch)
2021-11-22 17:56 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (329.13 KB, patch)
2021-11-29 12:14 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (280.06 KB, patch)
2022-01-10 18:05 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (289.39 KB, patch)
2022-01-10 20:39 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (289.94 KB, patch)
2022-01-11 16:31 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2020-11-12 18:13:54 PST
https://drafts.csswg.org/cssom/#serialize-a-css-component-value

> A base-ten number using digits 0-9 (U+0030 to U+0039) in the shortest form possible, using "." to separate decimals (if any), rounding the value if necessary to not produce more than 6 decimals, preceded by "-" (U+002D) if it is negative.
> Note: scientific notation is not used.

Currently, WebKit serializes <number>s with more than 6 decimals, and uses exponents (scientific notation).  For example:
  *  [1] (PASS ... "hue-rotate(calc(8594.366926962348deg))")
  *  [2] (PASS ... should serialize as 'matrix(6.123233995736766e-17, 1, -1, 6.123233995736766e-17, 0, 0)')
  *  [3] (PASS ... fontSize is "9.600000381469727px")

Comparing with Chromium and Gecko:

[1] WebKit:   8594.366926962348deg
[1] Chromium: 8594.37deg        
[1] Gecko:    8594.37deg         

[2] WebKit:   matrix(6.123233995736766e-17, 1, -1, 6.123233995736766e-17, 0, 0)
[2] Chromium: matrix(6.12323e-17, 1, -1, 6.12323e-17, 0, 0)
[2] Gecko:    matrix(0, 1, -1, 0, 0, 0)

[3] WebKit:   9.600000381469727px
[3] Chromium: 9.6px
[3] Gecko:    9.6px

[1]: https://github.com/WebKit/webkit/blob/817c46e152af795d735678386db68805d0aa505e/LayoutTests/fast/css/calc-with-angle-time-frequency-expected.txt#L10
[2]: https://github.com/WebKit/webkit/blob/8b173fd921c96eedcbceab8d1fec14d12afaab8b/LayoutTests/imported/w3c/web-platform-tests/css/css-values/minmax-angle-serialize-expected.txt
[3]: https://github.com/WebKit/webkit/blob/817c46e152af795d735678386db68805d0aa505e/LayoutTests/css3/calc/font-size-fractional-expected.txt#L7
Comment 1 Tyler Wilcock 2020-11-12 18:38:56 PST
Created attachment 413990 [details]
WIP
Comment 2 Tyler Wilcock 2020-11-12 20:21:44 PST
Created attachment 414000 [details]
WIPv2
Comment 3 Tyler Wilcock 2020-11-15 18:10:10 PST
Created attachment 414187 [details]
Patch
Comment 4 Tyler Wilcock 2020-11-15 18:49:26 PST
Created attachment 414189 [details]
Patch
Comment 5 Tyler Wilcock 2020-11-15 21:15:43 PST
Created attachment 414193 [details]
Patch
Comment 6 Tyler Wilcock 2020-11-15 22:15:41 PST
Created attachment 414194 [details]
Patch
Comment 7 Tyler Wilcock 2020-11-16 07:35:58 PST
Created attachment 414235 [details]
Patch
Comment 8 Tyler Wilcock 2020-11-16 10:19:11 PST
OK, this is ready for a look.  This implementation changes serialization of <number> `CSSPrimitiveValue`s to be formatted at a fixed decimal width.  `*toFixedWidth` in WTFString and `dtoa.{h, cpp}` have been updated to allow truncation of trailing zeros to match the `*toFixedPrecision` variants of these methods (shown in below link).

https://github.com/WebKit/webkit/blob/817c46e152af795d735678386db68805d0aa505e/Source/WTF/wtf/dtoa.cpp#L95#L96
Comment 9 Darin Adler 2020-11-16 10:36:00 PST
Comment on attachment 414235 [details]
Patch

Thank you for tackling this!

While it’s great to have all these progressions, this takes the wrong basic approach. Fixed-width serialization that truncates trailing zeros is *not* shortest form serialization. Shortest form serialization is String::number(double) and String::number(float), not String::numberToStringFixedWidth.

The problems with serialization come from the fact that things are processed as float, then converted to double, then serialized with shortest form. The way to do this correctly for things that are float is to keep them as float, and not convert to double before serializing.
Comment 10 Darin Adler 2020-11-16 10:36:44 PST
Some other aspects of this might require new functions, like making sure there are no exponents.
Comment 11 Darin Adler 2020-11-16 10:37:55 PST
OK, perhaps I jumped to conclusions. If the specification says "rounding the value if necessary to not produce more than 6 decimals" then maybe that is the same thing as fixed precision.

Feel free to set review? again since I’m not sure about my review- and would be happy to have someone else review this (or review it myself) later.
Comment 12 Darin Adler 2020-11-16 12:02:35 PST
Comment on attachment 414235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414235&action=review

I think this is a step in the right direction.

At least some of these are also showing wrong results that seem to be due to unwanted mixing of float and double (for example, the string "200.7" turning into "200.699997px", but that's in a test that is supposed to forbid fractional values at all), but that’s a separate issue. This does seem to match the specification language, but I’m not sure there’s enough test coverage of edge cases for this, even in WPT. I’d like to see more test cases.

However, this implementation is subtly wrong, so please use FormattedNumber in the version you land.

> Source/WTF/wtf/dtoa.cpp:123
> +        // If we've truncated the trailing zeros and a trailing decimal, we may have a -0. Remove the negative sign in this case.
> +        if (builder.position() == 2 && buffer[0] == '-' && buffer[1] == '0')
> +            builder.RemoveCharacters(0, 1);

What about the case where the value *is* negative zero?

> Source/WTF/wtf/text/WTFString.cpp:510
> +String String::numberToStringFixedWidth(double number, unsigned decimalPlaces, TrailingZerosTruncatingPolicy trailingZerosTruncatingPolicy)

I think you could use a shorter name for this local, since its scope is really small and there is no ambiguity. Maybe just "policy" in fact.

> Source/WebCore/css/CSSPrimitiveValue.cpp:909
> +    return makeString(String::numberToStringFixedWidth(m_value.num, 6, TruncateTrailingZeros), suffix);

Instead of String::numberToStringFixedWidth, this should be using FormattedNumber::fixedWidth. That is more efficient and avoids allocating memory every time.

That’s going to mean that instead of (or in addition to) extending WTFString.h, we will want to extend StringConcatenateNumbers.h to add the truncate trailing zeroes feature to FormattedNumber::fixedWidth.

> LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/animation/flex-shrink-interpolation-expected.txt:22
> -FAIL Web Animations: property <flex-shrink> from neutral to [2] at (0.3) should be [1.65] assert_equals: expected "1.65 " but got "1.55 "
> +FAIL Web Animations: property <flex-shrink> from neutral to [2] at (0.3) should be [1.65] assert_equals: expected "1.65 " but got "1.54 "

What explains this change and the others like it, where the second digit after the decimal point changes?
Comment 13 Tyler Wilcock 2020-11-16 13:19:18 PST
Thanks for the review, Darin!

> This does seem to match the specification language, but I’m not sure there’s enough test
> coverage of edge cases for this, even in WPT. I’d like to see more test
> cases.
OK, I'll craft some more testcases.
 
> > Source/WTF/wtf/dtoa.cpp:123
> > +        // If we've truncated the trailing zeros and a trailing decimal, we may have a -0. Remove the negative sign in this case.
> > +        if (builder.position() == 2 && buffer[0] == '-' && buffer[1] == '0')
> > +            builder.RemoveCharacters(0, 1);
> 
> What about the case where the value *is* negative zero?
That is an interesting question.  Doing a bit of spec-lunking: 

https://www.w3.org/TR/css-values-4

> Additionally, IEEE-754 introduces the concept of "negative zero", which must be tracked within a calculation and between nested calculations...
> Note: Note that negative zeros don’t escape a math function; as detailed below, they’re "censored" away into an "unsigned" zero.
This seems to suggest negative zeros are disallowed outside math function context.  And given this:

<style> #target { font-size: -0px; } </style>
<div id="target">abc</div>
<script>
    let target = document.getElementById('target')
    let result = document.createTextNode(
        `getComputedStyle font-size: ${window.getComputedStyle(target)['font-size']}`
    )
    document.body.appendChild(result)
</script>

Chromium, WebKit, and Gecko currently serialize a 0px font-size.

In contrast, negative zero is technically valid according to the definition of <integer> (https://drafts.csswg.org/cssom/#ref-for-integer-value)

> A base-ten integer using digits 0-9 (U+0030 to U+0039) in the shortest form possible, preceded by "-" (U+002D) if it is negative.
MDN (while not authoritative) also explicitly states negative zero is valid (https://developer.mozilla.org/en-US/docs/Web/CSS/integer).

And as another data point, CSS2 states negative zero is the same as zero, and not actually negative.

https://www.w3.org/TR/CSS2/syndata.html#numbers

Based on my current implementation, allowing negative zero will result in some web animation tests serializing with negative zeros.  For example, this one:

https://github.com/WebKit/webkit/blob/4b9823872aa4c74dac7080e5ebf6c86b54bb0f95/LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/matrix-interpolation-expected.txt

May look something like:

...but got matrix3d ( 1 , 0 , - 0 , - 0 , 0 , 1, ... )

However, since -0 seems to be valid, maybe it's OK.  I can try removing the aforementioned code.

> > Source/WTF/wtf/text/WTFString.cpp:510
> > +String String::numberToStringFixedWidth(double number, unsigned decimalPlaces, TrailingZerosTruncatingPolicy trailingZerosTruncatingPolicy)
> 
> I think you could use a shorter name for this local, since its scope is
> really small and there is no ambiguity. Maybe just "policy" in fact.
Agreed.
 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:909
> > +    return makeString(String::numberToStringFixedWidth(m_value.num, 6, TruncateTrailingZeros), suffix);
> 
> Instead of String::numberToStringFixedWidth, this should be using
> FormattedNumber::fixedWidth. That is more efficient and avoids allocating
> memory every time. 
Really helpful, thanks — will do this.

> > LayoutTests/imported/w3c/web-platform-tests/css/css-flexbox/animation/flex-shrink-interpolation-expected.txt:22
> > -FAIL Web Animations: property <flex-shrink> from neutral to [2] at (0.3) should be [1.65] assert_equals: expected "1.65 " but got "1.55 "
> > +FAIL Web Animations: property <flex-shrink> from neutral to [2] at (0.3) should be [1.65] assert_equals: expected "1.65 " but got "1.54 "
> 
> What explains this change and the others like it, where the second digit
> after the decimal point changes?
That is a good question.  Let me look more into these.
Comment 14 Radar WebKit Bug Importer 2020-11-19 18:14:15 PST
<rdar://problem/71610715>
Comment 15 Simon Fraser (smfr) 2021-09-11 16:48:35 PDT
I just spent an entire day writing basically the same patch. Can this one land?

However, I did take a slightly different tack. I allowed exponential notation in serialized values for <number> properties, which matches the parsing rules (https://drafts.csswg.org/css-values/#numbers). <integer> values serialize as they do now.

This required adding a CSS_INTEGER CSSUnitType so that CSSPrimitiveValues are now CSS_INTEGER or CSS_NUMBER. This does add a bit of complexity, but brings clarity to <integer> vs <number> in various places.
Comment 16 Simon Fraser (smfr) 2021-09-11 16:51:28 PDT
Created attachment 437971 [details]
Possible alternative approach
Comment 17 Matt Woodrow 2021-11-18 12:34:57 PST
Created attachment 444718 [details]
Patch
Comment 18 Darin Adler 2021-11-18 14:51:24 PST
Comment on attachment 444718 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444718&action=review

> Source/WTF/wtf/dtoa.h:40
> +WTF_EXPORT_PRIVATE const char* numberToFixedWidthString(double, unsigned decimalPlaces, NumberToStringBuffer&, bool truncateTrailingZeros = false, const double_conversion::DoubleToStringConverter& converter = double_conversion::DoubleToStringConverter::EcmaScriptConverter());

Taking a DoubleToStringConverter here seems to be a layering violation. If we wanted to use double_conversion we could just use it directly! I suggest instead adding some other kind of option, maybe an enumeration or a boolean.
Comment 19 Darin Adler 2021-11-18 14:52:18 PST
Comment on attachment 444718 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444718&action=review

>> Source/WTF/wtf/dtoa.h:40
>> +WTF_EXPORT_PRIVATE const char* numberToFixedWidthString(double, unsigned decimalPlaces, NumberToStringBuffer&, bool truncateTrailingZeros = false, const double_conversion::DoubleToStringConverter& converter = double_conversion::DoubleToStringConverter::EcmaScriptConverter());
> 
> Taking a DoubleToStringConverter here seems to be a layering violation. If we wanted to use double_conversion we could just use it directly! I suggest instead adding some other kind of option, maybe an enumeration or a boolean.

Or a separate function, just for the CSS flavor.
Comment 20 Matt Woodrow 2021-11-18 19:41:42 PST
(In reply to Darin Adler from comment #18)
> Comment on attachment 444718 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444718&action=review
> 
> > Source/WTF/wtf/dtoa.h:40
> > +WTF_EXPORT_PRIVATE const char* numberToFixedWidthString(double, unsigned decimalPlaces, NumberToStringBuffer&, bool truncateTrailingZeros = false, const double_conversion::DoubleToStringConverter& converter = double_conversion::DoubleToStringConverter::EcmaScriptConverter());
> 
> Taking a DoubleToStringConverter here seems to be a layering violation. If
> we wanted to use double_conversion we could just use it directly! I suggest
> instead adding some other kind of option, maybe an enumeration or a boolean.

Yeah agreed, I'll add a separate function for the CSS variant. Mostly just wanted EWS coverage to figure out which other tests I needed to be debugging locally.

It looks like DoubleToStringConverter::ToFixed caps the number of digits before a decimal place to 21 (changed in bug 199163, to match what the JS spec needs). The CSS spec doesn't mention any limit, and we have at least one test that expect 1E27 to serialise back correctly.
Comment 21 Matt Woodrow 2021-11-18 20:14:29 PST
Created attachment 444776 [details]
Patch
Comment 22 Matt Woodrow 2021-11-21 15:36:19 PST
Created attachment 444933 [details]
Patch
Comment 23 Matt Woodrow 2021-11-22 10:59:47 PST
Created attachment 444977 [details]
Patch
Comment 24 Matt Woodrow 2021-11-22 14:46:12 PST
Created attachment 444985 [details]
Patch
Comment 25 Matt Woodrow 2021-11-22 17:56:47 PST
Created attachment 444994 [details]
Patch
Comment 26 Darin Adler 2021-11-29 10:56:49 PST
Comment on attachment 444994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444994&action=review

> Source/WTF/wtf/dtoa.cpp:42
> +template<std::size_t N>
> +static inline void truncateTrailingZeros(std::array<char, N>& buffer, double_conversion::StringBuilder& builder)

Seems wasteful to make this a template function; we do not want risk ending up with two separate copies. Instead I suggest we change the buffer argument to be a char* or char[].
Comment 27 Matt Woodrow 2021-11-29 12:14:53 PST
Created attachment 445324 [details]
Patch
Comment 28 Matt Woodrow 2022-01-10 18:05:19 PST
Created attachment 448822 [details]
Patch
Comment 29 Matt Woodrow 2022-01-10 20:39:11 PST
Created attachment 448829 [details]
Patch
Comment 30 Simon Fraser (smfr) 2022-01-11 11:58:57 PST
Comment on attachment 448829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448829&action=review

> Source/WTF/ChangeLog:8
> +
> +        * wtf/dtoa.cpp:

You should write some words here about why WTF changes were needed.

> Source/WebCore/ChangeLog:7
> +        Reviewed by Darin Adler.
> +

Words here please (the "why" and spec links).
Comment 31 Matt Woodrow 2022-01-11 16:31:20 PST
Created attachment 448889 [details]
Patch
Comment 32 EWS 2022-01-11 20:10:00 PST
Committed r287909 (245942@main): <https://commits.webkit.org/245942@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448889 [details].