Bug 1855110: Part 2 - Track and mark dependency cycles between custom…

… properties and font-related properties. r=firefox-style-system-reviewers,emilio

Registered custom properties may utilize font-relative units such as `em`,
`ex`, etc. Font-related properties (More with `calc()` unit algebra), in
turn, may refer to such registered custom properties, leading to a cycle
(Note, unregistered properties are effectively copy-pastes, so it does not
suffer from this issue).

This patch:
1. Defers computation of registstered custom properties using font-relative
   units
2. Keeps track of custom properties utilizing font-relative units
3. Keeps track of non-custom, font-related properties making variable
   references
4. Expands the cycle detection to non-custom properties

Because of 1, this patch causes registered custom property using
font-relative units to resolve as if they're unregistered - this will be
addressed in the next patch.

Differential Revision: https://phabricator.services.mozilla.com/D196194

servo/components/style/properties/cascade.rs

@@ -237,6 +237,11 @@ fn iter_declarations<'builder, 'decls: 'builder>(
         } else {
             let id = declaration.id().as_longhand().unwrap();
             declarations.note_declaration(declaration, priority, id);
+            if let Some(ref mut builder) = custom_builder {
+                if let PropertyDeclaration::WithVariables(ref v) = declaration {
+                    builder.note_potentially_cyclic_non_custom_dependency(id, v);
+                }
+            }
         }
     }
 }
