CSS numbers should round to 6 decimal digits when serializing
Created attachment 434026 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment on attachment 434026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434026&action=review > Source/WebCore/ChangeLog:8 > + This seems to be the behavior of Chrome and Firefox, as shown by several WPT tests. If this isn't specified, it should be. Please make sure file a GitHub issue if it is not. > Source/WebCore/css/CSSPrimitiveValue.cpp:927 > + return makeString(round(m_value.num * 1000000) / 1000000, suffix); The usual way we do this is using the FormattedNumber::fixedWidth() adaptor, `makeString(FormattedNumber::fixedWidth(m_value.num, 6), suffix)`. Does that give incorrect results here?
Comment on attachment 434026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434026&action=review >> Source/WebCore/ChangeLog:8 >> + This seems to be the behavior of Chrome and Firefox, as shown by several WPT tests. > > If this isn't specified, it should be. Please make sure file a GitHub issue if it is not. It is specified, please add the spec link: https://www.w3.org/TR/cssom-1/#serialize-a-css-component-value <number> section: """ <number> 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. """
I’ll try fixedWidth instead (if it rounds instead of truncating) and update some more tests and add a spec link in the change log. Thanks, Sam and Chris!
fixedPrecision, not fixedWidth
The spec also says this: "Note: scientific notation is not used." We do and Chrome does. I'm going to not change that in this patch.
Created attachment 434052 [details] Patch
It turns out fixedWidth is closer to what we need, but we need trailing 0's removed. This is quite similar to the approach in bug 218880 but I keep the scientific notation for large values, and I raised an issue on the css spec. This should probably not be done before our next branch, so I added (later) to the bug title.
Comment on attachment 434052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434052&action=review > Source/WebCore/css/CSSPrimitiveValue.cpp:931 > + constexpr double smallestDoubleNeedingScientificNotation = 1e21; This threshold might not be correct. Might need to be 1000000 or 100000000. Will write more tests after getting feedback on my css spec issue.
Created attachment 434058 [details] Patch
Comment on attachment 434058 [details] Patch Looks like there are still some edge cases with negative zero that aren't quite right and a few tests that still need updating.
Comment on attachment 434058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434058&action=review > Source/WTF/ChangeLog:3 > + CSS numbers should round to 6 decimal digits when serializing Having this fixed maximum of 6 is unfortunate. I understand the specification calls for it. In practice, the number of digits should automatically be limited by the internal precision of floating point numbers. The serializer code knows not to write out more digits than is required to round-trip a floating point value. But what happens in these cases is that by mixing float and double we confuse that machinery and end up writing out too many digits. I suspect that if we straightened that out so the floats weren’t converted to doubles, we would get the correct results without adding an explicit 6-digit limit. But if the fixed 6-digit limit helps us match the other browsers and gets all the edge cases right, then I suppose we can live with it. Stripping trailing zeroes but using a 6-digit maximum limit can result in longer serializations than the "smallest length needed for a round trip" rule for some numeric values; at some point we should find some of those values and add them as test cases. > Source/WebCore/css/CSSPrimitiveValue.cpp:937 > + return makeString(needsScientificNotation ? FormattedNumber::fixedPrecision(m_value.num) : FormattedNumber::fixedWidth(m_value.num, 6, WTF::TruncateTrailingZeros), suffix); This logic is peculiar and also not in the best possible place. The concept of using "fixed precision" when scientific notation is needed is a kind of hack that I would like to see us eventually eliminate, but also fits more logically in the serializer itself, an object like FormattedNumber that can be used directly in makeString. Having correct code for this only in a function that returns a String means we cannot do this efficiently as part of a larger string-building process; always requiring allocation. I’d like this to be pushed down lower in the formatting machinery. I can do it later as refactoring if you choose not to do it right now. Can add a function called formattedForCSS() that we can use where we would otherwise use FormattedNumber::fixedWidth(). Can put it in any header we like or even in this .cpp file for now. The WTF prefix here is only needed because WTFString.h has: using WTF::KeepTrailingZeros; But does not have: using WTF::TruncateTrailingZeros; The reason for that is that until now, TruncateTrailingZeros was always the default anywhere this was used, so it typically was a programming mistake to use TruncateTrailingZeros explicitly. Now that it’s not the default, in this new place, we should add the "using" to WTFString.h.
<rdar://problem/81284013>
My current patch is certainly not a correct or elegant implementation of anything good, but the spec is unclear here and implementations aren't consistent. I think other browsers seem to have decided that 6 digits past the decimal are enough and we don't care about scientific notation with negative exponents, which we're just going to call 0. I think it's strange that 1.123456 and 1000000.123456 are quite different numbers of significant digits, but that seems to be what the spec is suggesting right now.
Created attachment 438172 [details] Patch
Comment on attachment 438172 [details] Patch r- because large integer values need to round-trip.
CSS WG resolved on scientific notation for <number> serialization, and decimal serialization for <integer>, which is basically https://bugs.webkit.org/attachment.cgi?id=437971
https://github.com/w3c/csswg-drafts/issues/6471#issuecomment-937325105