Details

Reviewers

Commits

rMOZILLACENTRAL4556cc6bbefb: Bug 1855629 - Handling of custom properties when enumerating getComputedStyle().

Bugzilla Bug ID

Summary

After bug 1840478, non-inherited custom properties using their initial
values are represented as absent from ComputedCustomProperties in
order to save memory. Wherever the values of such properties are
requested, it is necessary to fallback to any registered initial value.
However, this makes difficult to properly enumerate custom properties
for exposure via the CSSStyleDeclaration.item() API and indeed our
current implementation only exposes the properties actually present in
ComputedCustomProperties.

Additionally, such a representation conflicts with pre-existent
representation of guaranteed-invalid values as absent values, causing
some issues e.g. bad handling of invalid at computed-value time [1] [2].

This patch changes ComputedCustomProperties so that registered initial
values are always stored in the non_inherited map, immediately fixing
the issue with CSSStyleDeclaration.item() and preparing follow-up
work on guaranteed-invalid values.

To avoid excessive increase of memory usage, the non_inherited map
becomes ref-counted. The associated Stylist contains an up-to-date
ComputedCustomProperties with registered custom properties that have
initial values, and the non_inherited map can generally just be
shallow-cloned from it.

A new test get-computed-style-enumeration.html is added to make sure
custom properties are correctly exposed when enumerating
CSSStyleDeclaration as a list. A similar but more restricted version
already exists: cssstyledeclaration-registered-custom-properties.html.
Two test cases are also added to determine-registration.html in order
to cover some issue previously detected during the review of
get_custom_property_initial_values.

[1] https://drafts.csswg.org/css-variables-2/#invalid-at-computed-value-time
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1855946

Test Plan

Try server: https://treeherder.mozilla.org/jobs?repo=try&revision=993f4569e8561e8075b4e527f33c1e3867ff936e&selectedTaskRun=Z0m87gfjT6Ouc8NUCkxTiA.0

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Build Status

Buildable 584987
Build 682968: arc lint + arc unit

Event Timeline

fredw planned changes to this revision.Oct 4 2023, 6:39 AM

fredw created this revision.

Herald added a project: secure-revision. · View Herald TranscriptOct 4 2023, 6:39 AM

Harbormaster completed remote builds in B583630: Diff 771142.Oct 4 2023, 6:39 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 4 2023, 6:39 AM

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

fredw requested review of this revision.Oct 4 2023, 8:56 AM

fredw updated this revision to Diff 771174.

fredw retitled this revision from WIP: Bug 1855629 - Handling of custom properties when enumerating getComputedStyle(). r=emilio to Bug 1855629 - Handling of custom properties when enumerating getComputedStyle(). r=emilio.

fredw added a reviewer: emilio.

Herald added a subscriber: zrhoffman. · View Herald TranscriptOct 4 2023, 8:56 AM

Harbormaster completed remote builds in B583664: Diff 771174.Oct 4 2023, 8:56 AM

fredw updated this revision to Diff 771212.Oct 4 2023, 10:23 AM

fredw edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B583695: Diff 771212.Oct 4 2023, 10:23 AM

@emilio: I'm not totally satisfied with how the initial values is handled, but sending for review to get some feedback... thanks!

Seems sensible at a glance... We should avoid recomputing the initial values over and over tho. Maybe doing it once after stylist rebuild / and when the JS registry changes?

We might need a way to opt out of the registered property initial values for anonymous content, but that's a separate issue.

servo/components/style/custom_properties.rs
326–329	nit: This could be a bit simpler now they are the same type, right? let mut map = if inherits { &mut self.inherited } else { &mut self.non_inherited }; Arc::make_mut(map.get_or_insert_with(Default::default))
804	Yeah this is not great. I'm ok with making `non_inherited` an `Arc` and so on, but we should definitely not go through all the `@property` rules and script registry once for every element, specially because the `Arc` as is doesn't accomplish much meaningful sharing...

This revision now requires changes to proceed.Oct 4 2023, 11:14 AM

fredw added a child revision: D189651: Bug 1855946 - Handle invalid at computed-value time custom properties. r=emilio.Oct 4 2023, 12:12 PM