@@ -310,15 +315,20 @@ where
             LonghandIdSet::visited_dependent()
         },
         CascadeMode::Unvisited { visited_rules } => {
-            context.builder.custom_properties = {
+            (
+                context.builder.custom_properties,
+                context.builder.invalid_non_custom_properties,
+            ) = {
                 let mut builder = CustomPropertiesBuilder::new(stylist, &context);
                 iter_declarations(iter, &mut declarations, Some(&mut builder));
                 builder.build()
             };
 
+            // Resolve prioritary properties - Guaranteed to not fall into a cycle with existing custom
+            // properties.
             cascade.apply_prioritary_properties(&mut context, &declarations, &mut shorthand_cache);
 
-            if let Some(visited_rules) = visited_rules {
+           if let Some(visited_rules) = visited_rules {
                 cascade.compute_visited_style_if_needed(
                     &mut context,
                     element,

servo/components/style/properties/mod.rs

@@ -1310,7 +1310,7 @@ pub struct SourcePropertyDeclarationDrain<'a> {
 #[derive(Debug, Eq, PartialEq, ToShmem)]
 pub struct UnparsedValue {
     /// The variable value, references and so on.
-    variable_value: custom_properties::VariableValue,
+    pub(super) variable_value: custom_properties::VariableValue,
     /// The shorthand this came from.
     from_shorthand: Option<ShorthandId>,
 }
@@ -1355,6 +1355,14 @@ impl UnparsedValue {
             Cow::Owned(PropertyDeclaration::css_wide_keyword(longhand_id, keyword))
         };
 
+        if computed_context
+            .builder
+            .invalid_non_custom_properties
+            .contains(longhand_id)
+        {
+            return invalid_at_computed_value_time();
+        }
+
         if let Some(shorthand_id) = self.from_shorthand {
             let key = (shorthand_id, longhand_id);
             if shorthand_cache.contains_key(&key) {

servo/components/style/properties/properties.mako.rs

@@ -2324,6 +2324,11 @@ pub struct StyleBuilder<'a> {
     /// The computed custom properties.
     pub custom_properties: crate::custom_properties::ComputedCustomProperties,
 
+    /// Non-custom properties that are considered invalid at compute time
+    /// due to cyclic dependencies with custom properties.
+    /// e.g. `--foo: 1em; font-size: var(--foo)` where `--foo` is registered.
+    pub invalid_non_custom_properties: LonghandIdSet,
+
     /// The pseudo-element this style will represent.
     pub pseudo: Option<<&'a PseudoElement>,
 
@@ -2379,6 +2384,7 @@ impl<'a> StyleBuilder<'a> {
             modified_reset: false,
             is_root_element,
             custom_properties: crate::custom_properties::ComputedCustomProperties::default(),
+            invalid_non_custom_properties: LonghandIdSet::default(),
             writing_mode: inherited_style.writing_mode,
             effective_zoom: inherited_style.effective_zoom,
             flags: Cell::new(flags),
@@ -2417,6 +2423,7 @@ impl<'a> StyleBuilder<'a> {
             is_root_element: false,
             rules: None,
             custom_properties: style_to_derive_from.custom_properties().clone(),
+            invalid_non_custom_properties: LonghandIdSet::default(),
             writing_mode: style_to_derive_from.writing_mode,
             effective_zoom: style_to_derive_from.effective_zoom,
             flags: Cell::new(style_to_derive_from.flags),

servo/components/style/properties_and_values/syntax/mod.rs

@@ -87,6 +87,21 @@ impl Descriptor {
             specified,
         })
     }
+
+    /// Returns true if the syntax permits the value to be computed as a length.
+    pub fn may_compute_length(&self) -> bool {
+        for component in self.components.iter() {
+            match &component.name {
+                ComponentName::DataType(ref t) => {
+                    if matches!(t, DataType::Length | DataType::LengthPercentage) {
+                        return true;
+                    }
+                },
+                ComponentName::Ident(_) => (),
+            };
+        }
+        false
+    }
 }
 
 impl ToCss for Descriptor {

servo/components/style/values/specified/length.rs

@@ -45,7 +45,9 @@ pub const PX_PER_PT: CSSFloat = PX_PER_IN / 72.;
 /// Number of pixels per pica
 pub const PX_PER_PC: CSSFloat = PX_PER_PT * 12.;
 
-/// A font relative length.
+/// A font relative length. Note that if any new value is
+/// added here, `custom_properties::NonCustomReferences::from_unit`
+/// must also be updated. Consult the comment in that function as to why.
 #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToCss, ToShmem)]
 pub enum FontRelativeLength {
     /// A "em" value: https://drafts.csswg.org/css-values/#em
@@ -103,6 +105,23 @@ impl FontBaseSize {
 }
 
 impl FontRelativeLength {
+    /// Unit identifier for `em`.
+    pub const EM: &'static str = "em";
+    /// Unit identifier for `ex`.
+    pub const EX: &'static str = "ex";
+    /// Unit identifier for `ch`.
+    pub const CH: &'static str = "ch";
+    /// Unit identifier for `cap`.
+    pub const CAP: &'static str = "cap";
+    /// Unit identifier for `ic`.
+    pub const IC: &'static str = "ic";
+    /// Unit identifier for `rem`.
+    pub const REM: &'static str = "rem";
+    /// Unit identifier for `lh`.
+    pub const LH: &'static str = "lh";
+    /// Unit identifier for `rlh`.
+    pub const RLH: &'static str = "rlh";
+
     /// Return the unitless, raw value.
     fn unitless_value(&self) -> CSSFloat {
         match *self {
@@ -120,14 +139,14 @@ impl FontRelativeLength {
     // Return the unit, as a string.
     fn unit(&self) -> &'static str {
         match *self {
-            Self::Em(_) => "em",
-            Self::Ex(_) => "ex",
-            Self::Ch(_) => "ch",
-            Self::Cap(_) => "cap",
-            Self::Ic(_) => "ic",
-            Self::Rem(_) => "rem",
-            Self::Lh(_) => "lh",
-            Self::Rlh(_) => "rlh",
+            Self::Em(_) => Self::EM,
+            Self::Ex(_) => Self::EX,
+            Self::Ch(_) => Self::CH,
+            Self::Cap(_) => Self::CAP,
+            Self::Ic(_) => Self::IC,
+            Self::Rem(_) => Self::REM,
+            Self::Lh(_) => Self::LH,
+            Self::Rlh(_) => Self::RLH,
         }
     }
 

servo/ports/geckolib/glue.rs

@@ -6233,7 +6233,8 @@ pub extern "C" fn Servo_GetComputedKeyframeValues(
                 }
             }
             iter.reset();
-            builder.build()
+            let (result, _) = builder.build();
+            result
         };
 
         let mut property_index = 0;

testing/web-platform/meta/css/css-properties-values-api/registered-property-computation.html.ini

@@ -43,3 +43,9 @@
 
   [<length> values are computed correctly [10lh\]]
     expected: FAIL
+
+  [<length> values are computed correctly when font-size is inherited [14em\]]
+    expected: FAIL
+
+  [<length> values are computed correctly when font-size is inherited [calc(14em + 10px)\]]
+    expected: FAIL

testing/web-platform/meta/css/css-properties-values-api/unit-cycles.html.ini

@@ -1,51 +1,12 @@
 [unit-cycles.html]
-  [Lengths with em units may not be referenced from font-size]
+  [Inherited lengths with em units may be used]
     expected: FAIL
 
-  [Lengths with ex units may not be referenced from font-size]
+  [Inherited lengths with ex units may be used]
     expected: FAIL
 
-  [Lengths with ch units may not be referenced from font-size]
+  [Inherited lengths with ch units may be used]
     expected: FAIL
 
-  [Lengths with lh units may not be referenced from font-size]
-    expected: FAIL
-
-  [Lengths with rem units may not be referenced from font-size on root element]
-    expected: FAIL
-
-  [Lengths with lh units may not be referenced from line-height]
-    expected: FAIL
-
-  [Fallback may not use font-relative units]
-    expected: FAIL
-
-  [Fallback may not use line-height-relative units]
-    expected: FAIL
-
-  [Fallback not triggered while inside em unit cycle]
-    expected: FAIL
-
-  [Fallback not triggered while inside ch unit cycle]
-    expected: FAIL
-
-  [Fallback not triggered while inside rem unit cycle on root element]
-    expected: FAIL
-
-  [Fallback not triggered while inside lh unit cycle]
-    expected: FAIL
-
-  [Lengths with em units are detected via var references]
-    expected: FAIL
-
-  [Lengths with ex units are detected via var references]
-    expected: FAIL
-
-  [Lengths with ch units are detected via var references]
-    expected: FAIL
-
-  [Lengths with rem units are detected via var references]
-    expected: FAIL
-
-  [Lengths with lh units are detected via var references]
+  [Inherited lengths with lh units may be used]
     expected: FAIL

testing/web-platform/tests/css/css-properties-values-api/unit-cycles.html

@@ -4,15 +4,23 @@
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script>
-    function register_length(name, inherits=false) {
+    function register_property(name, syntax, inherits) {
         CSS.registerProperty({
             name: name,
-            syntax: '<length>',
+            syntax: syntax,
             initialValue: '0px',
             inherits: inherits
         });
     }
 
+    function register_length(name, inherits=false) {
+        register_property(name, '<length>', inherits);
+    }
+
+    function register_universal(name) {
+        register_property(name, '*', false);
+    }
+
     register_length('--font-size-em');
     register_length('--font-size-rem');
     register_length('--font-size-ex');
@@ -29,6 +37,12 @@
     register_length('--line-height-lh');
     register_length('--line-height-lh-via-var');
     register_length('--line-height-lh-inherited', true);
+    register_universal('--font-size-em-universal');
+    register_universal('--font-size-rem-universal');
+    register_universal('--font-size-ex-universal');
+    register_universal('--font-size-ch-universal');
+    register_universal('--font-size-px-universal');
+    register_universal('--font-size-lh-universal');
 </script>
 <style>
     :root {
@@ -52,6 +66,12 @@
         --font-size-ex-via-var: var(--unregistered-ex);
         --font-size-ch-via-var: var(--unregistered-ch);
         --line-height-lh-via-var: var(--unregistered-lh);
+        --font-size-em-universal: 2em;
+        --font-size-rem-universal: 2rem;
+        --font-size-ex-universal: 2ex;
+        --font-size-ch-universal: 2ch;
+        --font-size-px-universal: 42px;
+        --font-size-lh-universal: 2lh;
     }
 
     #parent {
@@ -79,14 +99,22 @@
         element.style.marginBottom = '';
     }
 
+    function compute_margin_bottom(dimension, element) {
+        try {
+            element.style.marginBottom = dimension;
+            return getComputedStyle(element).marginBottom;
+        } finally {
+            element.style.marginBottom = '';
+        }
+    }
+
     // Compute a dimension (e.g. 1em) given a certain fontSize.
     function compute_dimension(dimension, fontSize, element = ref) {
         try {
             element.style.fontSize = fontSize;
-            element.style.marginBottom = dimension;
-            return getComputedStyle(element).marginBottom;
+            return compute_margin_bottom(dimension, element);
         } finally {
-            unset_dimension(element);
+            element.style.fontSize = '';
         }
     }
 
@@ -103,6 +131,41 @@
         unset_dimension(document.documentElement);
     });
 
+    test(function() {
+        // Unregistered variables behave as if they're copy & paste, so
+        // follow the regular font-relative size resolution.
+        const unregistered_variables = {
+            'unregistered-em': target.parentElement,
+            'unregistered-ex': target.parentElement,
+            'unregistered-ch': target.parentElement,
+            'unregistered-lh': target.parentElement,
+            'unregistered-rem': document.documentElement,
+        };
+
+        for (let variable in unregistered_variables) {
+            const e = unregistered_variables[variable];
+            const v = `var(--${variable})`;
+            target.style = `font-size: ${v};`;
+            assert_property_equals('font-size', compute_margin_bottom(v, e));
+        }
+    }, 'Font-dependent unregistered variables can be used in font-size');
+
+    test(function() {
+        const universal_variables = {
+            'font-size-em-universal': (v) => compute_dimension(v, 'unset', target),
+            'font-size-ex-universal': (v) => compute_dimension(v, 'unset', target),
+            'font-size-ch-universal': (v) => compute_dimension(v, 'unset', target),
+            'font-size-lh-universal': (v) => compute_margin_bottom('2lh', target.parentElement),
+            'font-size-rem-universal': (v) => compute_dimension(v, 'unset', target),
+        };
+        for (let variable in universal_variables) {
+            const v = `var(--${variable})`;
+            const expected = universal_variables[variable](v);
+            target.style = `font-size: ${v};`;
+            assert_property_equals('font-size', expected);
+        }
+    }, 'Registered universal variables referencing font-dependent units can be used in font-size');
+
     test(function() {
         target.style = 'font-size: var(--font-size-px);';
         assert_property_equals('font-size', '42px');