Currently when a large number (outside of the supported range) is applied from a style rule or set from javascript using Element.style we check if it is within the supported range and set it to zero if it is not. This is incorrect according to the CSS3 specification which instead mandates that the value be ignored. This patch changes the behavior to ignore out-of-range values. "Properties may restrict numeric values to some range. If the value is outside the allowed range, the declaration is invalid and must be ignored." - http://www.w3.org/TR/css3-values/#numeric-types
do you have specific properties in mind, or is this a meta-requirement
(In reply to comment #1) > do you have specific properties in mind, or is this a meta-requirement Length properties, specifically width, height, left and top as these have been causing some problems with a couple of google properties. Downstream chromium bug: http://code.google.com/p/chromium/issues/detail?id=145798
Created attachment 175081 [details] Patch
Comment on attachment 175081 [details] Patch lgtm fwiw
Comment on attachment 175081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175081&action=review Looms good tome, too. Please correct my minor nit on the a/an when landing. R=me. > Source/WebCore/ChangeLog:26 > + Return an Length with the type set to Undefined for unsupported Nit: return *a* length with ... > Source/WebCore/css/CSSPrimitiveValue.cpp:-483 > - return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed); Based on the naming of these limits, it almost seems like these bounds should be inclusive. However, they have obviously not been used this way historically... > Source/WebCore/css/CSSPrimitiveValue.cpp:483 > + if (value > intMaxForLayoutUnit || value < intMinForLayoutUnit) Whoops! They are inclusive. Never mind! :)
(In reply to comment #5) > (From update of attachment 175081 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175081&action=review > > Looms good tome, too. > > Please correct my minor nit on the a/an when landing. Thanks for the review! I'll wait until tomorrow to commit it to give others a chance to comment further or object. > > Source/WebCore/ChangeLog:26 > > + Return an Length with the type set to Undefined for unsupported > > Nit: return *a* length with ... Good catch, thanks! > Whoops! They are inclusive. Never mind! :) I tried to make it clearer by reversing the check.
Comment on attachment 175081 [details] Patch one more thought... it might be worth generating a warning in the inspector log when an out of bounds value is ignored, but that could be a follow-up patch
(In reply to comment #7) > (From update of attachment 175081 [details]) > one more thought... it might be worth generating a warning in the inspector log when an out of bounds value is ignored, but that could be a follow-up patch That is not a bad idea. Filed bug 102750 to track it.
(In reply to comment #0) > Currently when a large number (outside of the supported range) is applied from a style rule or set from javascript using Element.style we check if it is within the supported range and set it to zero if it is not. This is incorrect according to the CSS3 specification which instead mandates that the value be ignored. This patch changes the behavior to ignore out-of-range values. > > "Properties may restrict numeric values to some range. If the value is outside the allowed range, the declaration is invalid and must be ignored." - http://www.w3.org/TR/css3-values/#numeric-types You're reading this slightly wrong. *Properties* can restrict their range, which should cause rejection at parse-time if you violate that range. Authors can know ahead of time what the allowed range is, and violating that range generally means you're supplying a nonsensical value. The Chromium bug in question is related to *implementation-defined* limits on the allowed range. The behavior here is undefined by CSS. (We wanted to define it in Values & Units 3, but had to punt it to get the spec to CR. It'll reappear in level 4.) The most author-friendly behavior is to clamp at the limit of our range. That gives the author something as close as we can deliver to what they asked for. Rejecting the rule at parse-time is author-hostile, as authors have no way of predicting how large is "too large", since it's impl-defined and can change over time, and predicting wrongly will cause properties to be ignored in a seemingly random fashion, perhaps in a non-interoperable way.
(In reply to comment #9) > (In reply to comment #0) > > Currently when a large number (outside of the supported range) is applied from a style rule or set from javascript using Element.style we check if it is within the supported range and set it to zero if it is not. This is incorrect according to the CSS3 specification which instead mandates that the value be ignored. This patch changes the behavior to ignore out-of-range values. > > > > "Properties may restrict numeric values to some range. If the value is outside the allowed range, the declaration is invalid and must be ignored." - http://www.w3.org/TR/css3-values/#numeric-types > > You're reading this slightly wrong. *Properties* can restrict their range, which should cause rejection at parse-time if you violate that range. Authors can know ahead of time what the allowed range is, and violating that range generally means you're supplying a nonsensical value. > > The Chromium bug in question is related to *implementation-defined* limits on the allowed range. The behavior here is undefined by CSS. (We wanted to define it in Values & Units 3, but had to punt it to get the spec to CR. It'll reappear in level 4.) The most author-friendly behavior is to clamp at the limit of our range. That gives the author something as close as we can deliver to what they asked for. Thanks for clarifying, it would be really helpful if we could add that back to the next version of the spec. > Rejecting the rule at parse-time is author-hostile, as authors have no way of predicting how large is "too large", since it's impl-defined and can change over time, and predicting wrongly will cause properties to be ignored in a seemingly random fashion, perhaps in a non-interoperable way. Ok, I'll change it to clamp values instead (which matches the gecko behavior).
(In reply to comment #10) > Ok, I'll change it to clamp values instead (which matches the gecko behavior). Thanks, Emil! That'll make it a lot easier to specify in the next go-round of the spec, too.
Created attachment 175260 [details] Patch
Made the changes suggested by Tab. Please take another look (cleared the r+ as this is effectively a different change now).
Just an FYI, but CSS specs are not presently consistent regarding clamping versus ignoring. For example, CSS3 Animation requires a keyframe declaration outside of 0-100% to be ignored and not merely clamped to 0 or 100. See https://bugs.webkit.org/show_bug.cgi?id=96844.
(In reply to comment #14) > Just an FYI, but CSS specs are not presently consistent regarding clamping versus ignoring. For example, CSS3 Animation requires a keyframe declaration outside of 0-100% to be ignored and not merely clamped to 0 or 100. See https://bugs.webkit.org/show_bug.cgi?id=96844. Again, as I explained, the rule is that *language-defined limits* cause things to be ignored. It's a syntax error, since you know about it beforehand and everyone is the same (barring bugs). We clamp when we encounter *implementation-defined limits*, because they're unspecified and variable across time and browsers, so there's no way for authors to robustly protect themselves from violating the limit. The behavior of keyframes is consistent with this. (If you do know of any inconsistencies, please report them on www-style so we can try to fix them!)
Comment on attachment 175260 [details] Patch Attachment 175260 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14907712 New failing tests: css3/flexbox/flex-algorithm.html
Comment on attachment 175260 [details] Patch Attachment 175260 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14907720 New failing tests: css3/flexbox/flex-algorithm.html
Created attachment 175283 [details] Patch
Comment on attachment 175283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175283&action=review > LayoutTests/css3/flexbox/flex-algorithm.html:133 > - <div data-expected-width="600" style="-webkit-flex: 100000000000000000000000000000000000000 0 600px; -moz-flex: 100000000000000000000000000000000000000 0 600px"></div> > + <div data-expected-width="0" style="-webkit-flex: 100000000000000000000000000000000000000 0 600px; -moz-flex: 100000000000000000000000000000000000000 0 600px"></div> > <div data-expected-width="600" style="-webkit-flex: 0 100000000000000000000000000000000000000 600px; -moz-flex: 0 100000000000000000000000000000000000000 600px"></div> > - <div data-expected-width="0" style="-webkit-flex: 1 1 100000000000000000000000000000000000000px; -moz-flex: 1 1 100000000000000000000000000000000000000px"></div> > + <div data-expected-width="33554428" style="-webkit-flex: 1 1 100000000000000000000000000000000000000px; -moz-flex: 1 1 100000000000000000000000000000000000000px"></div> FWIW, this change seems fine to me. The test is just making sure that we don't crash or infinite loop when there are really big input values.
(In reply to comment #19) > (From update of attachment 175283 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175283&action=review > > > LayoutTests/css3/flexbox/flex-algorithm.html:133 > > - <div data-expected-width="600" style="-webkit-flex: 100000000000000000000000000000000000000 0 600px; -moz-flex: 100000000000000000000000000000000000000 0 600px"></div> > > + <div data-expected-width="0" style="-webkit-flex: 100000000000000000000000000000000000000 0 600px; -moz-flex: 100000000000000000000000000000000000000 0 600px"></div> > > <div data-expected-width="600" style="-webkit-flex: 0 100000000000000000000000000000000000000 600px; -moz-flex: 0 100000000000000000000000000000000000000 600px"></div> > > - <div data-expected-width="0" style="-webkit-flex: 1 1 100000000000000000000000000000000000000px; -moz-flex: 1 1 100000000000000000000000000000000000000px"></div> > > + <div data-expected-width="33554428" style="-webkit-flex: 1 1 100000000000000000000000000000000000000px; -moz-flex: 1 1 100000000000000000000000000000000000000px"></div> > > FWIW, this change seems fine to me. The test is just making sure that we don't crash or infinite loop when there are really big input values. Ah, was just about to ask you to validate that. Thanks! I tested with other large (but in-range) numbers and got the same results/behavior.
Comment on attachment 175283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175283&action=review The conversion to zero seems arbitrary and confusing, it looks like a good idea to clamp it. I have a couple of questions: > Source/WebCore/css/CSSPrimitiveValue.cpp:485 > double value = computeLengthDouble(style, rootStyle, multiplier, computingFontSize); > - return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed); > + return Length(clampToLayoutUnitRange(value), Fixed); > #else > return Length(roundForImpreciseConversion<float>(computeLengthDouble(style, rootStyle, multiplier, computingFontSize)), Fixed); Why is the Length clamped here and not when the conversion double->int occurs? To avoid fragmentation, could we have the exact same clamping rules with and without ENABLE(SUBPIXEL_LAYOUT)? > Source/WebCore/platform/LayoutUnit.h:869 > + // Adjust values somewhat to allow for float imprecision and rounding without overflowing. > + return clampTo<float>(value, intMinForLayoutUnit + 2, intMaxForLayoutUnit - 2); Can you please explain why you use the value 2 here?
(In reply to comment #21) > (From update of attachment 175283 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175283&action=review > > The conversion to zero seems arbitrary and confusing, it looks like a good idea to clamp it. I have a couple of questions: > > > Source/WebCore/css/CSSPrimitiveValue.cpp:485 > > double value = computeLengthDouble(style, rootStyle, multiplier, computingFontSize); > > - return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed); > > + return Length(clampToLayoutUnitRange(value), Fixed); > > #else > > return Length(roundForImpreciseConversion<float>(computeLengthDouble(style, rootStyle, multiplier, computingFontSize)), Fixed); > > Why is the Length clamped here and not when the conversion double->int occurs? We don't convert it to an int, it is represented as a float in Length and then covered to a LayoutUnit. > To avoid fragmentation, could we have the exact same clamping rules with and without ENABLE(SUBPIXEL_LAYOUT)? We could but then we'd have to hardcode the MIN/MAX values. That would probably be fine. > > > Source/WebCore/platform/LayoutUnit.h:869 > > + // Adjust values somewhat to allow for float imprecision and rounding without overflowing. > > + return clampTo<float>(value, intMinForLayoutUnit + 2, intMaxForLayoutUnit - 2); > > Can you please explain why you use the value 2 here? To ensure that we have a number that can be correctly represented as a float and having enough room for rounding without overflowing. Usig intMinForLayoutUnit and intMaxForLayoutUnit as is would overflow when rounding, adding/subtracting one and the values returned by computed style differs from the values returned by the offsetTop/Width/* properties due to float<->LayoutUnit conversions. I suppose just hardcoding the min/max values might be a better idea and would also allow the same behavior regardless of the ENABLE_SUBPIXEL flag.
> > > Source/WebCore/css/CSSPrimitiveValue.cpp:485 > > > double value = computeLengthDouble(style, rootStyle, multiplier, computingFontSize); > > > - return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed); > > > + return Length(clampToLayoutUnitRange(value), Fixed); > > > #else > > > return Length(roundForImpreciseConversion<float>(computeLengthDouble(style, rootStyle, multiplier, computingFontSize)), Fixed); > > > > Why is the Length clamped here and not when the conversion double->int occurs? > > We don't convert it to an int, it is represented as a float in Length and then covered to a LayoutUnit. I know...but if you look at what is LayoutUnit: an int ;) I believed the conversion in LayoutUnit was already clamping the input, but that's actually only true with SATURATED_LAYOUT_ARITHMETIC. This makes me believe the original code using intMaxForLayoutUnit && intMinForLayoutUnit may have been bogus to begin with, and SATURATED_LAYOUT_ARITHMETIC is the right solution. > > > Source/WebCore/platform/LayoutUnit.h:869 > > > + // Adjust values somewhat to allow for float imprecision and rounding without overflowing. > > > + return clampTo<float>(value, intMinForLayoutUnit + 2, intMaxForLayoutUnit - 2); > > > > Can you please explain why you use the value 2 here? > > To ensure that we have a number that can be correctly represented as a float and having enough room for rounding without overflowing. Usig intMinForLayoutUnit and intMaxForLayoutUnit as is would overflow when rounding, adding/subtracting one and the values returned by computed style differs from the values returned by the offsetTop/Width/* properties due to float<->LayoutUnit conversions. > > I suppose just hardcoding the min/max values might be a better idea and would also allow the same behavior regardless of the ENABLE_SUBPIXEL flag. Yep, that'd be great. I think the code must use the same path as float<->LayoutUnit in order to be more readable.
(In reply to comment #23) > > > > Source/WebCore/css/CSSPrimitiveValue.cpp:485 > > > > double value = computeLengthDouble(style, rootStyle, multiplier, computingFontSize); > > > > - return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed); > > > > + return Length(clampToLayoutUnitRange(value), Fixed); > > > > #else > > > > return Length(roundForImpreciseConversion<float>(computeLengthDouble(style, rootStyle, multiplier, computingFontSize)), Fixed); > > > > > > Why is the Length clamped here and not when the conversion double->int occurs? > > > > We don't convert it to an int, it is represented as a float in Length and then covered to a LayoutUnit. > > I know...but if you look at what is LayoutUnit: an int ;) > > I believed the conversion in LayoutUnit was already clamping the input, but that's actually only true with SATURATED_LAYOUT_ARITHMETIC. > This makes me believe the original code using intMaxForLayoutUnit && intMinForLayoutUnit may have been bogus to begin with, and SATURATED_LAYOUT_ARITHMETIC is the right solution. Right, I've been trying to turn on SATURATED_LAYOUT_ARITHMETIC for awhile but there are still some lingering performance concerns. Once (if) it has been turned on everywhere we can revisit this and remove the clampTo call here. > > > > > Source/WebCore/platform/LayoutUnit.h:869 > > > > + // Adjust values somewhat to allow for float imprecision and rounding without overflowing. > > > > + return clampTo<float>(value, intMinForLayoutUnit + 2, intMaxForLayoutUnit - 2); > > > > > > Can you please explain why you use the value 2 here? > > > > To ensure that we have a number that can be correctly represented as a float and having enough room for rounding without overflowing. Usig intMinForLayoutUnit and intMaxForLayoutUnit as is would overflow when rounding, adding/subtracting one and the values returned by computed style differs from the values returned by the offsetTop/Width/* properties due to float<->LayoutUnit conversions. > > > > I suppose just hardcoding the min/max values might be a better idea and would also allow the same behavior regardless of the ENABLE_SUBPIXEL flag. > > Yep, that'd be great. > > I think the code must use the same path as float<->LayoutUnit in order to be more readable. I changed both to use the same clampTo call. The non-subpixel path still needs do some some other rounding though.
Created attachment 175470 [details] Patch
Created attachment 176023 [details] Patch
Cleaned it up a bit and made sure both code paths (subpixel on/off) uses the same logic. Please take another look.
Please take another look when you get a chance, thanks!
>LayoutTests/platform/chromium/fast/css/large-number-round-trip-expected.txt >1 PASS: read 90010000px back as 0px, read again as 0px > 1PASS: read 90010000px back as 33554428px, read again as 33554428px I wonder if the above should also include a test for the large negative number clamp (-33554430)? >Source/WebCore/css/CSSPrimitiveValue.cpp >56 // Max/min values for CSS, needs to slightly smaller/larger than the true max/min values to allow for rounding without overflowing. > 57 const int maxValueForCssLength = 33554428; > 58 const int minValueForCssLength = -33554430; One wonders where these magic numbers come from. It might be nice to add something to the comment about how these are derived.
Created attachment 177023 [details] Patch
Made the changes you suggested, please take another look.
lgtm tnx!
Comment on attachment 177023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177023&action=review I had one suggestion about the two magic numbers you use, but otherwise it looks good to me. > Source/WebCore/css/CSSPrimitiveValue.cpp:59 > +const int minValueForCssLength = -33554430; If that is the case, why can't we reference LayoutUnit::nearlyMax or nearlyMin? Or if they aren't in scope for some reason, just inline them as: #if ENABLE(SUBPIXEL_LAYOUT) static const int kFixedPointDenominator = 64; #else static const int kFixedPointDenominator = 1; #endif const int maxValueForCssLength = std::numeric_limits<int>::max() - kFixedPointDenominator / 2; const int minValueForCssLength = std::numeric_limits<int>::min() + kFixedPointDenominator / 2; Instead of the magic number 33554428, we now have the kFixedPointDenominator of 1 or 64, which are easy to understand and defend.
(In reply to comment #33) > Instead of the magic number 33554428, we now have the kFixedPointDenominator of 1 or 64, which are easy to understand and defend. It is not quite that easy. I can't use nearlyMax/nearlyMin as that would result in different limits depending on the SUBPIXEL flag. Similarly I can't use "std::numeric_limits<int>::max() / 64 - 1" due to loosing precision when converting to a float and back. INT_MAX / 64 == 33554431 static_cast<int>(static_cast<float>(INT_MAX / 64) - 1) + 1 == 33554433 The largest value I've found that works is 33554429 (INT_MAX / 64 - 2). The cleanest solution would be to use (1 << 24) as the max value as it is guaranteed to have a stable representation but that would in effective reduce the the usable range of numbers by a factor of two.
Any suggestions as to which is preferable?
Simon, what do you think we should do here? I don't really want to hard-code the limits.
(In reply to comment #36) > Simon, what do you think we should do here? I don't really want to hard-code the limits. Can't you define it as (INT_MAX / kFixedPointDenominator - 2)? That seems close enough.
(In reply to comment #37) > (In reply to comment #36) > > Simon, what do you think we should do here? I don't really want to hard-code the limits. > > Can't you define it as (INT_MAX / kFixedPointDenominator - 2)? That seems close enough. Yeah, that's probably the best we can do. Thanks!
Created attachment 178284 [details] Patch
Comment on attachment 178284 [details] Patch Attachment 178284 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15191519
Created attachment 178603 [details] Patch
Brent/Simon, would either of you mind taking another look?
Comment on attachment 178603 [details] Patch Clearing flags on attachment: 178603 Committed r137365: <http://trac.webkit.org/changeset/137365>
All reviewed patches have been landed. Closing bug.