From fb975cc15656660c1f562315dceb1a976a133462 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Thu, 22 May 2025 17:02:44 +0100 Subject: [PATCH] LibWeb/CSS: Correct how we evaluate boolean media-features The spec has a general rule for this, which is roughly that "If it's not a falsey value, it's true". However, a couple of media-features are always false, apparently breaking this rule. To handle that, we have an array of false keywords in the JSON, instead of a single keyword. For those always-false media-features, we can enter all their values into this array. Gets us 2 more WPT subtest passes. --- Documentation/CSSGeneratedFiles.md | 10 ++-- Libraries/LibWeb/CSS/MediaFeatures.json | 58 +++++++++++++++++++ Libraries/LibWeb/CSS/MediaQuery.cpp | 9 +-- .../LibWeb/GenerateCSSMediaFeatureID.cpp | 37 ++++++++++++ .../css/mediaqueries/dynamic-range.txt | 7 +-- 5 files changed, 107 insertions(+), 14 deletions(-) diff --git a/Documentation/CSSGeneratedFiles.md b/Documentation/CSSGeneratedFiles.md index d28b9f4d1ee..1ff682db68d 100644 --- a/Documentation/CSSGeneratedFiles.md +++ b/Documentation/CSSGeneratedFiles.md @@ -216,10 +216,11 @@ They are listed in the [`@media` descriptor table](https://www.w3.org/TR/mediaqu The definitions here are like a simplified version of the `Properties.json` definitions. -| Field | Description | -|----------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `type` | String. How the media-feature is evaluated, either `discrete` or `range`. | -| `values` | Array of strings. These are directly taken from the spec, with keywords as they are, and `<>` around type names. Types may be ``, ``, ``, ``, or ``. | +| Field | Description | +|------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `type` | String. How the media-feature is evaluated, either `discrete` or `range`. | +| `values` | Array of strings. These are directly taken from the spec, with keywords as they are, and `<>` around type names. Types may be ``, ``, ``, ``, or ``. | +| `false-keywords` | Array of strings. These are any keywords that should be considered false when the media feature is evaluated as `@media (foo)`. Generally this will be a single value, such as `"none"`. | The generated code provides: - A `MediaFeatureValueType` enum listing the possible value types @@ -229,6 +230,7 @@ The generated code provides: - `bool media_feature_type_is_range(MediaFeatureID)` returns whether the media feature is a `range` type, as opposed to a `discrete` type - `bool media_feature_accepts_type(MediaFeatureID, MediaFeatureValueType)` returns whether the media feature will accept values of this type - `bool media_feature_accepts_keyword(MediaFeatureID, Keyword)` returns whether the media feature accepts this keyword +- `bool media_feature_keyword_is_falsey(MediaFeatureID, Keyword)` returns whether the given keyword is considered false when the media-feature is evaluated in a boolean context. (Like `@media (foo)`) ## MathFunctions.json diff --git a/Libraries/LibWeb/CSS/MediaFeatures.json b/Libraries/LibWeb/CSS/MediaFeatures.json index 2beaf64c05b..40f9cefcb7a 100644 --- a/Libraries/LibWeb/CSS/MediaFeatures.json +++ b/Libraries/LibWeb/CSS/MediaFeatures.json @@ -4,6 +4,9 @@ "values": [ "none", "hover" + ], + "false-keywords": [ + "none" ] }, "any-pointer": { @@ -12,6 +15,9 @@ "none", "coarse", "fine" + ], + "false-keywords": [ + "none" ] }, "aspect-ratio": { @@ -73,6 +79,11 @@ "values": [ "standard", "high" + ], + "FIXME": "This always evaluating to false in a boolean context seems like a spec bug. https://github.com/w3c/csswg-drafts/issues/8050", + "false-keywords": [ + "standard", + "high" ] }, "environment-blending": { @@ -88,6 +99,9 @@ "values": [ "none", "active" + ], + "false-keywords": [ + "none" ] }, "grid": { @@ -113,6 +127,9 @@ "values": [ "none", "hover" + ], + "false-keywords": [ + "none" ] }, "inverted-colors": { @@ -120,6 +137,9 @@ "values": [ "none", "inverted" + ], + "false-keywords": [ + "none" ] }, "monochrome": { @@ -133,6 +153,9 @@ "values": [ "none", "back" + ], + "false-keywords": [ + "none" ] }, "orientation": { @@ -148,6 +171,9 @@ "none", "scroll", "paged" + ], + "false-keywords": [ + "none" ] }, "overflow-inline": { @@ -155,6 +181,9 @@ "values": [ "none", "scroll" + ], + "false-keywords": [ + "none" ] }, "pointer": { @@ -163,6 +192,9 @@ "none", "coarse", "fine" + ], + "false-keywords": [ + "none" ] }, "prefers-color-scheme": { @@ -179,6 +211,9 @@ "less", "more", "custom" + ], + "false-keywords": [ + "no-preference" ] }, "prefers-reduced-data": { @@ -186,6 +221,9 @@ "values": [ "no-preference", "reduce" + ], + "false-keywords": [ + "no-preference" ] }, "prefers-reduced-motion": { @@ -193,6 +231,9 @@ "values": [ "no-preference", "reduce" + ], + "false-keywords": [ + "no-preference" ] }, "prefers-reduced-transparency": { @@ -200,6 +241,9 @@ "values": [ "no-preference", "reduce" + ], + "false-keywords": [ + "no-preference" ] }, "resolution": { @@ -214,6 +258,9 @@ "values": [ "interlace", "progressive" + ], + "false-keywords": [ + "none" ] }, "scripting": { @@ -222,6 +269,9 @@ "none", "initial-only", "enabled" + ], + "false-keywords": [ + "none" ] }, "update": { @@ -230,6 +280,9 @@ "none", "slow", "fast" + ], + "false-keywords": [ + "none" ] }, "vertical-viewport-segments": { @@ -251,6 +304,11 @@ "values": [ "standard", "high" + ], + "FIXME": "See dynamic-range", + "false-keywords": [ + "standard", + "high" ] }, "width": { diff --git a/Libraries/LibWeb/CSS/MediaQuery.cpp b/Libraries/LibWeb/CSS/MediaQuery.cpp index 6e8afb871b1..5be730a98c8 100644 --- a/Libraries/LibWeb/CSS/MediaQuery.cpp +++ b/Libraries/LibWeb/CSS/MediaQuery.cpp @@ -121,12 +121,9 @@ MatchResult MediaFeature::evaluate(HTML::Window const* window) const if (queried_value.is_resolution()) return as_match_result(queried_value.resolution().resolved(calculation_context).map([](auto& it) { return it.to_dots_per_pixel(); }).value_or(0) != 0); if (queried_value.is_ident()) { - // NOTE: It is not technically correct to always treat `no-preference` as false, but every - // media-feature that accepts it as a value treats it as false, so good enough. :^) - // If other features gain this property for other keywords in the future, we can - // add more robust handling for them then. - return as_match_result(queried_value.ident() != Keyword::None - && queried_value.ident() != Keyword::NoPreference); + if (media_feature_keyword_is_falsey(m_id, queried_value.ident())) + return MatchResult::False; + return MatchResult::True; } return MatchResult::False; diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateCSSMediaFeatureID.cpp b/Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateCSSMediaFeatureID.cpp index 4e7510fda91..d8372b830ff 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateCSSMediaFeatureID.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/GenerateCSSMediaFeatureID.cpp @@ -80,6 +80,8 @@ bool media_feature_type_is_range(MediaFeatureID); bool media_feature_accepts_type(MediaFeatureID, MediaFeatureValueType); bool media_feature_accepts_keyword(MediaFeatureID, Keyword); +bool media_feature_keyword_is_falsey(MediaFeatureID, Keyword); + } )~~~"); @@ -290,6 +292,41 @@ bool media_feature_accepts_keyword(MediaFeatureID media_feature_id, Keyword keyw VERIFY_NOT_REACHED(); } +bool media_feature_keyword_is_falsey(MediaFeatureID media_feature_id, Keyword keyword) +{ + switch (media_feature_id) {)~~~"); + media_feature_data.for_each_member([&](auto& name, JsonValue const& feature_value) { + VERIFY(feature_value.is_object()); + auto& feature = feature_value.as_object(); + auto false_keywords = feature.get_array("false-keywords"sv); + if (!false_keywords.has_value() || false_keywords->is_empty()) + return; + + auto member_generator = generator.fork(); + member_generator.set("name:titlecase", title_casify(name)); + member_generator.append(R"~~~( + case MediaFeatureID::@name:titlecase@: + switch (keyword) {)~~~"); + + false_keywords.value().for_each([&](JsonValue const& value) { + auto value_generator = member_generator.fork(); + member_generator.set("false_keyword:titlecase", title_casify(value.as_string())); + member_generator.append(R"~~~( + case Keyword::@false_keyword:titlecase@:)~~~"); + }); + member_generator.append(R"~~~( + return true; + default: + return false; + })~~~"); + }); + + generator.append(R"~~~( + default: + return false; + } +} + } )~~~"); diff --git a/Tests/LibWeb/Text/expected/wpt-import/css/mediaqueries/dynamic-range.txt b/Tests/LibWeb/Text/expected/wpt-import/css/mediaqueries/dynamic-range.txt index fada10bab37..b2e205c3ebd 100644 --- a/Tests/LibWeb/Text/expected/wpt-import/css/mediaqueries/dynamic-range.txt +++ b/Tests/LibWeb/Text/expected/wpt-import/css/mediaqueries/dynamic-range.txt @@ -2,8 +2,7 @@ Harness status: OK Found 23 tests -21 Pass -2 Fail +23 Pass Pass Should be known: '(dynamic-range: standard)' Pass Should be known: '(dynamic-range: high)' Pass Should be known: '(video-dynamic-range: standard)' @@ -22,8 +21,8 @@ Pass Should be parseable: '(video-dynamic-range: 10px)' Pass Should be unknown: '(video-dynamic-range: 10px)' Pass Should be parseable: '(video-dynamic-range: invalid)' Pass Should be unknown: '(video-dynamic-range: invalid)' -Fail Check that dynamic-range evaluates to false in the boolean context -Fail Check that video-dynamic-range evaluates to false in the boolean context +Pass Check that dynamic-range evaluates to false in the boolean context +Pass Check that video-dynamic-range evaluates to false in the boolean context Pass Check that dynamic-range always matches 'standard' Pass Check that video-dynamic-range always matches 'standard' Pass Check that video-dynamic-range is not 'invalid' \ No newline at end of file