Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-flexbox] Why does flex item aspect ratio sizing not take max-width into account? #3736

Closed
cbiesinger opened this issue Mar 16, 2019 · 14 comments

Comments

@cbiesinger
Copy link

@cbiesinger cbiesinger commented Mar 16, 2019

Here:
https://drafts.csswg.org/css-flexbox/#algo-main-item
The cross axis max-size is not taken into account. That differs from the min-size calculation here:
https://drafts.csswg.org/css-flexbox/#min-size-auto

This leads to unfortunate outcomes, e.g.:

<div style="display: flex; flex-direction: column; width: 10px;">
  <img src="10x10.png" style="max-width: 5px">
</div>

Now, the algorithm will compute a height of 10px, but the width is limited to 5px, and the image is distorted.

@cbiesinger
Copy link
Author

@cbiesinger cbiesinger commented Mar 16, 2019

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Apr 17, 2019

@cbiesinger Uh, that looks like a definite oversight. Should be factored in.

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Apr 18, 2019

OK, so looking at this more closely, it is handled in E (which is where we fall down to):

Otherwise, size the item into the available space using its used flex basis in place of its main size, treating a value of content as max-content. If a cross size is needed to determine the main size (e.g. when the flex item’s main size is in its block axis) and the flex item’s cross size is auto and not definite, in this calculation use fit-content as the flex item’s cross size. The flex base size is the item’s resulting main size.

The max-content size takes into account any sizing constraints in the cross axis:
https://drafts.csswg.org/css-sizing-3/#intrinsic-sizes

We might want to make it clearer that this case is handled, though.

@cbiesinger
Copy link
Author

@cbiesinger cbiesinger commented Apr 18, 2019

But that's not what I'm concerned about (and anyway, why would we fall down to that?)

If we have a replaced item, for the min-size calculation, we do:
main size = min(cross size, min-cross size) * aspect ratio

For the flex base size, we do:
main size = cross size * aspect ratio

Shouldn't that be consistent?

@cbiesinger
Copy link
Author

@cbiesinger cbiesinger commented Jun 11, 2019

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Jun 11, 2019

@cbiesinger I'm not sure I understand your point about consistency. Those two calculations are consistent, are they not? The only difference is that the min-size uses the minimum size and the base size doesn't, which is appropriate.

Anyway, to follow your example...

<div style="display: flex; flex-direction: column; width: 10px;">
  <img src="10x10.png" style="max-width: 5px">
</div>

First, we calculate the minimum main size of this image, which is an automatic minimum size. The specified size is undefined, the transferred size is 5px, and the content size is 10px. The transferred size wins, so its minimum main size is 5px.

Then we calculate the base size. The flex basis is content and it has an aspect ratio. It doesn't have a definite cross size, so we fall down to E, which says to size the item into the available space treating its main size as max-content. The max-content value runs its calculations based off of sizing an auto-height floated 10x10 image in an infinitely-sized containing block--according to CSS2.1 section 10.4 this is max(max-width*h/w,min-height) which here is equal to 5px. The resulting main size is 5px, the cross size is 5px, and the image maintains its 1:1 aspect ratio, which is exactly the result you want in 972576, no?

@cbiesinger
Copy link
Author

@cbiesinger cbiesinger commented Jun 13, 2019

Why doesn't it have a definite cross size? Remember point 1 in https://drafts.csswg.org/css-flexbox/#definite

However, the same issue occurs if you give it a width: 100px (which is basically the 972576 case), and then the cross size is certainly definite.

Also, my point about consistency was about how the cross size max size is treated, it seems inconsistent for no real reason?

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Jun 13, 2019

@cbiesinger Well, the flex container has a definite cross size, sure. Wrt the flex item itself... If it's not stretched, then it's not definite and would fall through E as I described above. If it is stretched... then you're right, we'd fall into B. The flex base size would be calculated from the cross size, which is max([container cross size], 5px), which is usually 5px. So let's assume 5px as the inner cross size. Transferred through the aspect ratio gives us a flex base size of 5px, and the image maintains its 1:1 aspect ratio. Basically, same result.

I suppose the issue here is that “cross size” is not clarified as to whether it's the “used cross size” or the value of the cross size property? It should definitely be the used cross size that's used in this calculation, so that the max is taken into account. Lmk if this is the issue, or if there's something else.

@cbiesinger
Copy link
Author

@cbiesinger cbiesinger commented Jun 13, 2019

Why would it "usually" be 5px? That's only if the container is smaller than 5px. In my example the item is stretched (default value) and max(5px, cross size) is 10px. So with a 1:1 ratio, you'll get a 10px height (main size) and 5px cross size (because of max-width).

Or, if you specify something like width: 100px; max-width: 5px;, we'll use 100px to compute main size but will clamp cross size to 5px.

Yes, specifying that the cross size needs to be clamped by max (and min?) is the outcome I am hoping for. I would prefer not using the terminology "used cross size" because to me it's not clear that this implies applying min/max.

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Jun 17, 2019

Why would it "usually" be 5px? That's only if the container is smaller than 5px.

Yeah, that's the only case I can think of. Your specific example would indeed be 5px by 5px afaict.

Or, if you specify something like width: 100px; max-width: 5px;, we'll use 100px to compute main size but will clamp cross size to 5px.

So as I mentioned, I think “cross size” under B should be “used cross size”, which means it's going to be 5px in that calculation. And thus the main size will resolve to 5px.

Yes, specifying that the cross size needs to be clamped by max (and min?) is the outcome I am hoping for. I would prefer not using the terminology "used cross size" because to me it's not clear that this implies applying min/max.

The “used cross size” maps to the “used height” in this case, right? I think that should be clear. And the used height is definitely applying min/max, otherwise getComputedStyle() wouldn't be honoring them either.

fantasai added a commit that referenced this issue Jun 17, 2019
… aspect ratio take the *used* cross size as input. #3736
@fantasai
Copy link
Collaborator

@fantasai fantasai commented Jun 17, 2019

Clarified to “used inner cross size” in the section defining the calculation of flex base sizes as a function of the cross size and an aspect ratio. @dholbert Would you mind reviewing this issue, please? :)

@cbiesinger
Copy link
Author

@cbiesinger cbiesinger commented Jun 17, 2019

OK, that looks good, thanks!

@dholbert
Copy link
Member

@dholbert dholbert commented Jun 21, 2019

@dholbert Would you mind reviewing this issue, please? :)

I think the change makes sense, yeah. Thanks!

@tabatkins
Copy link
Member

@tabatkins tabatkins commented Sep 11, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants