Bug 224097 - Media queries with max-width greater than 999999999px evaluate to false
Summary: Media queries with max-width greater than 999999999px evaluate to false
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Mac (Intel) macOS 11
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-02 01:55 PDT by mathieu
Modified: 2021-04-17 16:28 PDT (History)
15 users (show)

See Also:


Attachments
Patch (8.76 KB, patch)
2021-04-02 17:10 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (12.19 KB, patch)
2021-04-06 22:05 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (12.16 KB, patch)
2021-04-16 13:50 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (12.22 KB, patch)
2021-04-16 17:43 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mathieu 2021-04-02 01:55:33 PDT
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
Comment 1 Tyler Wilcock 2021-04-02 17:10:45 PDT
Created attachment 425067 [details]
Patch
Comment 2 Darin Adler 2021-04-06 15:28:44 PDT
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).
Comment 3 Tyler Wilcock 2021-04-06 21:26:17 PDT
(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
Comment 4 Tyler Wilcock 2021-04-06 22:05:32 PDT
Created attachment 425354 [details]
Patch
Comment 5 Radar WebKit Bug Importer 2021-04-07 11:25:47 PDT
<rdar://problem/76357291>
Comment 6 Tyler Wilcock 2021-04-16 13:50:55 PDT
Created attachment 426271 [details]
Patch
Comment 7 Simon Fraser (smfr) 2021-04-16 14:15:57 PDT
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 8 Darin Adler 2021-04-16 14:25:09 PDT
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 9 Simon Fraser (smfr) 2021-04-16 14:30:07 PDT
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 10 Tyler Wilcock 2021-04-16 17:31:09 PDT
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.
Comment 11 Tyler Wilcock 2021-04-16 17:43:50 PDT
Created attachment 426298 [details]
Patch
Comment 12 EWS 2021-04-17 15:21:28 PDT
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].
Comment 13 Tyler Wilcock 2021-04-17 16:28:37 PDT
Thanks for the review and cq+, Darin!