Bug 1623396 - Custom properties with invalid variable references should be unset, not invalid. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 26 Mar 2020 11:34:12 +0000
changeset 520495 d49bad3acbc6c4a8eb3227b24f6bf18ae06c072e
parent 520494 c1b90d60baea8dda9cee5ea803303e01a72c5643
child 520496 0c696de70fd48512ff7cb84024c67112fc5c8a85
push id37252
push usermalexandru@mozilla.com
push dateThu, 26 Mar 2020 15:34:27 +0000
treeherdermozilla-central@31360ced8ff8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1623396, 1623347
milestone76.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 1623396 - Custom properties with invalid variable references should be unset, not invalid. r=heycam See https://github.com/w3c/csswg-drafts/issues/4075. There are tests that will get updated and this will make pass in bug 1623347. Differential Revision: https://phabricator.services.mozilla.com/D67373
servo/components/style/custom_properties.rs
testing/web-platform/meta/css/css-env/env-in-custom-properties.tentative.html.ini
testing/web-platform/meta/css/css-variables/variable-substitution-variable-declaration.html.ini
testing/web-platform/meta/css/css-variables/variables-substitute-guaranteed-invalid.html.ini
--- a/servo/components/style/custom_properties.rs
+++ b/servo/components/style/custom_properties.rs
@@ -574,19 +574,20 @@ impl<'a> CustomPropertiesBuilder<'a> {
                 // If the variable value has no references and it has an
                 // environment variable here, perform substitution here instead
                 // of forcing a full traversal in `substitute_all` afterwards.
                 let value = if !has_references && unparsed_value.references_environment {
                     let result = substitute_references_in_value(unparsed_value, &map, &self.device);
                     match result {
                         Ok(new_value) => Arc::new(new_value),
                         Err(..) => {
-                            map.remove(name);
+                            // Don't touch the map, this has the same effect as
+                            // making it compute to the inherited one.
                             return;
-                        },
+                        }
                     }
                 } else {
                     (*unparsed_value).clone()
                 };
                 map.insert(name.clone(), value);
             },
             CustomDeclarationValue::CSSWideKeyword(keyword) => match keyword {
                 CSSWideKeyword::Revert => {
@@ -648,26 +649,32 @@ impl<'a> CustomPropertiesBuilder<'a> {
     ///
     /// Otherwise, just use the inherited custom properties map.
     pub fn build(mut self) -> Option<Arc<CustomPropertiesMap>> {
         let mut map = match self.custom_properties.take() {
             Some(m) => m,
             None => return self.inherited.cloned(),
         };
         if self.may_have_cycles {
-            substitute_all(&mut map, self.device);
+            let inherited = self.inherited.as_ref().map(|m| &***m);
+            substitute_all(&mut map, inherited, self.device);
         }
         Some(Arc::new(map))
     }
 }
 
-/// Resolve all custom properties to either substituted or invalid.
+/// Resolve all custom properties to either substituted, invalid, or unset
+/// (meaning we should use the inherited value).
 ///
 /// It does cycle dependencies removal at the same time as substitution.
-fn substitute_all(custom_properties_map: &mut CustomPropertiesMap, device: &Device) {
+fn substitute_all(
+    custom_properties_map: &mut CustomPropertiesMap,
+    inherited: Option<&CustomPropertiesMap>,
+    device: &Device,
+) {
     // The cycle dependencies removal in this function is a variant
     // of Tarjan's algorithm. It is mostly based on the pseudo-code
     // listed in
     // https://en.wikipedia.org/w/index.php?
     // title=Tarjan%27s_strongly_connected_components_algorithm&oldid=801728495
 
     /// Struct recording necessary information for each variable.
     #[derive(Debug)]
@@ -693,16 +700,19 @@ fn substitute_all(custom_properties_map:
         /// The map from custom property name to its order index.
         index_map: PrecomputedHashMap<Name, usize>,
         /// Information of each variable indexed by the order index.
         var_info: SmallVec<[VarInfo; 5]>,
         /// The stack of order index of visited variables. It contains
         /// all unfinished strong connected components.
         stack: SmallVec<[usize; 5]>,
         map: &'a mut CustomPropertiesMap,
+        /// The inherited variables. We may need to restore some if we fail
+        /// substitution.
+        inherited: Option<&'a CustomPropertiesMap>,
         /// to resolve the environment to substitute `env()` variables.
         device: &'a Device,
     }
 
     /// This function combines the traversal for cycle removal and value
     /// substitution. It returns either a signal None if this variable
     /// has been fully resolved (to either having no reference or being
     /// marked invalid), or the order index for the given name.
@@ -826,44 +836,53 @@ fn substitute_all(custom_properties_map:
             in_loop = true;
         }
         if in_loop {
             // This variable is in loop. Resolve to invalid.
             context.map.remove(&name);
             return None;
         }
 
-        // Now we have shown that this variable is not in a loop, and
-        // all of its dependencies should have been resolved. We can
-        // start substitution now.
+        // Now we have shown that this variable is not in a loop, and all of its
+        // dependencies should have been resolved. We can start substitution
+        // now.
         let result = substitute_references_in_value(&value, &context.map, &context.device);
-
         match result {
             Ok(computed_value) => {
                 context.map.insert(name, Arc::new(computed_value));
-            },
+            }
             Err(..) => {
-                context.map.remove(&name);
-            },
+                // This is invalid, reset it to the unset (inherited) value.
+                let inherited = context.inherited.and_then(|m| m.get(&name)).cloned();
+                match inherited {
+                    Some(computed_value) => {
+                        context.map.insert(name, computed_value);
+                    },
+                    None => {
+                        context.map.remove(&name);
+                    },
+                };
+            }
         }
 
         // All resolved, so return the signal value.
         None
     }
 
     // We have to clone the names so that we can mutably borrow the map
     // in the context we create for traversal.
     let names: Vec<_> = custom_properties_map.keys().cloned().collect();
     for name in names.into_iter() {
         let mut context = Context {
             count: 0,
             index_map: PrecomputedHashMap::default(),
             stack: SmallVec::new(),
             var_info: SmallVec::new(),
             map: custom_properties_map,
+            inherited,
             device,
         };
         traverse(name, &mut context);
     }
 }
 
 /// Replace `var()` and `env()` functions in a pre-existing variable value.
 fn substitute_references_in_value<'i>(
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-env/env-in-custom-properties.tentative.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[env-in-custom-properties.tentative.html]
-  [Substitution of unrecognized env() causes unset]
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-variables/variable-substitution-variable-declaration.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[variable-substitution-variable-declaration.html]
-  [target10 --varC]
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-variables/variables-substitute-guaranteed-invalid.html.ini
+++ /dev/null
@@ -1,7 +0,0 @@
-[variables-substitute-guaranteed-invalid.html]
-  [A custom property referencing a cycle is treated as unset]
-    expected: FAIL
-
-  [A custom property referencing a non-existent variable is treated as unset]
-    expected: FAIL
-