1840478 - [css-properties-values-api] Handle non-inherited custom properties.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task)

Description

[Emilio Cobos Álvarez (:emilio)]

Once bug 1840476 lands, we have the registered custom property information in the Stylist, that we can look up.

Right now custom properties are all stored in a single CustomPropertiesMap, that gets inherited through. @property / registerProperty allows you to define non-inherited custom properties, so we need to split that into two, one that we inherit and one that we don't, and store properties appropriately.

Comment 1

[zsun]

Attachment: Bug 1840478 - Part1: Fetching inherits value of a CSS registered custom propertie from the Stylist. r=emilio

Comment 2

[Frédéric Wang (:fredw)]

Attachment: WIP: Bug 1840478 - Expose custom property registration per name.

Comment 3

[zsun]

Attachment: Bug 1840478 - Use stylist instead of device in some places. r=emilio

Comment 4

[zsun]

Attachment: Bug 1840478 - Introduce stylist as a memeber for stylebuilder. r=emilio

Comment 5

[zsun]

Attachment: Bug 1840478 - Fetching inherits value of a CSS registered custom propertie. r=emilio

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60c6149d4c4c
Introduce stylist as a memeber for stylebuilder. r=emilio,zrhoffman

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/60c6149d4c4c

Comment 8

[zsun]

Ah, this issue is not resolved yet. There will be more patches to follow. Shall we re-open it?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d251f88cec8
Fetching inherits value of a CSS registered custom propertie. r=emilio

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/7d251f88cec8

Comment 11

[Frédéric Wang (:fredw)]

Attachment: custom-property-test.html

Here is a basic testcase to check how the three kinds of properties (custom prop without inherit flag, custom prop with inherit flag and css variable) are initialized, inherited, set etc

Comment 12

[Frédéric Wang (:fredw)]

Attachment: WIP: Bug 1840478 - Handle non-inherited custom properties.

Comment 13

[Frédéric Wang (:fredw)]

@Emilio, @Zach:

Ziran and I have been experimenting the idea of two CustomPropertiesMap maps. Attachment 9352909 [details] is a preliminary patch for that and attachment 9352888 [details] contains basic test cases we are looking at. We are still working on the patch and we are currently investigating two points, which we thought would be worth asking you in case you can provide an answer more quickly:

1. What is actually the rationale for having two separate maps? It seems this is adding extra cost for the build() function (to merge the two maps) and is not necessary in the rest of the code (as one can always quickly know from the name whether a property is registered as inherited or not). Also, the CustomPropertiesMap named inherited currently always contains all computed custom property values of the parent and we really need this map to contain custom properties registered as non-inherited there too in order to implement the case --non-inherited-custom-property: inherit (see e.g. id=inherit2 in attachment 9352888 [details]).

2. In which part of the code do you think we should try to insert the "initial value"? Note this is a bit different from CSS variables since for custom properties that initial value is defined in the registration (instead of having a "guaranteed-invalid value"). The attached patch uses it for 'inital' and 'reset' for now. Our understanding is that this initial value should at least also be set on the root node for custom properties with the inherit flag, and on all nodes with unspecified values for custom properties without inherit flag. We have been experimenting inserting this value when initially building the self.(non_)inherited_custom_properties maps in the cascade function, but that only helps passing a few more tests while not working for the most basic cases (see e.g. id=root or id=unspecified2 in attachment 9352888 [details]).

Thanks in advance.

Comment 14

[Frédéric Wang (:fredw)]

