Page MenuHomePhabricator
Differential D188812

Bug 1840478 - Handle inherits flag and initial value for custom properties. r=emilio,zrhoffman
ClosedPublic
Actions

Authored by fredw on Sep 21 2023, 11:03 AM.
Tags
Referenced Files
Unknown Object (File)
Fri, Apr 10, 9:32 PM
Unknown Object (File)
Fri, Apr 10, 9:32 PM
Unknown Object (File)
Wed, Apr 8, 3:38 AM
Unknown Object (File)
Nov 13 2025, 12:01 PM
Unknown Object (File)
Nov 13 2025, 12:01 PM
Unknown Object (File)
Nov 13 2025, 12:01 PM
Unknown Object (File)
Nov 13 2025, 12:01 PM
Unknown Object (File)
Nov 13 2025, 12:01 PM
View All Files
Subscribers
zrhoffman

Details

Reviewers
emilio
zrhoffman
Commits
rMOZILLACENTRAL6453cd78e4f1: Bug 1840478 - Handle inherits flag and initial value for custom properties.
Bugzilla Bug ID
1840478
Summary

The CSS Properties and Values API allows to register CSS properties that
can be either inherited or non-inherited, and which can also have
initial values specified [1].

In [2], the representation of computed value for custom properties was
changed to a pair of CustomPropertiesMaps: one map which is ref-counted
(for properties unregistered or registered as inherited) and one which
is not (for properties registered as non-inherited). The latter map
is currently always None (i.e. all custom properties are handled as
inherited) and several parts of the code assume this condition holds.

This patch instead ensures that values for custom properties on a node
are properly placed in the inherit or non_inherit map according to the
inherits flag. Initial values for registered properties are taken
into account and missing implementations of functions assuming
non_inherited==None is completed.

In order to minimize the size of the maps, absent values for
non-inherited properties are interpreted as initial values during var
substitution or retrieval of computed values (for non-inherited
properties). This is used for unset and initial keywords while
a copy of the parent's value is used for inherit.

Last but not least, CustomPropertiesBuilder tries to perform lazy
copy of the inherited style, postponing actual deep clone when
necessary or falling back to a shallow copy when possible [3].
This is generalized a bit when the document contains custom properties
registered as non-inherited and current optimizations are preserved as
is for pages containing only non-registered custom properties. This
could be further improved later [4].

[1] https://drafts.css-houdini.org/css-properties-values-api-1/
[2] https://hg.mozilla.org/mozilla-central/rev/8a7d9524a1b9
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1840478
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=1855887

Test Plan

Try server: https://treeherder.mozilla.org/jobs?repo=try&revision=fe3ffc9c03bee4bd2709314f01c52e0cf755630d

Some of these changes are covered by WPT tests. More tests will be added in the future while the implementation is refined.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
fredw planned changes to this revision.Sep 27 2023, 8:48 AM
fredw updated this revision to Diff 768410.
Harbormaster completed remote builds in B581308: Diff 768410.Sep 27 2023, 8:48 AM
fredw planned changes to this revision.Sep 27 2023, 12:33 PM
fredw updated this revision to Diff 768493.
Harbormaster completed remote builds in B581423: Diff 768493.Sep 27 2023, 12:33 PM
fredw planned changes to this revision.Sep 27 2023, 1:17 PM
fredw updated this revision to Diff 768511.
Harbormaster completed remote builds in B581436: Diff 768511.Sep 27 2023, 1:17 PM
fredw edited the test plan for this revision. (Show Details)Sep 27 2023, 1:31 PM
fredw requested review of this revision.Sep 27 2023, 2:20 PM
fredw updated this revision to Diff 768537.
fredw retitled this revision from WIP: Bug 1840478 - Handle non-inherited custom properties. r=emilio to Bug 1840478 - Handle inherits flag and initial value for custom properties. r=emilio,zrhoffman.
fredw edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B581468: Diff 768537.Sep 27 2023, 2:20 PM
fredw added a reviewer: zrhoffman.Sep 27 2023, 2:21 PM

