Skip to content

Commit c4091ee

Browse files
authored
Ensure compiled range media queries are correctly parenthesised (#1114)
Fixes #1105 In the test case added, the range query `(width < 256px)` gets compiled into `not (min-width: 256px)`. If we combine this with another query, for example `(width < 256px) or (hover: none)`, the compiled query should parenthesise the compiled range query. That is, the output should be `(not (min-width: 256px)) or (hover: none)`. Instead, we see an output of `not (min-width: 256px) or (hover: none)`, which incorrectly negates the entire media query, not just the `min-width`. `QueryFeature::needs_parens` determines if parentheses are need with the following logic: ```rust match self { QueryFeature::Interval { .. } => parent_operator != Some(Operator::And), QueryFeature::Range { operator, .. } => { matches!( operator, MediaFeatureComparison::GreaterThan | MediaFeatureComparison::LessThan ) } _ => false, } } ``` This is correct ***if*** both interval and range queries are being compiled, since parentheses are required when the range is replaced with a negation. However, above this return was the block: ```rust if !should_compile!(targets, MediaIntervalSyntax) { return false; } ``` This causes the check to be skipped when `Feature::MediaIntervalSyntax` isn't enabled. This leaves an edge case: if `Feature::MediaRangeSyntax` is enabled, without `Feature::MediaIntervalSyntax`, the correct parentheses check isn't carried out. One would think that this is an unreasonable edge case, since targeting a browser without range syntax support would also be a browser without interval syntax support. However, in `turbopack-css`, the `MediaRangeSyntax` feature is always included, without adding `MediaIntervalSyntax` (see vercel/next.js@a95f861#diff-389b0ea768c0dbbca95b4aff5d1237ecddb33677521e3e2eb1f717c43c0d4658). In Next 16, the default targets were updated (see vercel/next.js#84401), which caused `MediaIntervalSyntax` to no longer be included by default, hence opening up this edge case. I beleive that, unfortunately, the reason for Next adding `MediaIntervalSyntax` still holds, and applies to intervals too. I'll make a PR there to add `MediaIntervalSyntax` which will avoid this edge case, but it'd be nice to fix it here, too :) This moves the `MediaIntervalSyntax` check into the `Interval` case in the switch, and adds a separate `MediaRangeSyntax` check in the `Range` case.
1 parent 28d7794 commit c4091ee

File tree

2 files changed

+40
-9
lines changed

2 files changed

+40
-9
lines changed

src/lib.rs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,13 @@ mod tests {
7979
assert_eq!(res.code, expected);
8080
}
8181

82+
fn test_with_printer_options<'i, 'o>(source: &'i str, expected: &'i str, options: PrinterOptions<'o>) {
83+
let mut stylesheet = StyleSheet::parse(&source, ParserOptions::default()).unwrap();
84+
stylesheet.minify(MinifyOptions::default()).unwrap();
85+
let res = stylesheet.to_css(options).unwrap();
86+
assert_eq!(res.code, expected);
87+
}
88+
8289
fn minify_test(source: &str, expected: &str) {
8390
minify_test_with_options(source, expected, ParserOptions::default())
8491
}
@@ -9064,6 +9071,31 @@ mod tests {
90649071
},
90659072
);
90669073

9074+
test_with_printer_options(
9075+
r#"
9076+
@media (width < 256px) or (hover: none) {
9077+
.foo {
9078+
color: #fff;
9079+
}
9080+
}
9081+
"#,
9082+
indoc! { r#"
9083+
@media (not (min-width: 256px)) or (hover: none) {
9084+
.foo {
9085+
color: #fff;
9086+
}
9087+
}
9088+
"#},
9089+
PrinterOptions {
9090+
targets: Targets {
9091+
browsers: None,
9092+
include: Features::MediaRangeSyntax,
9093+
exclude: Features::empty(),
9094+
},
9095+
..Default::default()
9096+
},
9097+
);
9098+
90679099
error_test(
90689100
"@media (min-width: hi) { .foo { color: chartreuse }}",
90699101
ParserError::InvalidMediaQuery,

src/media_query.rs

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1131,17 +1131,16 @@ where
11311131
}
11321132

11331133
pub(crate) fn needs_parens(&self, parent_operator: Option<Operator>, targets: &Targets) -> bool {
1134-
if !should_compile!(targets, MediaIntervalSyntax) {
1135-
return false;
1136-
}
1137-
11381134
match self {
1139-
QueryFeature::Interval { .. } => parent_operator != Some(Operator::And),
1135+
QueryFeature::Interval { .. } => {
1136+
should_compile!(targets, MediaIntervalSyntax) && parent_operator != Some(Operator::And)
1137+
}
11401138
QueryFeature::Range { operator, .. } => {
1141-
matches!(
1142-
operator,
1143-
MediaFeatureComparison::GreaterThan | MediaFeatureComparison::LessThan
1144-
)
1139+
should_compile!(targets, MediaRangeSyntax)
1140+
&& matches!(
1141+
operator,
1142+
MediaFeatureComparison::GreaterThan | MediaFeatureComparison::LessThan
1143+
)
11451144
}
11461145
_ => false,
11471146
}

0 commit comments

Comments
 (0)