Bug 1899272 - Defer computation of registered custom color properties if needed. r=dshin,firefox-style-system-reviewers,zrhoffman

This is a bit less complicated than lengths because there's no cycle
possible which could turn the color-scheme declaration invalid afaict.

So it's just that we need to defer the colors when color-scheme is
specified, which is slightly annoying, but maybe not too bad.

I had to tweak a bit the code to defer properties to fix a bug that we
were papering over accidentally. We were using the wrong registration
here:

  https://searchfox.org/mozilla-central/rev/f60bb10a5fe6936f9e9f9e8a90d52c18a0ffd818/servo/components/style/custom_properties.rs#1613

That's the registration for reference.name, not for name, which
papered over some issues. The fix is simple tho, which is storing a
single CustomPropertiesMap.

Differential Revision: https://phabricator.services.mozilla.com/D211860
This commit is contained in:
Emilio Cobos Álvarez 2024-05-29 14:19:02 +00:00
parent 6c1f865eac
commit 0c79149d0a
11 changed files with 287 additions and 160 deletions

View file

@ -11,10 +11,11 @@ use crate::custom_properties_map::CustomPropertiesMap;
use crate::media_queries::Device;
use crate::properties::{
CSSWideKeyword, CustomDeclaration, CustomDeclarationValue, LonghandId, LonghandIdSet,
VariableDeclaration,
PropertyDeclaration,
};
use crate::properties_and_values::{
registry::PropertyRegistrationData,
syntax::data_type::DependentDataTypes,
value::{
AllowComputationallyDependent, ComputedValue as ComputedRegisteredValue,
SpecifiedValue as SpecifiedRegisteredValue,
@ -338,9 +339,9 @@ bitflags! {
/// At least one custom property depends on root element's line height units.
const ROOT_LH_UNITS = 1 << 3;
/// All dependencies not depending on the root element.
const NON_ROOT_DEPENDENCIES = Self::FONT_UNITS.bits() | Self::LH_UNITS.bits();
const NON_ROOT_DEPENDENCIES = Self::FONT_UNITS.0 | Self::LH_UNITS.0;
/// All dependencies depending on the root element.
const ROOT_DEPENDENCIES = Self::ROOT_FONT_UNITS.bits() | Self::ROOT_LH_UNITS.bits();
const ROOT_DEPENDENCIES = Self::ROOT_FONT_UNITS.0 | Self::ROOT_LH_UNITS.0;
}
}
@ -458,14 +459,11 @@ impl References {
!self.refs.is_empty()
}
fn get_non_custom_dependencies(&self, is_root_element: bool) -> NonCustomReferences {
let mask = NonCustomReferences::NON_ROOT_DEPENDENCIES;
let mask = if is_root_element {
mask | NonCustomReferences::ROOT_DEPENDENCIES
} else {
mask
};
fn non_custom_references(&self, is_root_element: bool) -> NonCustomReferences {
let mut mask = NonCustomReferences::NON_ROOT_DEPENDENCIES;
if is_root_element {
mask |= NonCustomReferences::ROOT_DEPENDENCIES
}
self.non_custom_references & mask
}
}
@ -896,6 +894,7 @@ fn parse_declaration_value_block<'i, 't>(
pub struct CustomPropertiesBuilder<'a, 'b: 'a> {
seen: PrecomputedHashSet<&'a Name>,
may_have_cycles: bool,
has_color_scheme: bool,
custom_properties: ComputedCustomProperties,
reverted: PrecomputedHashMap<&'a Name, (CascadePriority, bool)>,
stylist: &'a Stylist,
@ -903,6 +902,28 @@ pub struct CustomPropertiesBuilder<'a, 'b: 'a> {
references_from_non_custom_properties: NonCustomReferenceMap<Vec<Name>>,
}
fn has_non_custom_dependency(
registration: &PropertyRegistrationData,
value: &VariableValue,
may_have_color_scheme: bool,
is_root_element: bool,
) -> bool {
let dependent_types = registration.syntax.dependent_types();
if dependent_types.is_empty() {
return false;
}
if dependent_types.intersects(DependentDataTypes::COLOR) && may_have_color_scheme {
return true;
}
if dependent_types.intersects(DependentDataTypes::LENGTH) {
let value_dependencies = value.references.non_custom_references(is_root_element);
if !value_dependencies.is_empty() {
return true;
}
}
false
}
impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
/// Create a new builder, inheriting from a given custom properties map.
///
@ -916,6 +937,7 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
seen: PrecomputedHashSet::default(),
reverted: Default::default(),
may_have_cycles: false,
has_color_scheme: false,
custom_properties,
stylist,
computed_context,
@ -973,23 +995,24 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
let registration = self.stylist.get_custom_property_registration(&name);
match value {
CustomDeclarationValue::Value(unparsed_value) => {
let has_custom_property_references = unparsed_value.references.any_var;
let registered_length_property =
registration.syntax.may_reference_font_relative_length();
// At this point of the cascade we're not guaranteed to have seen the color-scheme
// declaration, so need to assume the worst. We could track all system color
// keyword tokens + the light-dark() function, but that seems non-trivial /
// probably overkill.
let may_have_color_scheme = true;
// Non-custom dependency is really relevant for registered custom properties
// that require computed value of such dependencies.
let has_non_custom_dependencies = registered_length_property &&
!unparsed_value
.references
.get_non_custom_dependencies(self.computed_context.is_root_element())
.is_empty();
self.may_have_cycles |=
has_custom_property_references || has_non_custom_dependencies;
let has_dependency = unparsed_value.references.any_var ||
has_non_custom_dependency(
registration,
unparsed_value,
may_have_color_scheme,
self.computed_context.is_root_element(),
);
// If the variable value has no references to other properties, perform
// substitution here instead of forcing a full traversal in `substitute_all`
// afterwards.
if !has_custom_property_references && !has_non_custom_dependencies {
if !has_dependency {
return substitute_references_if_needed_and_apply(
name,
unparsed_value,
@ -998,6 +1021,7 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
self.computed_context,
);
}
self.may_have_cycles = true;
let value = ComputedRegisteredValue::universal(Arc::clone(unparsed_value));
map.insert(registration, name, value);
},
@ -1032,11 +1056,7 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
/// Note a non-custom property with variable reference that may in turn depend on that property.
/// e.g. `font-size` depending on a custom property that may be a registered property using `em`.
pub fn note_potentially_cyclic_non_custom_dependency(
&mut self,
id: LonghandId,
decl: &VariableDeclaration,
) {
pub fn maybe_note_non_custom_dependency(&mut self, id: LonghandId, decl: &PropertyDeclaration) {
// With unit algebra in `calc()`, references aren't limited to `font-size`.
// For example, `--foo: 100ex; font-weight: calc(var(--foo) / 1ex);`,
// or `--foo: 1em; zoom: calc(var(--foo) * 30px / 2em);`
@ -1055,13 +1075,20 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
NonCustomReferences::LH_UNITS | NonCustomReferences::FONT_UNITS
}
},
LonghandId::ColorScheme => {
// If we might change the color-scheme, we need to defer computation of colors.
self.has_color_scheme = true;
return;
},
_ => return,
};
let refs = match decl {
PropertyDeclaration::WithVariables(ref v) => &v.value.variable_value.references,
_ => return,
};
let refs = &decl.value.variable_value.references;
if !refs.any_var {
return;
}
let variables: Vec<Atom> = refs
.refs
.iter()
@ -1069,11 +1096,13 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
if !reference.is_var {
return None;
}
if !self
let registration = self
.stylist
.get_custom_property_registration(&reference.name)
.get_custom_property_registration(&reference.name);
if !registration
.syntax
.may_compute_length()
.dependent_types()
.intersects(DependentDataTypes::LENGTH)
{
return None;
}
@ -1087,7 +1116,7 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
if was_none {
return;
}
v.extend(variables.clone().into_iter());
v.extend(variables.iter().cloned());
});
}
@ -1198,17 +1227,18 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
pub fn build(
mut self,
defer: DeferFontRelativeCustomPropertyResolution,
) -> Option<ComputedCustomProperties> {
) -> Option<CustomPropertiesMap> {
let mut deferred_custom_properties = None;
if self.may_have_cycles {
if defer == DeferFontRelativeCustomPropertyResolution::Yes {
deferred_custom_properties = Some(ComputedCustomProperties::default());
deferred_custom_properties = Some(CustomPropertiesMap::default());
}
let mut invalid_non_custom_properties = LonghandIdSet::default();
substitute_all(
&mut self.custom_properties,
deferred_custom_properties.as_mut(),
&mut invalid_non_custom_properties,
self.has_color_scheme,
&self.seen,
&self.references_from_non_custom_properties,
self.stylist,
@ -1252,48 +1282,29 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
/// Fully resolve all deferred custom properties, assuming that the incoming context
/// has necessary properties resolved.
pub fn build_deferred(
deferred: ComputedCustomProperties,
deferred: CustomPropertiesMap,
stylist: &Stylist,
computed_context: &mut computed::Context,
) {
if deferred.is_empty() {
return;
}
// Guaranteed to not have cycles at this point.
let substitute =
|deferred: &CustomPropertiesMap,
stylist: &Stylist,
context: &computed::Context,
custom_properties: &mut ComputedCustomProperties| {
// Since `CustomPropertiesMap` preserves insertion order, we shouldn't
// have to worry about resolving in a wrong order.
for (k, v) in deferred.iter() {
let Some(v) = v else { continue };
let Some(v) = v.as_universal() else {
unreachable!("Computing should have been deferred!")
};
substitute_references_if_needed_and_apply(
k,
v,
custom_properties,
stylist,
context,
);
}
};
let mut custom_properties = std::mem::take(&mut computed_context.builder.custom_properties);
substitute(
&deferred.inherited,
stylist,
computed_context,
&mut custom_properties,
);
substitute(
&deferred.non_inherited,
stylist,
computed_context,
&mut custom_properties,
);
// Since `CustomPropertiesMap` preserves insertion order, we shouldn't have to worry about
// resolving in a wrong order.
for (k, v) in deferred.iter() {
let Some(v) = v else { continue };
let Some(v) = v.as_universal() else {
unreachable!("Computing should have been deferred!")
};
substitute_references_if_needed_and_apply(
k,
v,
&mut custom_properties,
stylist,
computed_context,
);
}
computed_context.builder.custom_properties = custom_properties;
}
}
@ -1304,8 +1315,9 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
/// It does cycle dependencies removal at the same time as substitution.
fn substitute_all(
custom_properties_map: &mut ComputedCustomProperties,
mut deferred_properties_map: Option<&mut ComputedCustomProperties>,
mut deferred_properties_map: Option<&mut CustomPropertiesMap>,
invalid_non_custom_properties: &mut LonghandIdSet,
has_color_scheme: bool,
seen: &PrecomputedHashSet<&Name>,
references_from_non_custom_properties: &NonCustomReferenceMap<Vec<Name>>,
stylist: &Stylist,
@ -1354,6 +1366,8 @@ fn substitute_all(
stack: SmallVec<[usize; 5]>,
/// References to non-custom properties in this strongly connected component.
non_custom_references: NonCustomReferences,
/// Whether the builder has seen a non-custom color-scheme reference.
has_color_scheme: bool,
map: &'a mut ComputedCustomProperties,
/// The stylist is used to get registered properties, and to resolve the environment to
/// substitute `env()` variables.
@ -1363,8 +1377,10 @@ fn substitute_all(
computed_context: &'a computed::Context<'b>,
/// Longhand IDs that became invalid due to dependency cycle(s).
invalid_non_custom_properties: &'a mut LonghandIdSet,
/// Properties that cannot yet be substituted.
deferred_properties: Option<&'a mut ComputedCustomProperties>,
/// Properties that cannot yet be substituted. Note we store both inherited and
/// non-inherited properties in the same map, since we need to make sure we iterate through
/// them in the right order.
deferred_properties: Option<&'a mut CustomPropertiesMap>,
}
/// This function combines the traversal for cycle removal and value
@ -1391,19 +1407,47 @@ fn substitute_all(
context: &mut Context<'a, 'b>,
) -> Option<usize> {
// Some shortcut checks.
let (value, should_substitute) = match var {
let value = match var {
VarType::Custom(ref name) => {
let registration = context.stylist.get_custom_property_registration(name);
let value = context.map.get(registration, name)?;
let value = value.as_universal()?;
let non_custom_references = value
.references
.get_non_custom_dependencies(context.computed_context.is_root_element());
let has_custom_property_reference = value.references.any_var;
let value = context.map.get(registration, name)?.as_universal()?;
let is_root = context.computed_context.is_root_element();
// We need to keep track of (potential) non-custom-references even on unregistered
// properties for cycle-detection purposes.
let value_non_custom_references = value.references.non_custom_references(is_root);
context.non_custom_references |= value_non_custom_references;
let has_dependency = value.references.any_var ||
!value_non_custom_references.is_empty() ||
has_non_custom_dependency(
registration,
value,
context.has_color_scheme,
is_root,
);
// Nothing to resolve.
if !has_custom_property_reference && non_custom_references.is_empty() {
if !has_dependency {
debug_assert!(!value.references.any_env, "Should've been handled earlier");
if !registration.syntax.is_universal() {
// We might still need to compute the value if this is not an universal
// registration if we thought this had a dependency before but turned out
// not to be (due to has_color_scheme, for example). Note that if this was
// already computed we would've bailed out in the as_universal() check.
debug_assert!(
registration
.syntax
.dependent_types()
.intersects(DependentDataTypes::COLOR),
"How did an unresolved value get here otherwise?",
);
let value = value.clone();
substitute_references_if_needed_and_apply(
name,
&value,
&mut context.map,
context.stylist,
context.computed_context,
);
}
return None;
}
@ -1416,11 +1460,10 @@ fn substitute_all(
entry.insert(context.count);
},
}
context.non_custom_references |= value.as_ref().references.non_custom_references;
// Hold a strong reference to the value so that we don't
// need to keep reference to context.map.
(Some(value.clone()), has_custom_property_reference)
Some(value.clone())
},
VarType::NonCustom(ref non_custom) => {
let entry = &mut context.non_custom_index_map[*non_custom];
@ -1428,7 +1471,7 @@ fn substitute_all(
return Some(*v);
}
*entry = Some(context.count);
(None, false)
None
},
};
@ -1586,47 +1629,37 @@ fn substitute_all(
if let Some(ref v) = value {
let registration = context.stylist.get_custom_property_registration(&name);
let registered_length_property =
registration.syntax.may_reference_font_relative_length();
let mut defer = false;
if !context.non_custom_references.is_empty() && registered_length_property {
if let Some(deferred) = &mut context.deferred_properties {
// This property directly depends on a non-custom property, defer resolving it.
let deferred_property = ComputedRegisteredValue::universal(Arc::clone(v));
deferred.insert(registration, &name, deferred_property);
if let Some(ref mut deferred) = context.deferred_properties {
// We need to defer this property if it has a non-custom property dependency, or
// any variable that it references is already deferred.
defer =
has_non_custom_dependency(
registration,
v,
context.has_color_scheme,
context.computed_context.is_root_element(),
) || v.references.refs.iter().any(|reference| {
reference.is_var && deferred.get(&reference.name).is_some()
});
if defer {
let value = ComputedRegisteredValue::universal(Arc::clone(v));
deferred.insert(&name, value);
context.map.remove(registration, &name);
defer = true;
}
}
if should_substitute && !defer {
for reference in v.references.refs.iter() {
if !reference.is_var {
continue;
}
if let Some(deferred) = &mut context.deferred_properties {
let registration = context
.stylist
.get_custom_property_registration(&reference.name);
if deferred.get(registration, &reference.name).is_some() {
// This property depends on a custom property that depends on a non-custom property, defer.
let deferred_property =
ComputedRegisteredValue::universal(Arc::clone(v));
deferred.insert(registration, &name, deferred_property);
context.map.remove(registration, &name);
defer = true;
break;
}
}
}
if !defer {
substitute_references_if_needed_and_apply(
&name,
v,
&mut context.map,
context.stylist,
context.computed_context,
);
}
// If there are no var references we should already be computed and substituted by now.
if !defer && v.references.any_var {
substitute_references_if_needed_and_apply(
&name,
v,
&mut context.map,
context.stylist,
context.computed_context,
);
}
}
context.non_custom_references = NonCustomReferences::default();
@ -1647,6 +1680,7 @@ fn substitute_all(
var_info: SmallVec::new(),
map: custom_properties_map,
non_custom_references: NonCustomReferences::default(),
has_color_scheme,
stylist,
computed_context,
invalid_non_custom_properties,

View file

@ -240,9 +240,7 @@ fn iter_declarations<'builder, 'decls: 'builder>(
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);
}
builder.maybe_note_non_custom_dependency(id, declaration);
}
}
}

View file

@ -62,7 +62,7 @@ PRIORITARY_PROPERTIES = set(
"font-stretch",
"font-style",
"font-family",
# color-scheme affects how system colors resolve.
# color-scheme affects how system colors and light-dark() resolve.
"color-scheme",
# forced-color-adjust affects whether colors are adjusted.
"forced-color-adjust",

View file

@ -8,6 +8,18 @@ use super::{Component, ComponentName, Multiplier};
use std::fmt::{self, Debug, Write};
use style_traits::{CssWriter, ToCss};
/// Some types (lengths and colors) depend on other properties to resolve correctly.
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, MallocSizeOf, ToShmem)]
pub struct DependentDataTypes(u8);
bitflags! {
impl DependentDataTypes: u8 {
/// <length> values depend on font-size/line-height/zoom...
const LENGTH = 1 << 0;
/// <color> values depend on color-scheme, etc..
const COLOR= 1 << 1;
}
}
/// <https://drafts.css-houdini.org/css-properties-values-api-1/#supported-names>
#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq)]
pub enum DataType {
@ -83,17 +95,16 @@ impl DataType {
})
}
/// Returns true if this data type requires deferring computation to properly
/// resolve font-dependent lengths.
pub fn may_reference_font_relative_length(&self) -> bool {
/// Returns which kinds of dependent data types this property might contain.
pub fn dependent_types(&self) -> DependentDataTypes {
match self {
DataType::Length |
DataType::LengthPercentage |
DataType::TransformFunction |
DataType::TransformList => true,
DataType::TransformList => DependentDataTypes::LENGTH,
DataType::Color => DependentDataTypes::COLOR,
DataType::Number |
DataType::Percentage |
DataType::Color |
DataType::Image |
DataType::Url |
DataType::Integer |
@ -101,7 +112,7 @@ impl DataType {
DataType::Time |
DataType::Resolution |
DataType::CustomIdent |
DataType::String => false,
DataType::String => DependentDataTypes::empty(),
}
}
}

View file

@ -17,7 +17,7 @@ use style_traits::{
StyleParseErrorKind, ToCss,
};
use self::data_type::DataType;
use self::data_type::{DataType, DependentDataTypes};
mod ascii;
pub mod data_type;
@ -94,35 +94,17 @@ impl Descriptor {
Ok(Self { components, specified })
}
/// Returns true if the syntax permits the value to be computed as a length.
pub fn may_compute_length(&self) -> bool {
/// Returns the dependent types this syntax might contain.
pub fn dependent_types(&self) -> DependentDataTypes {
let mut types = DependentDataTypes::empty();
for component in self.components.iter() {
match &component.name {
ComponentName::DataType(ref t) => {
if matches!(t, DataType::Length | DataType::LengthPercentage) {
return true;
}
},
ComponentName::Ident(_) => (),
let t = match &component.name {
ComponentName::DataType(ref t) => t,
ComponentName::Ident(_) => continue,
};
types.insert(t.dependent_types());
}
false
}
/// Returns true if the syntax requires deferring computation to properly
/// resolve font-dependent lengths.
pub fn may_reference_font_relative_length(&self) -> bool {
for component in self.components.iter() {
match &component.name {
ComponentName::DataType(ref t) => {
if t.may_reference_font_relative_length() {
return true;
}
},
ComponentName::Ident(_) => (),
};
}
false
types
}
}

View file

@ -0,0 +1,9 @@
<!DOCTYPE html>
<style>
div {
background-color: green;
width: 100px;
height: 100px;
}
</style>
<div></div>

View file

@ -0,0 +1,18 @@
<!DOCTYPE html>
<link rel="help" href="https://drafts.csswg.org/css-properties-values-api/#calculation-of-computed-values" />
<link rel="match" href="registered-property-computation-color-001-ref.html">
<style>
@property --x {
inherits: true;
initial-value: black;
syntax: "<color>";
}
div {
color-scheme: dark;
--x: light-dark(red, green);
background-color: var(--x);
width: 100px;
height: 100px;
}
</style>
<div></div>

View file

@ -0,0 +1,19 @@
<!DOCTYPE html>
<link rel="help" href="https://drafts.csswg.org/css-properties-values-api/#calculation-of-computed-values" />
<link rel="match" href="registered-property-computation-color-001-ref.html">
<style>
@property --x {
inherits: true;
initial-value: black;
syntax: "<color>";
}
div {
color-scheme: dark;
--x: light-dark(red, green);
--y: var(--x);
background-color: var(--y);
width: 100px;
height: 100px;
}
</style>
<div></div>

View file

@ -0,0 +1,10 @@
<!DOCTYPE html>
<style>
div {
color-scheme: dark;
background-color: Canvas;
width: 100px;
height: 100px;
}
</style>
<div></div>

View file

@ -0,0 +1,18 @@
<!DOCTYPE html>
<link rel="help" href="https://drafts.csswg.org/css-properties-values-api/#calculation-of-computed-values" />
<link rel="match" href="registered-property-computation-color-003-ref.html">
<style>
@property --x {
inherits: true;
initial-value: black;
syntax: "<color>";
}
div {
color-scheme: dark;
--x: Canvas;
background-color: var(--x);
width: 100px;
height: 100px;
}
</style>
<div></div>

View file

@ -0,0 +1,28 @@
<!DOCTYPE HTML>
<link rel="help" href="https://drafts.css-houdini.org/css-properties-values-api/#dom-css-registerproperty" />
<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1899272">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<div id=element></div>
<script>
test(function() {
CSS.registerProperty({
name: '--length-calc',
syntax: '<length>',
initialValue: '0px',
inherits: true
});
CSS.registerProperty({
name: '--length-calc-reset',
syntax: '<length>',
initialValue: '0px',
inherits: false
});
let element = document.getElementById("element");
element.style = 'font-size: 11px; --length-calc: calc(10em + 10px); --unregistered:var(--length-calc-reset); --length-calc-reset: var(--length-calc)';
let cs = getComputedStyle(element);
for (let prop of ["--length-calc", "--length-calc-reset", "--unregistered"]) {
assert_equals(cs.getPropertyValue(prop), "120px", "Should resolve properly: " + prop);
}
}, "Property dependency tracking across inherited and non-inherited properties");
</script>