@zach, @emilio: This patch is now properly working with my basic test on bug 1840478 and it increases WPT pass rate. There is probably more fine-tuning to do and tests to add, but I believe it's ready for a preliminary review.

fredw edited the summary of this revision. (Show Details)Sep 27 2023, 2:25 PM
emilio requested changes to this revision.Sep 27 2023, 3:55 PM
emilio added inline comments.
servo/components/style/custom_properties.rs
285–286

Let's factor this into its own function. Specially since we eventually are going to need inherited_equal and non_inherited_equal for invalidation.

So something like:

fn maps_equal(l: Option<&CustomPropertiesMap>, r: Option<&CustomPropertiesMap>) -> bool {
    match (l, r) { ... }
}

impl ComputedCustomProperties {
    fn inherited_equal(&self, other: &Self) -> bool {
        maps_equal(self.inherited.as_deref(), other.inherited.as_deref())
    }
    fn non_inherited_equal(&self, other: &Self) -> bool {
        maps_equal(self.non_inherited.as_deref(), other.non_inherited.as_deref());
    }
}

impl PartialEq for ComputedCustomProperties {
    fn eq(&self, other: &Self) -> bool {
        self.inherited_equal(other) && self.non_inherited_equal(other)
    }
}
839

let's debug_assert!(inherited.is_empty()) or so?

840

stylist.get_custom_property_initial_values().map(UniqueArc::new))

842

We don't want to clone the inherited properties unconditionally. That defeats the whole point of reference-counting them.

1051

How can this case happen? The non-inherited map starts off always empty right?

1137

This case seems somewhat silly right? Can't we just take the path under us?

servo/components/style/properties_and_values/registry.rs
45

Let's not clone the map and just return a reference?

servo/components/style/stylist.rs
713

Looking at the first value is wrong. You want to look at the last one which is the one that wins right?

715

This does two lookups. Use map.entry instead?

But this doesn't seem correct if you have a script registration without an initial value and a CSS registration with an initial value, right?

servo/ports/geckolib/glue.rs
7435

nit: useless braces.

7462–7463

What's the behavior for non-inherited properties that have an initial value? Shouldn't we include them here? Maybe separate bug.

This revision now requires changes to proceed.Sep 27 2023, 3:55 PM
fredw planned changes to this revision.Sep 28 2023, 7:15 AM
fredw updated this revision to Diff 768931.
fredw retitled this revision from Bug 1840478 - Handle inherits flag and initial value for custom properties. r=emilio,zrhoffman to WIP: Bug 1840478 - Handle inherits flag and initial value for custom properties. r=emilio,zrhoffman.
fredw edited the summary of this revision. (Show Details)
fredw marked 8 inline comments as done.
Harbormaster completed remote builds in B581791: Diff 768931.Sep 28 2023, 7:15 AM
fredw added inline comments.Sep 28 2023, 7:16 AM
servo/components/style/custom_properties.rs
1137

Compared to existing code, I think doing a shrink_to_fit below on the inherited map might lead to a reallocation... But anyway I changed to only shrink the non_inherited map and other operations are cheap, so I can just use the path indeed.

servo/ports/geckolib/glue.rs
7462–7463

Yes, I think you are right we should probably include them... I'm still not sure what would be the proper order. I moved this to bug 1855629.

fredw planned changes to this revision.Sep 28 2023, 9:04 AM
fredw updated this revision to Diff 768981.
Harbormaster completed remote builds in B581829: Diff 768981.Sep 28 2023, 9:04 AM
fredw added inline comments.Sep 28 2023, 9:05 AM
servo/components/style/custom_properties.rs
1051

Right, so here existing_value is always the initial value (if any) and we want to check it's not the same as in inherited.non_inherited. But here that actually means checking whether the name is absent from the inherited.non_inherited map, instead of trying to get retrieve the inherited value. Fixed.

