Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSome more boring stuff to change how we compute content sizes #26852
Conversation
|
@bors-servo r+ |
|
|
|
|
|
|
| @@ -22,7 +22,7 @@ use style::Zero; | |||
|
|
|||
| #[derive(Debug, Serialize)] | |||
| pub(crate) struct AbsolutelyPositionedBox { | |||
| pub contents: IndependentFormattingContext, | |||
This comment has been minimized.
This comment has been minimized.
SimonSapin
Jun 15, 2020
Member
I’m not thrilled about this one (layout_context and style_content are groupings of transient data used "on the side" during layout, rather than the "object" of what we’re doing), but I don’t have a better suggestion.
| @@ -335,6 +347,87 @@ impl ComputedValuesExt for ComputedValues { | |||
| ) | |||
| } | |||
|
|
|||
| fn outer_inline( | |||
This comment has been minimized.
This comment has been minimized.
SimonSapin
Jun 15, 2020
Member
This move feels conceptually wrong to me. On the method receiver type: styles don’t have content sizes, boxes do. On the module location (which is independent): this code belongs in the sizing module.
This comment has been minimized.
This comment has been minimized.
nox
Jun 15, 2020
Author
Member
That method living on BoxContentSizes look even more wrong to me given it seldomly does anything with self there. "There are styles, and those styles can clamp an inline size, or a content size if there are no inline size" makes way more sense to me and better compartmentalize the code.
This comment has been minimized.
This comment has been minimized.
nox
Jun 15, 2020
Author
Member
And given you can't split a trait in multiple parts, this needs to be in style_ext.
This comment has been minimized.
This comment has been minimized.
nox
Jun 15, 2020
Author
Member
To further explain what I mean: styles don't have content sizes, but they definitely are what owns the size extremums, and the box-sizing property, and often a non-auto inline size. When there is a non-auto inline size, the BoxContentSizes value isn't even looked at. That means to me that it shouldn't be a method on BoxContentSizes.
This comment has been minimized.
This comment has been minimized.
SimonSapin
Jun 15, 2020
Member
How about making it a free function in the sizing module? style_ext already feels like a kitchen sink to me.
This comment has been minimized.
This comment has been minimized.
nox
Jun 15, 2020
Author
Member
That is totally fair, will do that, absolutely no damn clue how I didn't think of that on my own.
| @@ -20,44 +20,43 @@ use style::values::specified::text::TextDecorationLine; | |||
|
|
|||
| /// https://drafts.csswg.org/css-display/#independent-formatting-context | |||
| #[derive(Debug, Serialize)] | |||
| pub(crate) struct IndependentFormattingContext { | |||
| pub(crate) enum IndependentFormattingContext { | |||
This comment has been minimized.
This comment has been minimized.
SimonSapin
Jun 15, 2020
Member
What do we gain with this change? In particular pushing common fields like style further "down" into enum variants.
By the way, in a sizing model as described in w3c/csswg-drafts#5190 (comment) where the parent provides an available size on both axes and the child return its used size of both axis, I think we may be able to unify replace content with other kinds of independent formatting contexts and make it so that callers don’t need to handle them separately.
This comment has been minimized.
This comment has been minimized.
nox
Jun 15, 2020
Author
Member
It lets me write a method on NonReplacedFormattingContext that will use StyleExt::outer_inline_and_percentages and compute the content size on demand. I can't do that without splitting the two kinds of contents given the computation isn't done at all for replaced content.
This comment has been minimized.
This comment has been minimized.
nox
Jun 15, 2020
Author
Member
If I didn't do that, I would need to change NonReplacedIFC to also have a reference to the style field of IndependentFormattingContext, or have a method on NonReplacedIFC that takes a style argument, which seems way more confusing to me.
|
Fundamentally, I think it's wrong for any content-size-related code to be a method with many movable parts being passed as arguments, specifically I don't think it's good design for it to be a method on the content size type that can represent an absence of content size (in the case of explicit inline size), in a way where you can technically pass unrelated styles. |
|
@bors-servo r+ |
|
|
Some more boring stuff to change how we compute content sizes
|
|
|
@bors-servo retry |
|
|
nox commentedJun 10, 2020
No description provided.