OK, I just uploaded a quick dirty change, performing initialization of the custom properties on the root element using some code from a WIP patch by Ziran. With that attachment 9352888 [details] looks almost like Chromium (remaining case is unspecified2 which is handled by Ziran's WIP patch).

Comment 15

[Frédéric Wang (:fredw)]

Attachment: optimization-failing-001.html

Attached testcase demonstrate a case for https://searchfox.org/mozilla-central/rev/fa219b9c1d6c2c64aa139a848573caa03bc81f25/servo/components/style/custom_properties.rs#825

We cannot just return self.inherited since --non-inherited-custom-property should be the initial value 50%, not the inherited value 25%

Comment 16

[Frédéric Wang (:fredw)]

Attachment: optimization-failing-002.html

This is similar to attachment 9353221 [details], but targeting the optimization when no inherited custom properties are specified.

Comment 17

[Frédéric Wang (:fredw)]

@Emilio, @Zach: I updated the patch and proposed an answer for (2) of comment 13. I'm still not clear about question (1) though.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Frédéric Wang (:fredw) from comment #13)

@Emilio, @Zach:

Sorry for the lag, TPAC...

1. What is actually the rationale for having two separate maps?

You want inheriting to be cheap. So we want to differentiate between the inherited custom properties and not, so that inheriting from a style requires at most one reference count bump (Arc::clone in rust).

If you merge everything into a single map then inheriting becomes an expensive operation because you need to clone the map, keep the inherited properties and then remove the non-inherited ones, right?

2. In which part of the code do you think we should try to insert the "initial value"? Note this is a bit different from CSS variables since for custom properties that initial value is defined in the registration (instead of having a "guaranteed-invalid value"). The attached patch uses it for 'inital' and 'reset' for now. Our understanding is that this initial value should at least also be set on the root node for custom properties with the inherit flag, and on all nodes with unspecified values for custom properties without inherit flag. We have been experimenting inserting this value when initially building the self.(non_)inherited_custom_properties maps in the cascade function, but that only helps passing a few more tests while not working for the most basic cases (see e.g. id=root or id=unspecified2 in attachment 9352888 [details]).

That's tricky... The usual place where we keep the initial values is Device::default_values. But it's a bit yuck having to recompute that all the time we add a new @property rule or what not. Though then again we kinda need to, so maybe we can just do that? But I wonder if we can get away with not storing those initial values and just querying the stylist for those when we need to?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

If we store the initial values somewhere, it has to be Device::default_values, and not the root. Otherwise styles that don't inherit from the root like ::backdrop won't have the initial values.

Comment 20

[Frédéric Wang (:fredw)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

Sorry for the lag, TPAC...

No worries, we've been able to experiment more and understand things better.

1. What is actually the rationale for having two separate maps?

You want inheriting to be cheap. So we want to differentiate between the inherited custom properties and not, so that inheriting from a style requires at most one reference count bump (Arc::clone in rust).

If you merge everything into a single map then inheriting becomes an expensive operation because you need to clone the map, keep the inherited properties and then remove the non-inherited ones, right?

OK, I get it and I also realize existing optimizations to just reuse the inherited map don't work well in the current patch. The approach in the patch hasn't changed since Ziran's initial attempt which is just to have these maps:

    inherited_custom_properties: Option<CustomPropertiesMap> // the custom props on the node with inherit flag set to true, or classical CSS variables
    non_inherited_custom_properties: Option<CustomPropertiesMap>  // the custom props on the node with inherit flag set to false
    inherited: Option<&'a Arc<CustomPropertiesMap>>, // the computed style for custom props on the parent (whether or not they have the inherit flag)

With this setup, we still have to build inherited_custom_properties by extracting from inherited those with inherit flag set to true (or classical CSS variables), and then the build() function takes care of merging them into a single map, in order to use them later for var/env substitutions. So that's not really optimal.

As I mentioned, the cascade really need custom props with inherit flag set to false too in order to cover the case --non-inherited-custom-property: inherit. So maybe you meant to also split the inherited map into two maps too (according to whether inherit flag is set to true or false) so in general the first can just be a cheap clone of the corresponding inherited map while the second can just be None/empty (with the convention below for initial values)? An similarly the build() function and subsequent uses while maintain two separate maps (according to whether inherit flag is set to true or false)?

That's tricky... The usual place where we keep the initial values is Device::default_values. But it's a bit yuck having to recompute that all the time we add a new @property rule or what not. Though then again we kinda need to, so maybe we can just do that? But I wonder if we can get away with not storing those initial values and just querying the stylist for those when we need to?

OK, I believe we can use the convention no value == use the initial value (or the guaranteed invalid value if none is specified) and just query the stylist when needed. I'll give it a try.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

(In reply to Frédéric Wang (:fredw) from comment #20)

As I mentioned, the cascade really need custom props with inherit flag set to false too in order to cover the case --non-inherited-custom-property: inherit.

Ah, sorry if that was unclear. Of course, the style needs to keep both, and we need both custom properties map from the parent to inherit properly, indeed.

So maybe you meant to also split the inherited map into two maps too (according to whether inherit flag is set to true or false) so in general the first can just be a cheap clone of the corresponding inherited map while the second can just be None/empty (with the convention below for initial values)? An similarly the build() function and subsequent uses while maintain two separate maps (according to whether inherit flag is set to true or false)?

Yeah, the cleanest approach would be something like this in my opinion:

struct ComputedCustomProperties {
    inherited: Option<Arc<CustomPropertiesMap>>,
    non_inherited: Option<CustomPropertiesMap>,
}

CustomPropertiesBuilder etc would operate on that struct, effectively (it'd get a &'a ComputedCustomProperties from the parent style, and build a ComputedCustomProperties.

Comment 22

[Frédéric Wang (:fredw)]

Thanks for clarifying, let's iterate on the patch and see what we get. (btw wrapping in struct is what I had in mind too).

Comment 23

[Frédéric Wang (:fredw)]

Attachment: Bug 1840478 - Introduce ComputedCustomProperties struct to handle inherited/non-inherited properties. r=emilio

Comment 24

[Frédéric Wang (:fredw)]

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

Comment 25

[Frédéric Wang (:fredw)]

@Emilio: I've submitted https://phabricator.services.mozilla.com/D188727 for review, as per our discussion above. The second part https://phabricator.services.mozilla.com/D188812 is still in progress, as I need to properly handle initial values.

Comment 26

[Frédéric Wang (:fredw)]

@Emilio: Replying to your review comments here, as they are a bit related.

In layout/style/ServoBindings.toml, I initially tried to define mozilla::ServoComputedCustomProperties as a crate::custom_properties::ComputedCustomProperties.

This was making the https://searchfox.org/mozilla-central/source/servo/components/style/properties/properties.mako.rs#4189 fail, apparently because we are increasing the size of ComputedValues. So I went back to an Option<servo_arc::Arc<...>>, preserving the size to the one of a single pointer. Given that test, I assumed the style code is designed so that we use refcounted computed values and an Arc is still the way to go. Do you think this is actually not true?

Now, the existing code essentially works that way:

1. Create a new CustomPropertiesBuilder, initiating it with the computed properties of the inherited style.
2. Call the cascade for each specified property to generate the map.
3. Call the build function to take ownership of the generated map (or reuse the inherited one) which will be used as the computed property.
4. Call the substitution code to evaluate properties with css variables, passing the map returned in 3.

During steps 1-3, it seems to me that we are dealing with a single Arc so that seems fine.

For step 4, I'm not quite sure about the code. Some things worth noting:

• The substitution code is also called in step 2 (for env), step 3 (for cycle resolution) and I believe other places of the code.
• The substitution code can also edit the maps, I believe to fix cycle issues.
• The substitute function takes care of unwrapping the Arc for the computed property and other callers do something similar.

With the patch, we keep an Arc for ComputedCustomProperties (as explained for ServoBindings.toml) and an Arc for ComputedCustomProperties::inherited (as discussed on this bug) so that makes thing more confusing. I agree some of them are not necessary though, I'll try to follow your suggestion to see how that can be improve that.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

This was making the https://searchfox.org/mozilla-central/source/servo/components/style/properties/properties.mako.rs#4189 fail, apparently because we are increasing the size of ComputedValues. So I went back to an Option<servo_arc::Arc<...>>, preserving the size to the one of a single pointer. Given that test, I assumed the style code is designed so that we use refcounted computed values and an Arc is still the way to go. Do you think this is actually not true?

I think it's reasonable (if unfortunate) to grow the ComputedValues by one pointer (so bumping that to 240), see below.

Given that test, I assumed the style code is designed so that we use refcounted computed values and an Arc is still the way to go. Do you think this is actually not true?

The style code is designed to keep a bunch of reference-counted pointers to style structs, divided by inherited/non-inherited. Wrapping those in a useless arc is adding one completely useless indirection to the custom properties. Also there are use cases to copy the non-inherited properties (see copy_reset_from here). That having to allocate would be unfortunate.

So I'd rather make it work more similarly to how inherited / non-inherited structs work (e.g., nsStyleText and nsStyleTextReset are two pointers in the ComputedValues object) than trying to work around it in a weird way that also complicates the code and makes it slower.

Hopefully that makes sense? That should allow us to simplify the patch significantly.

Comment 28

[Frédéric Wang (:fredw)]

@Emilio: Thanks that makes totally sense! I'll give another try with the patch.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a7d9524a1b9
Introduce ComputedCustomProperties struct to handle inherited/non-inherited properties. r=emilio

Comment 30

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8a7d9524a1b9

Comment 31

[Frédéric Wang (:fredw)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #19)

If we store the initial values somewhere, it has to be Device::default_values, and not the root. Otherwise styles that don't inherit from the root like ::backdrop won't have the initial values.

So coming back to this, I see ComputedValues::default_values is called for Device::reset_computed_values and Device::new. I believe I need to pass the stylist to insert the initial values but my naive attempt to get it from the document lead to a MOZ_CRASH(already mutably borrowed) when trying to borrow the PerDocumentStyleData (and even if that borrow worked, it seems PerDocumentStyleData::new builds the Device before the stylist anyway). Any suggestion for how to do that?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

That's tricky... The usual place where we keep the initial values is Device::default_values. But it's a bit yuck having to recompute that all the time we add a new @property rule or what not. Though then again we kinda need to, so maybe we can just do that? But I wonder if we can get away with not storing those initial values and just querying the stylist for those when we need to?

Currently, I'm using the convention item absent from the map == initial value for custom properties registered as non-inherited and inherited value for custom properties that are inherited, which seems the best choice to keep the maps small. So I think we still need to set the initial values for the custom properties registered as inherited, but not for those registered as non-inherited.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Thinking more about comment 19... Even though ::backdrop doesn't inherit, I think the spec recently changed to that regard when it got moved from the fullscreen spec (here says it inherits now).

So probably the "stash initial values in the root's map" is good enough. That also has the nice side effect of @property initial valeus not affecting privileged AnonymousContent like the DevTools highlighters.

So TLDR I think your approach is probably alright.

Comment 33

[Frédéric Wang (:fredw)]

Attachment: custom-property-inheritance.html

This is like comment 11, fixing missing stuff for non-registered custom prop.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6453cd78e4f1
Handle inherits flag and initial value for custom properties. r=emilio,zrhoffman

Comment 35

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6453cd78e4f1