From ae72dbaf59ca124b9ed9f699561a9ef67cab0608 Mon Sep 17 00:00:00 2001 From: LeoniePhiline <22329650+LeoniePhiline@users.noreply.github.com> Date: Sun, 6 Nov 2022 19:39:11 +0100 Subject: [PATCH 1/5] feat: Add failing test --- src/media_query.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/media_query.rs b/src/media_query.rs index 57289ed9..ac25ca27 100644 --- a/src/media_query.rs +++ b/src/media_query.rs @@ -997,7 +997,7 @@ fn process_condition<'i>( #[cfg(test)] mod tests { use super::*; - use crate::stylesheet::PrinterOptions; + use crate::{stylesheet::PrinterOptions, targets::Browsers}; fn parse(s: &str) -> MediaQuery { let mut input = ParserInput::new(&s); @@ -1044,4 +1044,14 @@ mod tests { assert_eq!(and("only screen", "all"), "only screen"); assert_eq!(and("print", "print"), "print"); } + + #[test] + fn test_negated_interval_parens() { + let media_query = parse("screen and not (200px <= width < 500px)"); + let printer_options = PrinterOptions { + targets: Browsers::from_browserslist(["chrome 95"]).unwrap(), + ..Default::default() + }; + assert_eq!(media_query.to_css_string(printer_options).unwrap(), "screen and not ((min-width: 200px) and (max-width: 499.999px))"); + } } From c9161de3622a38fe8908292cc831aaf1b2b964a2 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 27 Nov 2022 12:16:06 -0500 Subject: [PATCH 2/5] Fix test --- src/media_query.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/media_query.rs b/src/media_query.rs index ac25ca27..8378d049 100644 --- a/src/media_query.rs +++ b/src/media_query.rs @@ -1049,7 +1049,10 @@ mod tests { fn test_negated_interval_parens() { let media_query = parse("screen and not (200px <= width < 500px)"); let printer_options = PrinterOptions { - targets: Browsers::from_browserslist(["chrome 95"]).unwrap(), + targets: Some(Browsers { + chrome: Some(95 << 16), + ..Default::default() + }), ..Default::default() }; assert_eq!(media_query.to_css_string(printer_options).unwrap(), "screen and not ((min-width: 200px) and (max-width: 499.999px))"); From f8f9eff64da1768e5b9c5a03e36af80b51c482c2 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 27 Nov 2022 12:22:46 -0500 Subject: [PATCH 3/5] Add parens around intervals in not expressions --- src/media_query.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/media_query.rs b/src/media_query.rs index 8378d049..95afcd20 100644 --- a/src/media_query.rs +++ b/src/media_query.rs @@ -457,7 +457,22 @@ impl<'i> ToCss for MediaCondition<'i> { MediaCondition::Feature(ref f) => f.to_css(dest), MediaCondition::Not(ref c) => { dest.write_str("not ")?; - c.to_css(dest) + + let needs_parens = dest.targets.is_some() && + matches!(&**c, MediaCondition::Feature(f) if matches!(f, MediaFeature::Interval { .. })) && + !Feature::MediaIntervalSyntax.is_compatible(dest.targets.unwrap()); + + if needs_parens { + dest.write_char('(')?; + } + + c.to_css(dest)?; + + if needs_parens { + dest.write_char(')')?; + } + + Ok(()) } MediaCondition::InParens(ref c) => { dest.write_char('(')?; From 4630f14d1671c6ea5719780bc210209a968df832 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 27 Nov 2022 13:03:44 -0500 Subject: [PATCH 4/5] Remove parentheses from AST and insert where needed during printing --- src/lib.rs | 6 ++- src/media_query.rs | 99 +++++++++++++++++++++------------------------- 2 files changed, 49 insertions(+), 56 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 43bf881c..8dac3d0e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6717,6 +6717,8 @@ mod tests { ); minify_test("@media { .foo { color: chartreuse }}", ".foo{color:#7fff00}"); minify_test("@media all { .foo { color: chartreuse }}", ".foo{color:#7fff00}"); + minify_test("@media not (((color) or (hover))) { .foo { color: chartreuse }}", "@media not ((color) or (hover)){.foo{color:#7fff00}}"); + minify_test("@media (hover) and ((color) and (test)) { .foo { color: chartreuse }}", "@media (hover) and (color) and (test){.foo{color:#7fff00}}"); prefix_test( r#" @@ -22586,7 +22588,7 @@ mod tests { } } "#, - "@container my-layout (not (width>500px)){.foo{color:red}}", + "@container my-layout not (width>500px){.foo{color:red}}", ); minify_test( @@ -22619,7 +22621,7 @@ mod tests { } } "#, - "@container my-layout ((width:100px) and (not (height:100px))){.foo{color:red}}", + "@container my-layout (width:100px) and (not (height:100px)){.foo{color:red}}", ); minify_test( diff --git a/src/media_query.rs b/src/media_query.rs index 95afcd20..f3973608 100644 --- a/src/media_query.rs +++ b/src/media_query.rs @@ -290,17 +290,8 @@ impl<'i> MediaQuery<'i> { if let Some(cond) = &b.condition { self.condition = if let Some(condition) = &self.condition { if condition != cond { - macro_rules! parenthesize { - ($condition: ident) => { - if matches!($condition, MediaCondition::Operation(_, Operator::Or)) { - MediaCondition::InParens(Box::new($condition.clone())) - } else { - $condition.clone() - } - }; - } Some(MediaCondition::Operation( - vec![parenthesize!(condition), parenthesize!(cond)], + vec![condition.clone(), cond.clone()], Operator::And, )) } else { @@ -346,11 +337,14 @@ impl<'i> ToCss for MediaQuery<'i> { None => return Ok(()), }; - if self.media_type != MediaType::All || self.qualifier.is_some() { + let needs_parens = if self.media_type != MediaType::All || self.qualifier.is_some() { dest.write_str(" and ")?; - } + matches!(condition, MediaCondition::Operation(_, op) if *op != Operator::And) + } else { + false + }; - condition.to_css(dest) + condition.to_css_with_parens_if_needed(dest, needs_parens) } } @@ -381,9 +375,6 @@ pub enum MediaCondition<'i> { /// A set of joint operations. #[skip_type] Operation(Vec>, Operator), - /// A condition wrapped in parenthesis. - #[skip_type] - InParens(Box>), } impl<'i> MediaCondition<'i> { @@ -439,13 +430,35 @@ impl<'i> MediaCondition<'i> { fn parse_paren_block<'t>(input: &mut Parser<'i, 't>) -> Result>> { input.parse_nested_block(|input| { if let Ok(inner) = input.try_parse(|i| Self::parse(i, true)) { - return Ok(MediaCondition::InParens(Box::new(inner))); + return Ok(inner); } let feature = MediaFeature::parse(input)?; Ok(MediaCondition::Feature(feature)) }) } + + fn needs_parens(&self, parent_operator: Operator) -> bool { + match self { + MediaCondition::Not(_) => true, + MediaCondition::Operation(_, op) => *op != parent_operator, + _ => false + } + } + + fn to_css_with_parens_if_needed(&self, dest: &mut Printer, needs_parens: bool) -> Result<(), PrinterError> + where + W: std::fmt::Write, + { + if needs_parens { + dest.write_char('(')?; + } + self.to_css(dest)?; + if needs_parens { + dest.write_char(')')?; + } + Ok(()) + } } impl<'i> ToCss for MediaCondition<'i> { @@ -458,35 +471,22 @@ impl<'i> ToCss for MediaCondition<'i> { MediaCondition::Not(ref c) => { dest.write_str("not ")?; - let needs_parens = dest.targets.is_some() && - matches!(&**c, MediaCondition::Feature(f) if matches!(f, MediaFeature::Interval { .. })) && - !Feature::MediaIntervalSyntax.is_compatible(dest.targets.unwrap()); - - if needs_parens { - dest.write_char('(')?; - } - - c.to_css(dest)?; + let needs_parens = matches!(&**c, MediaCondition::Operation(..)) + || (dest.targets.is_some() + && matches!(&**c, MediaCondition::Feature(f) if matches!(f, MediaFeature::Interval { .. })) + && !Feature::MediaIntervalSyntax.is_compatible(dest.targets.unwrap())); - if needs_parens { - dest.write_char(')')?; - } - - Ok(()) - } - MediaCondition::InParens(ref c) => { - dest.write_char('(')?; - c.to_css(dest)?; - dest.write_char(')') + c.to_css_with_parens_if_needed(dest, needs_parens) } MediaCondition::Operation(ref list, op) => { let mut iter = list.iter(); - iter.next().unwrap().to_css(dest)?; + let first = iter.next().unwrap(); + first.to_css_with_parens_if_needed(dest, first.needs_parens(op))?; for item in iter { dest.write_char(' ')?; op.to_css(dest)?; dest.write_char(' ')?; - item.to_css(dest)?; + item.to_css_with_parens_if_needed(dest, item.needs_parens(op))?; } Ok(()) } @@ -894,21 +894,9 @@ fn process_condition<'i>( MediaCondition::Not(cond) => { *condition = (**cond).clone(); } - MediaCondition::InParens(parens) => { - if let MediaCondition::Not(cond) = &**parens { - *condition = (**cond).clone(); - } - } _ => {} } } - MediaCondition::InParens(cond) => { - let res = process_condition(loc, custom_media, media_type, qualifier, &mut *cond, seen); - if let MediaCondition::InParens(cond) = &**cond { - *condition = (**cond).clone(); - } - return res; - } MediaCondition::Operation(conditions, _) => { let mut res = Ok(true); conditions.retain_mut(|condition| { @@ -978,8 +966,8 @@ fn process_condition<'i>( } // Parentheses are required around the condition unless there is a single media feature. match condition { - MediaCondition::Feature(..) | MediaCondition::InParens(..) => Some(condition), - _ => Some(MediaCondition::InParens(Box::new(condition))), + MediaCondition::Feature(..) => Some(condition), + _ => Some(condition), } } else { None @@ -1000,7 +988,7 @@ fn process_condition<'i>( if conditions.len() == 1 { *condition = conditions.pop().unwrap(); } else { - *condition = MediaCondition::InParens(Box::new(MediaCondition::Operation(conditions, Operator::Or))); + *condition = MediaCondition::Operation(conditions, Operator::Or); } } _ => {} @@ -1070,6 +1058,9 @@ mod tests { }), ..Default::default() }; - assert_eq!(media_query.to_css_string(printer_options).unwrap(), "screen and not ((min-width: 200px) and (max-width: 499.999px))"); + assert_eq!( + media_query.to_css_string(printer_options).unwrap(), + "screen and not ((min-width: 200px) and (max-width: 499.999px))" + ); } } From 318a5f241afc6814349847b654712c88d7846e81 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Sun, 27 Nov 2022 13:23:30 -0500 Subject: [PATCH 5/5] Fix interval inside or operation --- src/lib.rs | 63 ++++++++++++++++++++++++++++++++++++++++++++++ src/media_query.rs | 24 +++++++++--------- 2 files changed, 75 insertions(+), 12 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 8dac3d0e..da27fb58 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6867,6 +6867,69 @@ mod tests { }, ); + prefix_test( + r#" + @media not (100px <= width <= 200px) { + .foo { + color: chartreuse; + } + } + "#, + indoc! { r#" + @media not ((min-width: 100px) and (max-width: 200px)) { + .foo { + color: #7fff00; + } + } + "#}, + Browsers { + firefox: Some(85 << 16), + ..Browsers::default() + }, + ); + + prefix_test( + r#" + @media (hover) and (100px <= width <= 200px) { + .foo { + color: chartreuse; + } + } + "#, + indoc! { r#" + @media (hover) and (min-width: 100px) and (max-width: 200px) { + .foo { + color: #7fff00; + } + } + "#}, + Browsers { + firefox: Some(85 << 16), + ..Browsers::default() + }, + ); + + prefix_test( + r#" + @media (hover) or (100px <= width <= 200px) { + .foo { + color: chartreuse; + } + } + "#, + indoc! { r#" + @media (hover) or ((min-width: 100px) and (max-width: 200px)) { + .foo { + color: #7fff00; + } + } + "#}, + Browsers { + firefox: Some(85 << 16), + ..Browsers::default() + }, + ); + prefix_test( r#" @media (100px < width < 200px) { diff --git a/src/media_query.rs b/src/media_query.rs index f3973608..34f60cc7 100644 --- a/src/media_query.rs +++ b/src/media_query.rs @@ -6,6 +6,7 @@ use crate::macros::enum_property; use crate::printer::Printer; use crate::rules::custom_media::CustomMediaRule; use crate::rules::Location; +use crate::targets::Browsers; use crate::traits::{Parse, ToCss}; use crate::values::ident::Ident; use crate::values::number::CSSNumber; @@ -438,11 +439,16 @@ impl<'i> MediaCondition<'i> { }) } - fn needs_parens(&self, parent_operator: Operator) -> bool { + fn needs_parens(&self, parent_operator: Option, targets: &Option) -> bool { match self { MediaCondition::Not(_) => true, - MediaCondition::Operation(_, op) => *op != parent_operator, - _ => false + MediaCondition::Operation(_, op) => Some(*op) != parent_operator, + MediaCondition::Feature(f) => { + parent_operator != Some(Operator::And) + && targets.is_some() + && matches!(f, MediaFeature::Interval { .. }) + && !Feature::MediaIntervalSyntax.is_compatible(targets.unwrap()) + } } } @@ -470,23 +476,17 @@ impl<'i> ToCss for MediaCondition<'i> { MediaCondition::Feature(ref f) => f.to_css(dest), MediaCondition::Not(ref c) => { dest.write_str("not ")?; - - let needs_parens = matches!(&**c, MediaCondition::Operation(..)) - || (dest.targets.is_some() - && matches!(&**c, MediaCondition::Feature(f) if matches!(f, MediaFeature::Interval { .. })) - && !Feature::MediaIntervalSyntax.is_compatible(dest.targets.unwrap())); - - c.to_css_with_parens_if_needed(dest, needs_parens) + c.to_css_with_parens_if_needed(dest, c.needs_parens(None, &dest.targets)) } MediaCondition::Operation(ref list, op) => { let mut iter = list.iter(); let first = iter.next().unwrap(); - first.to_css_with_parens_if_needed(dest, first.needs_parens(op))?; + first.to_css_with_parens_if_needed(dest, first.needs_parens(Some(op), &dest.targets))?; for item in iter { dest.write_char(' ')?; op.to_css(dest)?; dest.write_char(' ')?; - item.to_css_with_parens_if_needed(dest, item.needs_parens(op))?; + item.to_css_with_parens_if_needed(dest, item.needs_parens(Some(op), &dest.targets))?; } Ok(()) }