fredw marked an inline comment as done.Sep 28 2023, 9:05 AM
fredw requested review of this revision.Sep 28 2023, 9:57 AM
fredw updated this revision to Diff 769002.
fredw retitled this revision from WIP: Bug 1840478 - Handle inherits flag and initial value for custom properties. r=emilio,zrhoffman to Bug 1840478 - Handle inherits flag and initial value for custom properties. r=emilio,zrhoffman.
Harbormaster completed remote builds in B581870: Diff 769002.Sep 28 2023, 9:57 AM
fredw marked 2 inline comments as done.Sep 28 2023, 9:59 AM
fredw added inline comments.
servo/components/style/stylist.rs
715

OK I added a HashSet to check whether the a name was already seen in a previous registration.

emilio requested changes to this revision.Sep 28 2023, 11:15 AM
emilio added inline comments.
servo/components/style/custom_properties.rs
913

Maybe we should pass the registration directly to remove / insert etc, to avoid the extra lookups. Follow-up perhaps?

1111

We don't need to do a deep clone here. We should just reuse our inherited properties from our parent.

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

nit: indentation, please

servo/components/style/stylist.rs
702–703

This should use a PrecomputedHashSet, since names have their hashes precomputed.

707

We need to do this even if the property doesn't inherit right? Think of having a script property with inherits: false but then an @property with inherits: true. Please add a test for this.

715

&v.last().unwrap().0 probably rather than messing with v.len()?

servo/ports/geckolib/glue.rs
7468

Let's add a property_at(&self, index: usize) -> Option<(&Name, &Value)> to ComputedCustomProperties? Then this would do:

match self.property_at(index as usize) {
    Some((name, _value)) => name.as_ptr(),
    None => ptr::null_mut(),
}
This revision now requires changes to proceed.Sep 28 2023, 11:15 AM
fredw requested review of this revision.Sep 28 2023, 1:56 PM
fredw updated this revision to Diff 769114.
fredw marked 7 inline comments as done.
Harbormaster completed remote builds in B581958: Diff 769114.Sep 28 2023, 1:56 PM
emilio accepted this revision.Sep 28 2023, 4:48 PM
emilio added a project: testing-approved.

Okay, I think this is progress. There are some rough edges we should fix up, but I don't object to this landing for now. Thanks for your work on this @fwang!

servo/components/style/custom_properties.rs
260

You could do this like:

(Some(map), None) | (None, Some(map)) => map.get_index(index),

Or so.

886–887

n

1445

This doesn't seem right. If non-inherited, we should look at the non-inherited map, right?

So this should be inherited.read().get(...) or so?

But we can fix it up in a follow-up bug if you want.

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

no need for as_ref, right? Just &self.custom_properties.

This revision is now accepted and ready to land.Sep 28 2023, 4:48 PM
zrhoffman accepted this revision.Sep 29 2023, 3:32 AM

The new failures in css/css-properties-values-api/var-reference-registered-properties.html look like they can be resolved in bug 1855629.

r+ with comments addressed. Thanks for all of your work on this, @fredw!

servo/components/style/custom_properties.rs
353

Nit: CustomPropertiesMap::default() can be Default::default()

356

Box::new(CustomPropertiesMap::default()) can be Default::default()

938

Instead of .as_ref(),

Some(ref initial_value) = registration.initial_value
1427

Instead of .as_ref(),

Some(ref initial_value) = registration.initial_value
servo/components/style/stylist.rs
704

CustomPropertiesMap::default() seems more readable here than Default::defaul(), what do you think?

718
Some(value) = &last_value.initial_value

can be

Some(ref value) = last_value.initial_value
fredw added inline comments.Sep 29 2023, 6:33 AM
servo/components/style/custom_properties.rs
886–887

I'm not sure what's that comment about.

Anyway, it seems I forgot to submit my comments yesterday. I think we might get rid of this, I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1855887 as a follow-up.

913

I thought about it too, so let's do it.

1111

OK, I can made self.inherited_properties_match return true if self.custom_properties.inherited is None so that the optimization below is re-used.

However, I'm not quite sure about substitute_all is correct as it is now. When accessing a value, it seems to me it should use proper fallback (either inherited from the parent or initial value depending on the inherits flag). And when inserting the first value (self.custom_properties.inherited` still None), it seems to be it should do a deep clone of the inherited map first. Probably need to be refined later.

