fix text height calc #11589
fix text height calc #11589
Conversation
alice-i-cecile
added
C-Bug
|
sorry, actually this isn't enough... it works for the red and blue items in the issue, but the green outer box still doesn't resize properly. it looks like there's a more fundamental size propagation issue, when that's fixed this pr may not be required. i'll move it to draft for now. |
|
it seems like this pr works (and is still required) when taken together with taffy-main and #10690, so i'll take it out of draft when that's done. |
There was a problem hiding this comment.
From what I can see there is an important change here: taking into account the "known_width" (width variable). Where width is Some(_) we should run text layout to determine the height which we are not currently doing.
The other changes around using the min- and max- height constraints do not make sense to me. Text layout is fundamentally asymmetric in it's axes, and as such I believe we should be ignoring the vertical constraints (for horizontal text - for vertical text we would ignore the width constraints).
| let min = info.compute_size(Vec2::new(0.0, f32::INFINITY)); | ||
| let max = info.compute_size(Vec2::INFINITY); | ||
| info.min = min; | ||
| info.max = max; | ||
| let min_width = info.compute_size(Vec2::new(0.0, f32::INFINITY)); | ||
| let max_width = info.compute_size(Vec2::INFINITY); | ||
| info.min = min_width.min(max_width); | ||
| info.max = min_width.max(max_width); |
There was a problem hiding this comment.
I believe that the values of these properties were good before. But the naming was confusing. Calling these max_width_size and min_width_size might be less confusing?
| if width.is_some() || matches!(available_width, AvailableSpace::Definite(_)) { | ||
| // given a definite width we have a definite height | ||
| self.info.compute_size(Vec2::new(x, f32::MAX)).y | ||
| } else { |
There was a problem hiding this comment.
Adding this case for width.is_some() seems correct to me, and the main change that I do think is necessary in this PR.
There was a problem hiding this comment.
i'll check with only this change, thanks for looking it over
| match available_height { | ||
| AvailableSpace::MinContent => self.info.min.y, | ||
| AvailableSpace::MaxContent => self.info.max.y, | ||
| AvailableSpace::Definite(_) => self.info.compute_size(Vec2::new(x, f32::MAX)).y, | ||
| } |
There was a problem hiding this comment.
I think this should be changed back to available_width, which would allow the available_width case in the above PR to be removed.
| // possible and as short as possible. so we use the max width and min height | ||
| // directly. | ||
| content_size.set(FixedMeasure { | ||
| size: Vec2::new(measure.max.x, measure.min.y), |
There was a problem hiding this comment.
If you don't change the value of min and max above then this doesn't need to be changed either.
I can confirm that there are no changes relevant to this PR in Taffy main / 0.4 and that this PR will still be required. |
while that's true, it doesn't mean we shouldn't be able to find the optimal width for a given height (for example). i would expect this to also crop up in a dimension-independent manner with flex-wrapped row-direction items in a fixed-height container (or col-dir items in a fixed width container) - it's basically the same problem. but i haven't looked into how browsers handle it, i guess maybe not well. the correct response for sizing with MinContent/MaxContent over two axes simultaneously is confusing to me, the only way i could see to make sense of it was to make the returned Min/Max size independent on each axis. but if you think that's incorrect i definitely defer to your expertise. |
|
@robtfm #10690 landed so in theory this PR ought to be unblocked.
Yeah, they pretty much just don't. In fact for classical "flow" (block/inline) layout I believe there is a strict rule that the inline (horizontal) axis cannot depend on the block (vertical) axis at all. That is somewhat relaxed for Flexbox/Grid where there is some support. We can of course do better if we want to, but doing so performantly is tricky. |
Objective
fixes #11542
Solution
It's not clear to me what the expected result from a taffy measure function is, i think there's ambiguity in the arguments. but, currently we disregard available_height entirely and return the height for the requested width regardless of what height space is requested. this seems wrong. in particular, when known width and height are None, available width is MaxContent and available height is MaxContent, we always return a height of 1 line, when the actual max height is line height * word count.
this pr changes it so that text measure returns:
width (unchanged in this pr)
height:
this seems to fix my issue and seems to otherwise preserve existing behaviour as far as i can tell, but there are no tests that i'm aware of.
note that this won't provide useful results when height is known and width is MinContent, i.e. solving for the minimum width that yields the given height. this doesn't work in browsers either so i think that's fine.
requires #10690