From b4546993b4f3f674f9776d5eb4f1e3698c9ac637 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 21 May 2023 22:07:08 +0200 Subject: [PATCH] style: Fix whitespace handling inside CSS variables As per w3c/csswg-drafts#881 and w3c/csswg-drafts#774. Differential Revision: https://phabricator.services.mozilla.com/D116459 --- components/style/custom_properties.rs | 16 ++----- .../style/properties/declaration_block.rs | 48 ++++--------------- .../style/properties/properties.mako.rs | 17 +++---- 3 files changed, 20 insertions(+), 61 deletions(-) diff --git a/components/style/custom_properties.rs b/components/style/custom_properties.rs index c031a0e3d87..df3f9660dd4 100644 --- a/components/style/custom_properties.rs +++ b/components/style/custom_properties.rs @@ -318,11 +318,6 @@ fn parse_declaration_value<'i, 't>( missing_closing_characters: &mut String, ) -> Result<(TokenSerializationType, TokenSerializationType), ParseError<'i>> { input.parse_until_before(Delimiter::Bang | Delimiter::Semicolon, |input| { - // Need at least one token - let start = input.state(); - input.next_including_whitespace()?; - input.reset(&start); - parse_declaration_value_block(input, references, missing_closing_characters) }) } @@ -334,6 +329,7 @@ fn parse_declaration_value_block<'i, 't>( mut references: Option<&mut VarOrEnvReferences>, missing_closing_characters: &mut String, ) -> Result<(TokenSerializationType, TokenSerializationType), ParseError<'i>> { + input.skip_whitespace(); let mut token_start = input.position(); let mut token = match input.next_including_whitespace_and_comments() { Ok(token) => token, @@ -477,10 +473,8 @@ fn parse_fallback<'i, 't>(input: &mut Parser<'i, 't>) -> Result<(), ParseError<' // Exclude `!` and `;` at the top level // https://drafts.csswg.org/css-syntax/#typedef-declaration-value input.parse_until_before(Delimiter::Bang | Delimiter::Semicolon, |input| { - // At least one non-comment token. - input.next_including_whitespace()?; // Skip until the end. - while let Ok(_) = input.next_including_whitespace_and_comments() {} + while input.next_including_whitespace_and_comments().is_ok() {} Ok(()) }) } @@ -996,12 +990,12 @@ fn substitute_block<'i>( while input.next().is_ok() {} } else { input.expect_comma()?; + input.skip_whitespace(); let after_comma = input.state(); let first_token_type = input .next_including_whitespace_and_comments() - // parse_var_function() ensures that .unwrap() will not fail. - .unwrap() - .serialization_type(); + .ok() + .map_or_else(TokenSerializationType::nothing, |t| t.serialization_type()); input.reset(&after_comma); let mut position = (after_comma.position(), first_token_type); last_token_type = substitute_block( diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 3639ae24f6e..6b05d7ab885 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -1076,15 +1076,9 @@ impl PropertyDeclarationBlock { // AppendableValue::Css. let mut v = CssString::new(); let value = match appendable_value { - AppendableValue::Css { - css, - with_variables, - } => { + AppendableValue::Css(css) => { debug_assert!(!css.is_empty()); - AppendableValue::Css { - css, - with_variables, - } + appendable_value }, other => { append_declaration_value(&mut v, other)?; @@ -1096,14 +1090,13 @@ impl PropertyDeclarationBlock { continue; } - AppendableValue::Css { + AppendableValue::Css({ // Safety: serialization only generates valid utf-8. #[cfg(feature = "gecko")] - css: unsafe { v.as_str_unchecked() }, + unsafe { v.as_str_unchecked() } #[cfg(feature = "servo")] - css: &v, - with_variables: false, - } + &v + }) }, }; @@ -1185,12 +1178,7 @@ where DeclarationsForShorthand(ShorthandId, I), /// A raw CSS string, coming for example from a property with CSS variables, /// or when storing a serialized shorthand value before appending directly. - Css { - /// The raw CSS string. - css: &'a str, - /// Whether the original serialization contained variables or not. - with_variables: bool, - }, + Css(&'a str), } /// Potentially appends whitespace after the first (property: value;) pair. @@ -1215,7 +1203,7 @@ where I: Iterator, { match appendable_value { - AppendableValue::Css { css, .. } => dest.write_str(css), + AppendableValue::Css(css) => dest.write_str(css), AppendableValue::Declaration(decl) => decl.to_css(dest), AppendableValue::DeclarationsForShorthand(shorthand, decls) => { shorthand.longhands_to_css(decls, &mut CssWriter::new(dest)) @@ -1238,25 +1226,7 @@ where handle_first_serialization(dest, is_first_serialization)?; property_name.to_css(&mut CssWriter::new(dest))?; - dest.write_char(':')?; - - // for normal parsed values, add a space between key: and value - match appendable_value { - AppendableValue::Declaration(decl) => { - if !decl.value_is_unparsed() { - // For normal parsed values, add a space between key: and value. - dest.write_str(" ")? - } - }, - AppendableValue::Css { with_variables, .. } => { - if !with_variables { - dest.write_str(" ")? - } - }, - // Currently append_serialization is only called with a Css or - // a Declaration AppendableValue. - AppendableValue::DeclarationsForShorthand(..) => unreachable!(), - } + dest.write_str(": ")?; append_declaration_value(dest, appendable_value)?; diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index b8a9255db48..dc48eed7608 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1521,7 +1521,7 @@ impl ShorthandId { // https://drafts.csswg.org/css-variables/#variables-in-shorthands if let Some(css) = first_declaration.with_variables_from_shorthand(self) { if declarations2.all(|d| d.with_variables_from_shorthand(self) == Some(css)) { - return Some(AppendableValue::Css { css, with_variables: true }); + return Some(AppendableValue::Css(css)); } return None; } @@ -1529,10 +1529,7 @@ impl ShorthandId { // Check whether they are all the same CSS-wide keyword. if let Some(keyword) = first_declaration.get_css_wide_keyword() { if declarations2.all(|d| d.get_css_wide_keyword() == Some(keyword)) { - return Some(AppendableValue::Css { - css: keyword.to_str(), - with_variables: false, - }); + return Some(AppendableValue::Css(keyword.to_str())) } return None; } @@ -1725,7 +1722,8 @@ impl UnparsedValue { let mut input = ParserInput::new(&css); let mut input = Parser::new(&mut input); - input.skip_whitespace(); // Unnecessary for correctness, but may help try() rewind less. + input.skip_whitespace(); + if let Ok(keyword) = input.try_parse(CSSWideKeyword::parse) { return Cow::Owned(PropertyDeclaration::css_wide_keyword(longhand_id, keyword)); } @@ -2441,12 +2439,11 @@ impl PropertyDeclaration { debug_assert!(id.allowed_in(context), "{:?}", id); let non_custom_id = id.non_custom_id(); + input.skip_whitespace(); + let start = input.state(); match id { PropertyId::Custom(property_name) => { - // FIXME: fully implement https://github.com/w3c/csswg-drafts/issues/774 - // before adding skip_whitespace here. - // This probably affects some test results. let value = match input.try_parse(CSSWideKeyword::parse) { Ok(keyword) => CustomDeclarationValue::CSSWideKeyword(keyword), Err(()) => CustomDeclarationValue::Value( @@ -2461,7 +2458,6 @@ impl PropertyDeclaration { } PropertyId::LonghandAlias(id, _) | PropertyId::Longhand(id) => { - input.skip_whitespace(); // Unnecessary for correctness, but may help try() rewind less. input.try_parse(CSSWideKeyword::parse).map(|keyword| { PropertyDeclaration::css_wide_keyword(id, keyword) }).or_else(|()| { @@ -2491,7 +2487,6 @@ impl PropertyDeclaration { } PropertyId::ShorthandAlias(id, _) | PropertyId::Shorthand(id) => { - input.skip_whitespace(); // Unnecessary for correctness, but may help try() rewind less. if let Ok(keyword) = input.try_parse(CSSWideKeyword::parse) { if id == ShorthandId::All { declarations.all_shorthand = AllShorthand::CSSWideKeyword(keyword)