Strict types for style values#269
Conversation
…aphic points (fixes DioxusLabs#236)
… for margin, padding, border and position. Required for removing Dimension::Undefined, as otherwise these tests break when the default Dimension changes from Undefined to Auto
nicoburns
force-pushed
the
strict-style-value-types
branch
from
6e6f96c to
0d957d3
Compare
alice-i-cecile
added
usability
Weibye
left a comment
Weibye
left a comment
There was a problem hiding this comment.
Again this is great work!
The CSS spec doesn't allow percentage borders, but Taffy historically has. For this reason, I've made the border property use LengthPercentage rather than f32. We could change this to f32 if people think that's better.
For Bevy I'm sure there will be use-cases for percentage-driven borders. I also think there's value on keeping to spec, at least until we support multiple paradigms. I don't feel strongly either way. Suggest we keep as is for now.
Dimension is currently identical to LengthPercentageAuto, so these could be merged into a single type if preferred. However, the properties using Dimension (width, min_width, max_width, flex_basis) support extra values (MinContent, MaxContent, FitContent, and flex_basis additionally supports Content) according the the spec, where the values using LengthPercentageAuto (position and margin) do not. So if and when we and support for those values, we'd need to split the type.
It feels weird to have two identical types at this point, but I think your argument is a strong one for why we want to keep them separate.
Sidenote: For something like flex_basis: Content, would it make sense to move measure-func into that enum and change how we deal with the measure functions?
How do people feel about the automagical shorthand functions? They make writing styles much easier, but they do make the types less explicit.
I really like them! With the increased "security" of the types, I think we can get away with less expicitness and more ergonomics.
| margin: Rect::zero(), | ||
| padding: Rect::zero(), | ||
| border: Rect::zero(), | ||
| gap: Size::zero(), |
There was a problem hiding this comment.
This reads so much clearer!
| pub position: Rect<LengthPercentageAuto>, | ||
| /// How large should the margin be on each side? | ||
| pub margin: Rect<Dimension>, | ||
| pub margin: Rect<LengthPercentageAuto>, | ||
| /// How large should the padding be on each side? | ||
| pub padding: Rect<Dimension>, | ||
| pub padding: Rect<LengthPercentage>, | ||
| /// How large should the border be on each side? | ||
| pub border: Rect<Dimension>, | ||
| pub border: Rect<LengthPercentage>, | ||
| // Gap | ||
| /// How large should the gaps between items in a grid or flex container be? | ||
| pub gap: Size<Dimension>, | ||
| pub gap: Size<LengthPercentage>, |
There was a problem hiding this comment.
I don't know enough about Rust. Can a type be flags of a different type? Cause that is what we are doing here: We have multiple options with points, percent, auto, undefined, and we want to accept some or many of those options.
Its something I'm curious about but not something that should block the PR.
There was a problem hiding this comment.
Do you mean like a sub-enum that takes a definition from a master one with all the variants? Unfortunately that's not yet a Rust feature, although it may be one day (it's been proposed before).
It would not. The reason being that I think MeasureFuncs are likely to morph into custom layout modes at some point. More to come on this once I've sorted my thoughts out a bit. |
I'm comfortable with natural spec extensions, and I think that this one is sensible. I think we should keep it. |
4 participants
Objective
Prevent invalid values from being on set on style properties.
Fixes #265 and #114
Does not fix #207 or #231
Context
Currently there are a number of properties that use the
Dimensionenum, and this meant that it had to represent a superset of all the possible value for each of those properties. This problem is anticipated to get worse as we add new values likeMinContent,MaxContentandCalc.Changes
Dimension::Undefined. As all CSS values have a default if unspecified, this is used in place ofUndefined.Defaultimpl forDimension. There is no single default: which value is the default depends on context. So to avoid false confidence in the default it is judged better simply not to have a default at all.TaffyZero,TaffyAuto, andFromPointstraits have been added that abstract over all types that can hold a value of zero (f32,Option<f32>,Dimension), auto (Dimension), and points (f32,Option<f32>,Dimensions). They are also implemented for Point, Size, and Rect where the containing types implements the trait. This allows the return-type generic functionszero(),auto(), andpoints(f32)to be defined which with automagically created an instance of the correct type through type inference.ResolveOrZerobased on theTaffyZerotrait.LengthPercentagetype which is likeDimension, except with onlyPointsandPercentvariants (noAuto). Convertgap,padding, andborderto useLengthPercentageinstead ofDimension.LengthPercentageAutotype. This is currently identical toDimension, except in future we will likely to want to addMinContent,MaxContent, andFitContentvariants toDimension. And properties usingLengthPercentageAutodon't support those properties. Convertpositionandmarginto useLengthPercentageAutoinstead ofDimension.Feedback wanted
borderproperty useLengthPercentagerather thanf32. We could change this to f32 if people think that's better.Dimensionis currently identical toLengthPercentageAuto, so these could be merged into a single type if preferred. However, the properties usingDimension(width,min_width,max_width,flex_basis) support extra values (MinContent,MaxContent,FitContent, andflex_basisadditionally supportsContent) according the the spec, where the values usingLengthPercentageAuto(positionandmargin) do not. So if and when we and support for those values, we'd need to split the type.