Bug 224097 – Media queries with max-width greater than 999999999px evaluate to false

• Component: CSS
• Status: RESOLVED FIXED

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

[Tyler Wilcock]

Created attachment 425067 [details]
Patch

[Darin Adler]

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

[Tyler Wilcock]

(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

[Tyler Wilcock]

Created attachment 425354 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/76357291>

[Tyler Wilcock]

Created attachment 426271 [details]
Patch

[Simon Fraser (smfr)]

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?

[Darin Adler]

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.

[Simon Fraser (smfr)]

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.

[Tyler Wilcock]

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.

[Tyler Wilcock]

Created attachment 426298 [details]
Patch

[EWS]

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

[Tyler Wilcock]

Thanks for the review and cq+, Darin!