There is an issue when using a media query with a max-width condition greater than 999999999px: ``` @media (max-width: 999999999px) { * {background-color: red !important;} } // will put a red background on every element @media (max-width: 9999999999px) { * {background-color: red !important;} } // will NOT put a red background ``` See the reproduction here: https://codepen.io/mathieudutour/pen/eYgvjRJ
Created attachment 425067 [details] Patch
Comment on attachment 425067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425067&action=review > Source/WebCore/css/MediaQueryEvaluator.cpp:485 > -static bool computeLength(CSSValue* value, bool strict, const CSSToLengthConversionData& conversionData, int& result) > +static bool computeLength(CSSValue* value, bool strict, const CSSToLengthConversionData& conversionData, double& result) If we are modifying all the callers of this anyway, we should take the opportunity to return Optional<double> instead of using an out argument. That’s the future direction for this kind of code. > Source/WebCore/css/MediaQueryEvaluator.cpp:493 > + result = clampTo<int>(primitiveValue.doubleValue()); Seems strange to compute a double, clamp it to int, then convert back to double. What’s the rationale here? The proposed code is inefficient (round trip from floating point to int and back), truncates rather than rounding (is that what we want), and limits values to the range of a 32-bit integer (not clear why that is desirable given that this function returns a double).
(In reply to Darin Adler from comment #2) > Comment on attachment 425067 [details] > > Source/WebCore/css/MediaQueryEvaluator.cpp:493 > > + result = clampTo<int>(primitiveValue.doubleValue()); > > Seems strange to compute a double, clamp it to int, then convert back to > double. What’s the rationale here? The proposed code is inefficient (round > trip from floating point to int and back), truncates rather than rounding > (is that what we want), and limits values to the range of a 32-bit integer > (not clear why that is desirable given that this function returns a double). You're right, that is weird. I did that to match Chromium[1], but I don't see anywhere in the spec that suggests this should be clamped to an int value. The linked Chromium code is basically the same behavior as the existing code in WebKit for this branch (and the code for this branch is 10 years old). I tried returning a double from this branch and found no failing layout tests locally. I'll push a patch and see what EWS looks like. [1]: https://github.com/chromium/chromium/blob/09a0b960b27f6e08fbe67ad97e6c4fb55ada383f/third_party/blink/renderer/core/css/media_query_evaluator.cc#L443
Created attachment 425354 [details] Patch
<rdar://problem/76357291>
Created attachment 426271 [details] Patch
Comment on attachment 426271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426271&action=review > Source/WebCore/css/MediaQueryEvaluator.cpp:494 > + if (strict && value) Shouldn't that be !value?
Comment on attachment 426271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426271&action=review >> Source/WebCore/css/MediaQueryEvaluator.cpp:494 >> + if (strict && value) > > Shouldn't that be !value? Not given the comment above. It essentially says "in strict mode only 0 is allowed, not other values", which is what this code does.
Comment on attachment 426271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426271&action=review >>> Source/WebCore/css/MediaQueryEvaluator.cpp:494 >>> + if (strict && value) >> >> Shouldn't that be !value? > > Not given the comment above. It essentially says "in strict mode only 0 is allowed, not other values", which is what this code does. Ah we're in the isNumber block. Right.
Comment on attachment 426271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426271&action=review > Source/WebCore/css/MediaQueryEvaluator.cpp:501 > } Sadness...I waited for EWS to go green just to realize this `if` now only contains a single statement and doesn't need braces anymore. I'll upload a new patch.
Created attachment 426298 [details] Patch
Committed r276208 (236690@main): <https://commits.webkit.org/236690@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426298 [details].
Thanks for the review and cq+, Darin!