Page MenuHomePhabricator

Bug 1846632 - [css-properties-values-api] Use canonical values for font-independent types r=fredw,zsun,#style,#layout
ClosedPublic

Authored by zrhoffman on Oct 6 2023, 6:08 AM.
Referenced Files
Unknown Object (File)
Mar 14 2026, 6:52 AM
Unknown Object (File)
Dec 13 2025, 10:00 PM
Unknown Object (File)
Nov 29 2025, 8:28 AM
Unknown Object (File)
Nov 13 2025, 12:13 PM
Unknown Object (File)
Nov 13 2025, 12:13 PM
Unknown Object (File)
Nov 13 2025, 12:13 PM
Unknown Object (File)
Nov 13 2025, 12:13 PM
Unknown Object (File)
Nov 13 2025, 12:13 PM
Subscribers

Details

Summary

We resolve calc expressions and use appropriate units by computing
ToComputedValue trait, computing each component's value, then converting
it back to that component's type, before making a new VariableValue from
the computed value's CSS string to store in the computed properties map.

Event Timeline

phab-bot published this revision for review.Oct 6 2023, 6:08 AM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
zrhoffman edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Oct 6 2023, 7:25 AM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

emilio requested changes to this revision.Oct 6 2023, 11:00 AM
emilio added inline comments.
servo/components/style/properties_and_values/value.rs
69

It's very weird that we implement ToComputedValue returning Self as the computed value type. The right way to do this is making the type generic over the types that are different between computed and specified.

So you'd have something like:

enum GenericValueComponent<Length, Number, Percentage, LengthPercentage, Color, Image, Url, Integer, Angle, Time, Resolution, Transform> {

And specialize it for specified and computed types. Did you try that, and is there a reason that wouldn't work?

This is fine for now but eventually we should probably do something along those lines, perhaps.

148

There shouldn't be a need to have an intermediate vec here right?

152

Can this just clone the list (self.clone()) rather than allocating a new one?

157

This is not a slice either.

258

Yeah, thanks for filing the follow-up.

This revision now requires changes to proceed.Oct 6 2023, 11:00 AM
zrhoffman updated this revision to Diff 772828.
zrhoffman edited the summary of this revision. (Show Details)
zrhoffman marked 3 inline comments as done.
servo/components/style/properties_and_values/value.rs
69

It's very weird that we implement ToComputedValue returning Self as the computed value type. The right way to do this is making the type generic over the types that are different between computed and specified.

So you'd have something like:

enum GenericValueComponent<Length, Number, Percentage, LengthPercentage, Color, Image, Url, Integer, Angle, Time, Resolution, Transform> {

And specialize it for specified and computed types. Did you try that, and is there a reason that wouldn't work?

I was thinking of the specified and computed values as the same type because ComputedValue::compute effectively both accepts and returns token streams, but separating the specified and computed types would definitely be an improvement.

This is fine for now but eventually we should probably do something along those lines, perhaps.

Added a TODO for bug 1857716 and submitted D190382/D190383.

148

Right, intermediate vec removed.

152

It can indeed, changed to computed.clone() (but changed it back in D190383).

emilio edited projects, added testing-approved; removed needs-testing-tag.
emilio removed reviewers: Restricted Project, layout-reviewers.
emilio added inline comments.
servo/components/style/properties_and_values/value.rs
148

I don't think you need .into_iter()

This revision is now accepted and ready to land.Oct 10 2023, 4:10 PM
zrhoffman edited the summary of this revision. (Show Details)
zrhoffman marked 3 inline comments as done.
servo/components/style/properties_and_values/value.rs
148

.into_iter() removed.