1111

It seems I forgot to send this message. Anyway, I think I should restore the deep clone here for now. I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1855887 as a follow-up.

servo/components/style/stylist.rs
707

Done but still need to add a test.

fredw updated this revision to Diff 769493.Sep 29 2023, 7:13 AM
fredw marked 11 inline comments as done.
Harbormaster completed remote builds in B582249: Diff 769493.Sep 29 2023, 7:13 AM
fredw added inline comments.Sep 29 2023, 7:16 AM
servo/components/style/custom_properties.rs
260

Unfortunately there is a type mismatch here:

(Some(map), None) | (None, Some(map)) => map.get_index(index),
                                ^^^ expected `&Arc<IndexMap<Atom, Arc<...>, ...>>`, found `&Box<IndexMap<Atom, Arc<...>, ...>>`
1445

You are right, fixed. This is adding another query to stylist.get_custom_property_registration. Maybe this should accept an Option<&PropertyRegistration> like get_map...

I'm also not clear why current code insert the inherited value (for inherited prop), but I'm keeping as is for now. I copied the same logic for the initial value, but again I'm not sure that's needed (for non-inherited prop). I added TODOs.

servo/components/style/stylist.rs
704

Right. Also that makes type inference fail when try to insert into the map, so let's keep it :-)

fredw edited the test plan for this revision. (Show Details)Sep 29 2023, 8:00 AM
fredw edited the summary of this revision. (Show Details)Sep 29 2023, 8:31 AM
Closed by commit rMOZILLACENTRAL6453cd78e4f1: Bug 1840478 - Handle inherits flag and initial value for custom properties. (authored by fredw). · Explain WhySep 29 2023, 9:07 AM
This revision was automatically updated to reflect the committed changes.
fredw added a commit: rMOZILLACENTRAL6453cd78e4f1: Bug 1840478 - Handle inherits flag and initial value for custom properties..

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
layout/
style/
3 lines86%
Loading...
servo/
components/
style/
411 lines93%
Loading...
properties/
2 lines97%
Loading...
2 lines87%
Loading...
15 lines--
properties_and_values/
6 lines29%
Loading...
34 lines89%
Loading...
ports/
geckolib/
55 lines94%
Loading...
testing/
web-platform/
meta/
css/
css-cascade/
5 lines--
11 lines--
css-properties-values-api/
--
animation/
3 lines--
3 lines--
3 lines--
3 lines--
--
--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
3 lines--
--
--
57 lines--
28 lines--
6 lines--
5 lines--
--
3 lines--
17 lines--
3 lines--
--
8 lines--
21 lines--
css-pseudo/
--

Diff 769531

View Options

layout/style/nsComputedDOMStyle.cpp

Loading...
View Options

servo/components/style/custom_properties.rs

Loading...
View Options

servo/components/style/properties/cascade.rs

Loading...
View Options

servo/components/style/properties/declaration_block.rs

Loading...
View Options

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

Loading...
View Options

servo/components/style/properties_and_values/registry.rs

Loading...
View Options

servo/components/style/stylist.rs

Loading...
View Options

servo/ports/geckolib/glue.rs

Loading...
View Options

testing/web-platform/meta/css/css-cascade/layer-cssom-order-reverse-at-property.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-cascade/layer-property-override.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animate-invalid.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-angle-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-angle-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-color-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-color-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-custom-ident.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-image.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-integer-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-integer-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-length-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-length-percentage-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-length-percentage-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-length-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-number-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-number-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-percentage-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-percentage-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-resolution-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-resolution-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-time-comma-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-time-space-list.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/animation/custom-property-animation-url.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/at-property-stylesheets.html.ini

Loading...
View Options

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

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/determine-registration.html.ini

Loading...
View Options

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

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/registered-property-change-style-001.html.ini

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/self-utils.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/var-reference-registered-properties-cycles.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-properties-values-api/var-reference-registered-properties.html.ini

Loading...
View Options

testing/web-platform/meta/css/css-pseudo/highlight-cascade-004.html.ini

Loading...
Privacy · Cookies · Legal