Skip to content

Commit

Permalink
Bug 1713787 - Fix whitespace handling inside CSS variables. r=xidorn
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
emilio committed Jun 7, 2021
1 parent 744c62a commit 488d886
Show file tree
Hide file tree
Showing 26 changed files with 105 additions and 182 deletions.
2 changes: 1 addition & 1 deletion browser/components/privatebrowsing/test/browser/browser_privatebrowsing_about.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ add_task(async function test_search_icon() {
await SpecialPowers.spawn(tab, [expectedIconURL], async function(iconURL) {
is(
content.document.body.getAttribute("style"),
`--newtab-search-icon:url(${iconURL});`,
`--newtab-search-icon: url(${iconURL});`,
"Should have the correct icon URL for the logo"
);
});
Expand Down
20 changes: 10 additions & 10 deletions layout/style/test/test_css_supports_variables.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
"(color: var( --a ) )",
"(color: var(--a, ))",
"(color: var(--a,/**/a))",
"(color: var(--a,))",
"(color: var(--a,/**/))",
"(color: 1px var(--a))",
"(color: var(--a) 1px)",
"(color: something 3px url(whereever) calc(var(--a) + 1px))",
Expand Down Expand Up @@ -59,6 +61,8 @@
"(--a: var( --b ) )",
"(--a: var(--b, ))",
"(--a: var(--b,/**/a))",
"(--a: var(--b,))",
"(--a: var(--b,/**/))",
"(--a: 1px var(--b))",
"(--a: var(--b) 1px)",
"(--a: something 3px url(whereever) calc(var(--b) + 1px))",
Expand Down Expand Up @@ -86,11 +90,10 @@
"(--a: '",
"(--a: '\\",
"(--a: \\",
"(--a:)",
];

var failingConditions = [
"(color: var(--a,))",
"(color: var(--a,/**/))",
"(color: var(--a,!))",
"(color: var(--a,!important))",
"(color: var(--a) !important !important)",
Expand All @@ -104,14 +107,11 @@
"(color: var(--",
"(color: var(--))",

"(--a: var(--b,))",
"(--a: var(--b,/**/))",
"(--a: var(--b,!))",
"(--a: var(--b,!important))",
"((--a: var(--b) !important !important))",
"(--a: var(--b,;))",
"(--a: var(--b);)",
"(--a:)",
"(--a: var(1px))",
"(--a: a))",
"(--a: \"\n",
Expand Down Expand Up @@ -150,13 +150,17 @@
["color", "var(--a, '"],
["color", "var(--a, '\\"],
["color", "var(--a, \\"],
["color", "var(--a,)"],
["color", "var(--a,/**/)"],

["--a", " var(--b)"],
["--a", "var(--b)"],
["--a", "var(--b) "],
["--a", "var( --b ) "],
["--a", "var(--b, )"],
["--a", "var(--b,/**/a)"],
["--a", "var(--b,)"],
["--a", "var(--b,/**/)"],
["--a", "1px var(--b)"],
["--a", "var(--b) 1px"],
["--a", "something 3px url(whereever) calc(var(--b) + 1px)"],
Expand All @@ -167,6 +171,7 @@
["--a", "{ [ var(--b) ] }"],
["--a", "[;] var(--b)"],
["--a", " "],
["--a", ""],
["--a", "var(--a)"],
["--0", "a"],
["--\\30", "a"],
Expand All @@ -187,8 +192,6 @@
];

var failingDeclarations = [
["color", "var(--a,)"],
["color", "var(--a,/**/)"],
["color", "var(--a,!)"],
["color", "var(--a,!important)"],
["color", "var(--a,;)"],
Expand All @@ -202,14 +205,11 @@
["color", "var(a)"],
["color", "var(--"],

["--a", "var(--b,)"],
["--a", "var(--b,/**/)"],
["--a", "var(--b,!)"],
["--a", "var(--b,!important)"],
["--a", "var(--b) !important !important"],
["--a", "var(--b,;)"],
["--a", "var(--b);"],
["--a", ""],
["--a", "var(1px)"],
["(VAR-a", "a"],
["--a", "a)"],
Expand Down
2 changes: 1 addition & 1 deletion layout/style/test/test_value_storage.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@
var value_has_variable_reference = resolved_value != null;
var is_system_font = property == "font" && value in gSystemFont;

var colon = value == "var(--a)" ? ":" : ": ";
var colon = ": ";
gDeclaration.setProperty(property, value, "");

var idx;
Expand Down
62 changes: 30 additions & 32 deletions layout/style/test/test_variable_serialization_computed.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,49 +13,47 @@
// its expected computed value.
var values = [
["", "--z", "an-inherited-value"],
["--a: ", "--a", " "],
["--a: ", "--a", ""],
["--a: initial", "--a", ""],
["--z: initial", "--z", ""],
["--a: inherit", "--a", ""],
["--z: inherit", "--z", "an-inherited-value"],
["--a: unset", "--a", ""],
["--z: unset", "--z", "an-inherited-value"],
["--a: 1px", "--a", " 1px"],
["--a: 1px", "--a", "1px"],
["--a: var(--a)", "--a", ""],
["--a: var(--b)", "--a", ""],
["--a: var(--b); --b: 1px", "--a", " 1px"],
["--a: var(--b, 1px)", "--a", " 1px"],
["--a: var(--b); --b: 1px", "--a", "1px"],
["--a: var(--b, 1px)", "--a", "1px"],
["--a: var(--a, 1px)", "--a", ""],
["--a: something 3px url(whereever) calc(var(--a) + 1px)", "--a", ""],
["--a: something 3px url(whereever) calc(var(--b,1em) + 1px)", "--a", " something 3px url(whereever) calc(1em + 1px)"],
["--a: var(--b, var(--c, var(--d, Black)))", "--a", " Black"],
["--a: a var(--b) c; --b:b", "--a", " a b c"],
["--a: a var(--b,b var(--c) d) e; --c:c", "--a", " a b c d e"],
["--a: var(--b)red; --b:orange;", "--a", " orange/**/red"],
["--a: var(--b)var(--c); --b:orange; --c:red;", "--a", " orange/**/red"],
["--a: var(--b)var(--c,red); --b:orange;", "--a", " orange/**/red"],
["--a: var(--b,orange)var(--c); --c:red;", "--a", " orange/**/red"],
["--a: var(--b)-; --b:-;", "--a", " -/**/-"],
["--a: var(--b)--; --b:-;", "--a", " -/**/--"],
["--a: var(--b)--x; --b:-;", "--a", " -/**/--x"],
["--a: var(--b)var(--c); --b:-; --c:-;", "--a", " -/**/-"],
["--a: var(--b)var(--c); --b:--; --c:-;", "--a", " --/**/-"],
["--a: var(--b)var(--c); --b:--x; --c:-;", "--a", " --x/**/-"],
["--a: something 3px url(whereever) calc(var(--b,1em) + 1px)", "--a", "something 3px url(whereever) calc(1em + 1px)"],
["--a: var(--b, var(--c, var(--d, Black)))", "--a", "Black"],
["--a: a var(--b) c; --b:b", "--a", "a b c"],
["--a: a var(--b,b var(--c) d) e; --c:c", "--a", "a b c d e"],
["--a: var(--b)red; --b:orange;", "--a", "orange/**/red"],
["--a: var(--b)var(--c); --b:orange; --c:red;", "--a", "orange/**/red"],
["--a: var(--b)var(--c,red); --b:orange;", "--a", "orange/**/red"],
["--a: var(--b,orange)var(--c); --c:red;", "--a", "orange/**/red"],
["--a: var(--b)-; --b:-;", "--a", "-/**/-"],
["--a: var(--b)--; --b:-;", "--a", "-/**/--"],
["--a: var(--b)--x; --b:-;", "--a", "-/**/--x"],
["--a: var(--b)var(--c); --b:-; --c:-;", "--a", "-/**/-"],
["--a: var(--b)var(--c); --b:--; --c:-;", "--a", "--/**/-"],
["--a: var(--b)var(--c); --b:--x; --c:-;", "--a", "--x/**/-"],
["counter-reset: var(--a)red; --a:orange;", "counter-reset", "orange 0 red 0"],
["--a: var(--b)var(--c); --c:[c]; --b:('ab", "--a", " ('ab')[c]"],
["--a: '", "--a", " ''"],
["--a: '\\", "--a", " ''"],
["--a: \\", "--a", " \\\ufffd"],
["--a: \"", "--a", " \"\""],
["--a: \"\\", "--a", " \"\""],
["--a: /* abc ", "--a", " /* abc */"],
["--a: /* abc *", "--a", " /* abc */"],
["--a: url(http://example.org/", "--a", " url(http://example.org/)"],
["--a: url(http://example.org/\\", "--a", " url(http://example.org/\\\ufffd)"],
["--a: url('http://example.org/", "--a", " url('http://example.org/')"],
["--a: url('http://example.org/\\", "--a", " url('http://example.org/')"],
["--a: url(\"http://example.org/", "--a", " url(\"http://example.org/\")"],
["--a: url(\"http://example.org/\\", "--a", " url(\"http://example.org/\")"]
["--a: var(--b)var(--c); --c:[c]; --b:('ab", "--a", "('ab')[c]"],
["--a: '", "--a", "''"],
["--a: '\\", "--a", "''"],
["--a: \\", "--a", "\\\ufffd"],
["--a: \"", "--a", "\"\""],
["--a: \"\\", "--a", "\"\""],
["--a: url(http://example.org/", "--a", "url(http://example.org/)"],
["--a: url(http://example.org/\\", "--a", "url(http://example.org/\\\ufffd)"],
["--a: url('http://example.org/", "--a", "url('http://example.org/')"],
["--a: url('http://example.org/\\", "--a", "url('http://example.org/')"],
["--a: url(\"http://example.org/", "--a", "url(\"http://example.org/\")"],
["--a: url(\"http://example.org/\\", "--a", "url(\"http://example.org/\")"]
];

function runTest() {
Expand Down
16 changes: 5 additions & 11 deletions servo/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 @@ -981,12 +975,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
42 changes: 9 additions & 33 deletions servo/components/style/properties/declaration_block.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1080,9 +1080,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 @@ -1094,14 +1094,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 @@ -1183,12 +1182,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 @@ -1213,7 +1207,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 @@ -1236,25 +1230,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 servo/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
3 changes: 0 additions & 3 deletions testing/web-platform/meta/css/css-syntax/declarations-trim-whitespace.html.ini
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
[declarations-trim-whitespace.html]
[--foo-2: bar;]
expected: FAIL

[--foo-3:bar ;]
expected: FAIL

Expand Down
3 changes: 0 additions & 3 deletions testing/web-platform/meta/css/css-variables/variable-cssText.html.ini
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,3 @@
[target6]
expected: FAIL

[target9]
expected: FAIL

Loading

0 comments on commit 488d886

Please sign in to comment.