fix: single-line flex-container should clamp the line's cross-size #638
Conversation
There was a problem hiding this comment.
I've requested a couple of changes, but this generally looks very good: thanks for the PR!
We may also wish to generally review the interaction of:
- The logic added in this PR
- The "if" statement at the top of
calculate_cross_size - The presence of min/max sizes (with no preferred size)
- The presence of wrap vs. nowrap
But that does not need to happen in this PR.
src/compute/flexbox.rs
Outdated
| .fold(0.0, |acc, x| acc.max(x)); | ||
| } | ||
| // If the flex container is single-line, then clamp the line’s cross-size to be within the container’s computed min and max cross sizes. | ||
| if flex_lines.len() == 1 { |
There was a problem hiding this comment.
This (confusingly) should be controlled by flex-wrap being nowrap (such that the container is inherently always single line) rather than just when it happens to contain a single line.
See https://www.w3.org/TR/css-flexbox-1/#flex-wrap-property:
The flex-wrap property controls whether the flex container is single-line or multi-line
We'll also need tests to check "nowrap" vs. "happens to be single-line"
There was a problem hiding this comment.
I noticed that flex_lines.len() == 1 is used in https://github.com/DioxusLabs/taffy/blob/main/src/compute/flexbox.rs#L1352 to determine if it is a single line.
I've also looked at some other browser layout engine implementations, I see no difference between "nowrap" and "happends to be single-line", but i'll add some tests to check it.
There was a problem hiding this comment.
I've also looked at some other browser layout engine implementations, I see no difference between "nowrap" and "happends to be single-line", but i'll add some tests to check it.
If you add flex-wrap: wrap (the default is nowrap) to the test you created for this PR then you'll find that the layout ends up changing. I rather suspect that line 1532 is wrong too (although that if does also include the flex_wrap property later in the condition).
but i'll add some tests to check it.
Thanks!
There was a problem hiding this comment.
I found the difference just like you said, we should use flex_wrap to determine if it is single-line or not.
I've added some test cases and it looks fine :)
- padding/border
- min/max size
- wrap/no-wrap
nicoburns
left a comment
There was a problem hiding this comment.
This looks good to me now. Thanks for the fixes (and the tests)!
Thanks for the review XD! |
Objective
In Taffy, missing a step when calculate the cross size of each flex line. https://www.w3.org/TR/css-flexbox-1/#algo-cross-line
This PR tries to complement this step.