Bug 1713787 - Fix whitespace handling inside CSS variables. r=xidorn

As per w3c/csswg-drafts#881 and w3c/csswg-drafts#774. Differential Revision: https://phabricator.services.mozilla.com/D116459
jamienicol · Jun 6, 2021 · 8e37ea8 · 8e37ea8
1 parent 65cd921
commit 8e37ea8
Show file tree

Hide file tree

Showing 21 changed files with 69 additions and 141 deletions.
diff --git a/layout/style/test/test_css_supports_variables.html b/layout/style/test/test_css_supports_variables.html
@@ -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))",
@@ -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))",
@@ -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)",
@@ -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",
@@ -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)"],
@@ -167,6 +171,7 @@
     ["--a", "{ [ var(--b) ] }"],
     ["--a", "[;] var(--b)"],
     ["--a", " "],
+    ["--a", ""],
     ["--a", "var(--a)"],
     ["--0", "a"],
     ["--\\30", "a"],
@@ -187,8 +192,6 @@
   ];
 
   var failingDeclarations = [
-    ["color", "var(--a,)"],
-    ["color", "var(--a,/**/)"],
     ["color", "var(--a,!)"],
     ["color", "var(--a,!important)"],
     ["color", "var(--a,;)"],
@@ -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)"],

diff --git a/layout/style/test/test_value_storage.html b/layout/style/test/test_value_storage.html
@@ -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;

diff --git a/servo/components/style/custom_properties.rs b/servo/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(())
     })
 }
@@ -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(

diff --git a/servo/components/style/properties/declaration_block.rs b/servo/components/style/properties/declaration_block.rs
@@ -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)?;
@@ -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
+                        })
                     },
                 };
 
@@ -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.
@@ -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))
@@ -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)?;
 

diff --git a/servo/components/style/properties/properties.mako.rs b/servo/components/style/properties/properties.mako.rs
@@ -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;
         }
@@ -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)

diff --git a/testing/web-platform/meta/css/css-variables/variable-cssText.html.ini b/testing/web-platform/meta/css/css-variables/variable-cssText.html.ini
@@ -2,6 +2,3 @@
   [target6]
     expected: FAIL
 
-  [target9]
-    expected: FAIL
-
diff --git a/testing/web-platform/meta/css/css-variables/variable-invalidation.html.ini b/testing/web-platform/meta/css/css-variables/variable-invalidation.html.ini
diff --git a/testing/web-platform/tests/css/css-variables/css-variable-change-style-001.html b/testing/web-platform/tests/css/css-variables/css-variable-change-style-001.html
@@ -58,8 +58,8 @@
             initializeStyles();
             inner.style.cssText = testcase.property + ': var(--x)';
             testcase.values.forEach(function (value) {
-                outer.style.cssText = "--x:" + value.outer;
-                inbetween.style.cssText = "--x:" + value.inbetween;
+                outer.style.cssText = value.outer ? "--x:" + value.outer : "";
+                inbetween.style.cssText = value.inbetween ? "--x:" + value.inbetween : "";
                 let computedStyle = getComputedStyle(inner);
                 let actualValue = computedStyle.getPropertyValue(testcase.property);
                 assert_equals(actualValue, value.expected, value.Id);

diff --git a/testing/web-platform/tests/css/css-variables/variable-declaration-07.html b/testing/web-platform/tests/css/css-variables/variable-declaration-07.html
@@ -3,15 +3,15 @@
      http://creativecommons.org/publicdomain/zero/1.0/
 -->
 <!DOCTYPE html>
-<title>CSS Test: Test declaring a variable with invalid syntax due to a variable reference having no tokens in its fallback.</title>
+<title>CSS Test: Test declaring a variable with valid syntax due to a variable reference having no tokens in its fallback.</title>
 <link rel="author" title="Cameron McCormack" href="mailto:[email protected]">
 <link rel="help" href="http://www.w3.org/TR/css-variables-1/#syntax">
 <link rel="match" href="support/color-green-ref.html">
 <style>
 p {
   color: red;
-  --a: green;
-  --b: crimson;
+  --a: crimson;
+  --b: green;
   --a: var(--b,);
   color: var(--a);
 }

diff --git a/testing/web-platform/tests/css/css-variables/variable-declaration-09.html b/testing/web-platform/tests/css/css-variables/variable-declaration-09.html
@@ -3,15 +3,15 @@
      http://creativecommons.org/publicdomain/zero/1.0/
 -->
 <!DOCTYPE html>
-<title>CSS Test: Test declaring a variable with invalid syntax due to a variable reference having only a comment in its fallback.</title>
+<title>CSS Test: Test declaring a variable with a variable reference having only a comment in its fallback.</title>
 <link rel="author" title="Cameron McCormack" href="mailto:[email protected]">
 <link rel="help" href="http://www.w3.org/TR/css-variables-1/#syntax">
 <link rel="match" href="support/color-green-ref.html">
 <style>
 p {
   color: red;
-  --a: green;
-  --b: crimson;
+  --a: crimson;
+  --b: green;
   --a: var(--b,/**/);
   color: var(--a);
 }

diff --git a/testing/web-platform/tests/css/css-variables/variable-declaration-28.html b/testing/web-platform/tests/css/css-variables/variable-declaration-28.html