Bug 1544886 - Allow CSS wide-keywords in custom property fallback. r=SimonSapin
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 23 Apr 2019 13:13:11 +0000
changeset 470483 1d1277b80e5c0a6accef9e80a111619036c13888
parent 470482 56f46d0eafe230c410e139ad51b2c596e18d14d5
child 470484 7d3f2f4c53beb19d3ccd4873a731cf15062170fc
push id35906
push useraciure@mozilla.com
push dateTue, 23 Apr 2019 22:14:56 +0000
treeherdermozilla-central@0ce3633f8b80 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersSimonSapin
bugs1544886
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1544886 - Allow CSS wide-keywords in custom property fallback. r=SimonSapin Differential Revision: https://phabricator.services.mozilla.com/D28349
servo/components/style/properties/properties.mako.rs
testing/web-platform/tests/css/css-variables/wide-keyword-fallback-ref.html
testing/web-platform/tests/css/css-variables/wide-keyword-fallback.html
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -1080,16 +1080,18 @@ impl LonghandId {
             iter: match *self {
                 % for property in data.longhands:
                     LonghandId::${property.camel_case} => ${property.ident.upper()},
                 % endfor
             }.iter(),
         }
     }
 
+    // TODO(emilio): Should we use a function table like CASCADE_PROPERTY does
+    // to avoid blowing up code-size here?
     fn parse_value<'i, 't>(
         &self,
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<PropertyDeclaration, ParseError<'i>> {
         match *self {
             % for property in data.longhands:
                 LonghandId::${property.camel_case} => {
@@ -1527,84 +1529,100 @@ impl ToCss for UnparsedValue {
 impl UnparsedValue {
     fn substitute_variables(
         &self,
         longhand_id: LonghandId,
         custom_properties: Option<<&Arc<crate::custom_properties::CustomPropertiesMap>>,
         quirks_mode: QuirksMode,
         environment: &::custom_properties::CssEnvironment,
     ) -> PropertyDeclaration {
-        crate::custom_properties::substitute(
-            &self.css,
-            self.first_token_type,
-            custom_properties,
-            environment,
-        ).ok().and_then(|css| {
-            // As of this writing, only the base URL is used for property
-            // values.
-            //
-            // NOTE(emilio): we intentionally pase `None` as the rule type here.
-            // If something starts depending on it, it's probably a bug, since
-            // it'd change how values are parsed depending on whether we're in a
-            // @keyframes rule or not, for example... So think twice about
-            // whether you want to do this!
-            //
-            // FIXME(emilio): ParsingMode is slightly fishy...
-            let context = ParserContext::new(
-                Origin::Author,
-                &self.url_data,
-                None,
-                ParsingMode::DEFAULT,
-                quirks_mode,
-                None,
-                None,
-            );
-
-            let mut input = ParserInput::new(&css);
-            Parser::new(&mut input).parse_entirely(|input| {
-                match self.from_shorthand {
-                    None => longhand_id.parse_value(&context, input),
-                    Some(ShorthandId::All) => {
-                        // No need to parse the 'all' shorthand as anything other than a CSS-wide
-                        // keyword, after variable substitution.
-                        Err(input.new_custom_error(SelectorParseErrorKind::UnexpectedIdent("all".into())))
-                    }
-                    % for shorthand in data.shorthands_except_all():
-                        Some(ShorthandId::${shorthand.camel_case}) => {
-                            shorthands::${shorthand.ident}::parse_value(&context, input)
-                            .map(|longhands| {
-                                match longhand_id {
-                                    % for property in shorthand.sub_properties:
-                                        LonghandId::${property.camel_case} => {
-                                            PropertyDeclaration::${property.camel_case}(
-                                                longhands.${property.ident}
-                                            )
-                                        }
-                                    % endfor
-                                    _ => unreachable!()
-                                }
-                            })
-                        }
-                    % endfor
-                }
-            })
-            .ok()
-        })
-        .unwrap_or_else(|| {
-            // Invalid at computed-value time.
+        let invalid_at_computed_value_time = || {
             let keyword = if longhand_id.inherited() {
                 CSSWideKeyword::Inherit
             } else {
                 CSSWideKeyword::Initial
             };
             PropertyDeclaration::CSSWideKeyword(WideKeywordDeclaration {
                 id: longhand_id,
                 keyword,
             })
-        })
+        };
+
+        let css = match crate::custom_properties::substitute(
+            &self.css,
+            self.first_token_type,
+            custom_properties,
+            environment,
+        ) {
+            Ok(css) => css,
+            Err(..) => return invalid_at_computed_value_time(),
+        };
+
+        // As of this writing, only the base URL is used for property
+        // values.
+        //
+        // NOTE(emilio): we intentionally pase `None` as the rule type here.
+        // If something starts depending on it, it's probably a bug, since
+        // it'd change how values are parsed depending on whether we're in a
+        // @keyframes rule or not, for example... So think twice about
+        // whether you want to do this!
+        //
+        // FIXME(emilio): ParsingMode is slightly fishy...
+        let context = ParserContext::new(
+            Origin::Author,
+            &self.url_data,
+            None,
+            ParsingMode::DEFAULT,
+            quirks_mode,
+            None,
+            None,
+        );
+
+        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.
+        if let Ok(keyword) = input.try(CSSWideKeyword::parse) {
+            return PropertyDeclaration::CSSWideKeyword(WideKeywordDeclaration {
+                id: longhand_id,
+                keyword,
+            });
+        }
+
+        let declaration = input.parse_entirely(|input| {
+            match self.from_shorthand {
+                None => longhand_id.parse_value(&context, input),
+                Some(ShorthandId::All) => {
+                    // No need to parse the 'all' shorthand as anything other
+                    // than a CSS-wide keyword, after variable substitution.
+                    Err(input.new_custom_error(SelectorParseErrorKind::UnexpectedIdent("all".into())))
+                }
+                % for shorthand in data.shorthands_except_all():
+                Some(ShorthandId::${shorthand.camel_case}) => {
+                    shorthands::${shorthand.ident}::parse_value(&context, input)
+                    .map(|longhands| {
+                        match longhand_id {
+                            % for property in shorthand.sub_properties:
+                                LonghandId::${property.camel_case} => {
+                                    PropertyDeclaration::${property.camel_case}(
+                                        longhands.${property.ident}
+                                    )
+                                }
+                            % endfor
+                            _ => unreachable!()
+                        }
+                    })
+                }
+                % endfor
+            }
+        });
+
+        match declaration {
+            Ok(decl) => decl,
+            Err(..) => invalid_at_computed_value_time(),
+        }
     }
 }
 
 /// An identifier for a given property declaration, which can be either a
 /// longhand or a custom property.
 #[derive(Clone, Copy, Debug, PartialEq)]
 #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
 pub enum PropertyDeclarationId<'a> {
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-variables/wide-keyword-fallback-ref.html
@@ -0,0 +1,14 @@
+<!doctype html>
+<title>CSS Test Reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<style>
+  #outer {
+    border: 10px solid transparent;
+  }
+
+  #inner {
+    border: 10px solid;
+  }
+</style>
+<div id="outer"><div id="inner"></div></div>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-variables/wide-keyword-fallback.html
@@ -0,0 +1,21 @@
+<!doctype html>
+<title>CSS Test: Wide keyword can be used as a fallback variable value</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<link rel="help" href="https://drafts.csswg.org/css-variables/#substitute-a-var">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1544886">
+<link rel="match" href="wide-keyword-fallback-ref.html">
+<style>
+  /* Should see a 10px border of the initial color */
+  #outer {
+    color: transparent;
+    border: 10px solid;
+  }
+
+  #inner {
+    color: var(--unknown, initial);
+    border-width: 10px;
+    border-style: var(--unknown, inherit);
+  }
+</style>
+<div id="outer"><div id="inner"></div></div>