Skip to content

Commit

Permalink
style: Fix whitespace handling inside CSS variables
Browse files Browse the repository at this point in the history 
As per w3c/csswg-drafts#881 and w3c/csswg-drafts#774.

Differential Revision: https://phabricator.services.mozilla.com/D116459
  • Loading branch information
@emilio @Loirooriol
emilio authored and Loirooriol committed May 24, 2023
1 parent 832807f commit b454699
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 61 deletions.
16 changes: 5 additions & 11 deletions components/style/custom_properties.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
Expand All @@ -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,
Expand Down Expand Up @@ -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(())
})
}
Expand Down Expand Up @@ -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(
Expand Down
48 changes: 9 additions & 39 deletions components/style/properties/declaration_block.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand All @@ -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
})
},
};

Expand Down Expand Up @@ -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.
Expand All @@ -1215,7 +1203,7 @@ where
I: Iterator<Item = &'a PropertyDeclaration>,
{
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))
Expand All @@ -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)?;

Expand Down
17 changes: 6 additions & 11 deletions components/style/properties/properties.mako.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1521,18 +1521,15 @@ 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;
}

// 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;
}
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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(
Expand All @@ -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(|()| {
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit b454699

Please sign in to comment.