Bug 218880

Created attachment 413990 [details] WIP

Created attachment 414000 [details] WIPv2

Created attachment 414187 [details] Patch

Created attachment 414189 [details] Patch

Created attachment 414193 [details] Patch

Created attachment 414194 [details] Patch

Created attachment 414235 [details] Patch

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 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.

Some other aspects of this might require new functions, like making sure there are no exponents.

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 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?

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.

<rdar://problem/71610715>

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.

Created attachment 437971 [details] Possible alternative approach

Created attachment 444718 [details] Patch

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 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.

(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.

Created attachment 444776 [details] Patch

Created attachment 444933 [details] Patch

Created attachment 444977 [details] Patch

Created attachment 444985 [details] Patch

Created attachment 444994 [details] Patch

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[].

Created attachment 445324 [details] Patch

Created attachment 448822 [details] Patch

Created attachment 448829 [details] Patch

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).

Created attachment 448889 [details] Patch

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].

This regressed some WPT tests, notably causing bug 235819.