fredw planned changes to this revision.Oct 4 2023, 12:51 PM

fredw updated this revision to Diff 771274.

fredw retitled this revision from Bug 1855629 - Handling of custom properties when enumerating getComputedStyle(). r=emilio to WIP: Bug 1855629 - Handling of custom properties when enumerating getComputedStyle(). r=emilio.

Harbormaster completed remote builds in B583751: Diff 771274.Oct 4 2023, 12:51 PM

fredw planned changes to this revision.Oct 4 2023, 12:56 PM

fredw updated this revision to Diff 771277.

Harbormaster completed remote builds in B583754: Diff 771277.Oct 4 2023, 12:56 PM

fredw mentioned this in D189651: Bug 1855946 - Handle invalid at computed-value time custom properties. r=emilio.Oct 4 2023, 1:07 PM

fredw planned changes to this revision.Oct 4 2023, 1:22 PM

fredw updated this revision to Diff 771304.

Harbormaster completed remote builds in B583774: Diff 771304.Oct 4 2023, 1:22 PM

fredw requested review of this revision.Oct 5 2023, 7:18 AM

fredw updated this revision to Diff 771776.

fredw edited the summary of this revision. (Show Details)

fredw marked an inline comment as done.

Harbormaster completed remote builds in B584142: Diff 771776.Oct 5 2023, 7:18 AM

fredw edited the test plan for this revision. (Show Details)Oct 5 2023, 4:29 PM

fredw updated this revision to Diff 772071.Oct 5 2023, 4:34 PM

fredw edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B584434: Diff 772071.Oct 5 2023, 4:34 PM

emilio accepted this revision.Oct 6 2023, 11:09 AM

emilio added a project: testing-approved.

emilio added inline comments.

testing/web-platform/meta/css/css-properties-values-api/registered-property-cssom.html.ini
15	Do you know what are these regressions about? Missing invalidation in `RegisterProperty`? Something else?

This revision is now accepted and ready to land.Oct 6 2023, 11:09 AM

fredw marked 2 inline comments as done.Oct 6 2023, 11:31 AM

fredw added inline comments.

testing/web-platform/meta/css/css-properties-values-api/registered-property-cssom.html.ini
15	I had checked and tests after `CSS.registerProperty` are still working as expected. The problem rather in the conflicts between guaranteed-invalid values VS initial values both represented by absent values. I suspect they were just passing by luck in the past but I will investigate further later after pending patches land.

fredw updated this revision to Diff 772414.Oct 6 2023, 11:35 AM

fredw marked an inline comment as done.

Harbormaster completed remote builds in B584695: Diff 772414.Oct 6 2023, 11:35 AM

Closed by commit rMOZILLACENTRAL4556cc6bbefb: Bug 1855629 - Handling of custom properties when enumerating getComputedStyle(). (authored by fredw). · Explain WhyOct 6 2023, 12:10 PM

This revision was automatically updated to reflect the committed changes.

fredw added a commit: rMOZILLACENTRAL4556cc6bbefb: Bug 1855629 - Handling of custom properties when enumerating getComputedStyle()..

Bug 1855629 - Handling of custom properties when enumerating getComputedStyle(). r=emilio
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 772428

servo/components/style/custom_properties.rs

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

servo/components/style/stylist.rs

servo/ports/geckolib/glue.rs

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

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

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

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

testing/web-platform/meta/css/cssom/cssstyledeclaration-registered-custom-properties.html.ini

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

testing/web-platform/tests/css/css-properties-values-api/get-computed-style-enumeration.html

Bug 1855629 - Handling of custom properties when enumerating getComputedStyle(). r=emilioClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 772428

servo/components/style/custom_properties.rs

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

servo/components/style/stylist.rs

servo/ports/geckolib/glue.rs

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

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

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

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

testing/web-platform/meta/css/cssom/cssstyledeclaration-registered-custom-properties.html.ini

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

testing/web-platform/tests/css/css-properties-values-api/get-computed-style-enumeration.html

Bug 1855629 - Handling of custom properties when enumerating getComputedStyle(). r=emilio
ClosedPublic
Actions

Revision Contents
Changeset List