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

[flex] Sizing images with intrisic aspect-ratio: ¿harmonize with grids? #1112

Closed
FremyCompany opened this Issue Mar 16, 2017 · 18 comments

Comments

Projects
None yet
7 participants
@FremyCompany
Contributor

FremyCompany commented Mar 16, 2017

In #523 (comment), @mrego mentioned that the change we proposed to Grid to enable images to shrink while maintaining their aspect ratio, but no to fill maintaining their aspect ratio created a potential difference between flex and grid.

Since I had absolutely no idea what flex was doing in that case, I wrote a few test cases and -- surprise! -- we are not interoperable :) The good news is that this means maybe we can try to match what we do in grid, but the bad news is that this means some of us have bugs to fix...

Here are the two test cases:

I had a quick discussion with @fantasai and she seemed to believe Edge is behaving the per-spec in this case. It would be a good idea to think about what we are doing here and maybe try to match this behavior in grid for the /normal/ alignment as much as it makes sense.

Now that we have both "stretch" to force-stretch and "width:contain" to force-fit, I think it would make sense to try to get the default behaviors of grid and flex match where possible.

The difference is that grids do not have a preferred axis like flex does, but maybe we could change grid to behave as-if in these cases (the preferred axis being deduced from the writing mode)? Or is there a way to make flexbox functioning more like what we said to do for grids in another way that would make more sense?

Thoughts?

cc @gregwhitworth @tabatkins @dbaron

@dbaron

This comment has been minimized.

Show comment
Hide comment
@dbaron
Member

dbaron commented Mar 22, 2017

@astearns astearns removed the Agenda+ label Mar 22, 2017

@astearns

This comment has been minimized.

Show comment
Hide comment
@astearns

astearns Mar 22, 2017

Member

On the call today we gave @FremyCompany an action to write bugs on the browsers that do not match the spec behavior. If fixing those bugs proves problematic, we can discuss how we can harmonize behavior in another way.

Member

astearns commented Mar 22, 2017

On the call today we gave @FremyCompany an action to write bugs on the browsers that do not match the spec behavior. If fixing those bugs proves problematic, we can discuss how we can harmonize behavior in another way.

@FremyCompany

This comment has been minimized.

Show comment
Hide comment
@FremyCompany

FremyCompany Mar 22, 2017

Contributor

I think there are two reasonable behaviors here.

Either the spec behavior (which Edge matches) or a modified spec behavior that prevents images from shrinking beyond their adjusted size in the flexible axis (adjustment being their cross-axis-height / natural-height * natural-width, instead of natural-width like the spec currently requires).

The advantage of the current spec text is that is does not create an exception for things that have an aspect ratio.

The advantage of the adjusted spec text would be that images preserve their aspect ratio by default in flex.

Contributor

FremyCompany commented Mar 22, 2017

I think there are two reasonable behaviors here.

Either the spec behavior (which Edge matches) or a modified spec behavior that prevents images from shrinking beyond their adjusted size in the flexible axis (adjustment being their cross-axis-height / natural-height * natural-width, instead of natural-width like the spec currently requires).

The advantage of the current spec text is that is does not create an exception for things that have an aspect ratio.

The advantage of the adjusted spec text would be that images preserve their aspect ratio by default in flex.

@dholbert

This comment has been minimized.

Show comment
Hide comment
@dholbert

dholbert Mar 22, 2017

Contributor

I posted some thoughts on the Mozilla bug that @FremyCompany filed. I agree that Edge's results are correct per the current spec here, in all of the cases shown. (Though it seems we're considering changing the spec somewhat to harmonize with grids, I guess.)

Contributor

dholbert commented Mar 22, 2017

I posted some thoughts on the Mozilla bug that @FremyCompany filed. I agree that Edge's results are correct per the current spec here, in all of the cases shown. (Though it seems we're considering changing the spec somewhat to harmonize with grids, I guess.)

@FremyCompany

This comment has been minimized.

Show comment
Hide comment
@FremyCompany

FremyCompany Mar 23, 2017

Contributor

Harmonization could happen both ways. Here is a plan that would harmonize grids with flex by mapping grid to flex:

  • We keep flexbox unchanged (except maybe -- if the group thinks this is worth it -- the adjusted auto flex basis for elements with intrinsic aspect ratio)
  • We change grid again to behave exactly like a flex with flex-direction:column (the image having flex: 0 0 auto, or flex: 0 1 auto if we adopt the previously-mentioned change to flex). That means "normal" means "stretch" in the width axis, and "start" in the height axis (if the element has an intrinsic aspect ratio which prevents natural stretching).
  • If you want things to work differently you either want "stretch" in both directions (which you can specify), or a manual "width: contain" which will take the available height and the aspect ratio into consideration when computing the width, and solve the issue for you.

This means images would by default behave like any other element in a grid, their width is determined first, then their height is deduced given their assigned width. With the caveat images are likely to overflow though since the grid tracks would probably have been sized without considering this. The options mentioned before allow to solve that problem though. To an author this is highly predictable and quickly understandable.

Thoughts? @fantasai @tabatkins @mrego

Contributor

FremyCompany commented Mar 23, 2017

Harmonization could happen both ways. Here is a plan that would harmonize grids with flex by mapping grid to flex:

  • We keep flexbox unchanged (except maybe -- if the group thinks this is worth it -- the adjusted auto flex basis for elements with intrinsic aspect ratio)
  • We change grid again to behave exactly like a flex with flex-direction:column (the image having flex: 0 0 auto, or flex: 0 1 auto if we adopt the previously-mentioned change to flex). That means "normal" means "stretch" in the width axis, and "start" in the height axis (if the element has an intrinsic aspect ratio which prevents natural stretching).
  • If you want things to work differently you either want "stretch" in both directions (which you can specify), or a manual "width: contain" which will take the available height and the aspect ratio into consideration when computing the width, and solve the issue for you.

This means images would by default behave like any other element in a grid, their width is determined first, then their height is deduced given their assigned width. With the caveat images are likely to overflow though since the grid tracks would probably have been sized without considering this. The options mentioned before allow to solve that problem though. To an author this is highly predictable and quickly understandable.

Thoughts? @fantasai @tabatkins @mrego

@fantasai

This comment has been minimized.

Show comment
Hide comment
@fantasai

fantasai Mar 23, 2017

Contributor

In general, we have tried to keep Grid’s sizing algorithms symmetric. So I've a preference to keep things as they were resolved in 523. Using the adjusted auto flex basis (the “transferred size” in the spec) in place of the intrinsic size makes sense, though. If it's a fix we can safely make, it might be worth doing. It would also resolve some confusion in Grid as well, I think.

Contributor

fantasai commented Mar 23, 2017

In general, we have tried to keep Grid’s sizing algorithms symmetric. So I've a preference to keep things as they were resolved in 523. Using the adjusted auto flex basis (the “transferred size” in the spec) in place of the intrinsic size makes sense, though. If it's a fix we can safely make, it might be worth doing. It would also resolve some confusion in Grid as well, I think.

@fantasai

This comment has been minimized.

Show comment
Hide comment
@fantasai

fantasai Mar 23, 2017

Contributor

(My general feeling is that, from an authoring perspective, a single intrinsic dimension of an image is not really important; it's maintaining the ratio that counts.)

Contributor

fantasai commented Mar 23, 2017

(My general feeling is that, from an authoring perspective, a single intrinsic dimension of an image is not really important; it's maintaining the ratio that counts.)

@FremyCompany

This comment has been minimized.

Show comment
Hide comment
@FremyCompany

FremyCompany Mar 24, 2017

Contributor

@fantasai Ok let's rephrase slightly then.

I am currently evaluating the following idea: elements with width:auto;height:auto and an aspect ratio would behave like if they had width:auto;height: var(width * aspect-ratio) instead when alignment is normal (and maybe also stretch).

That way we could say that all elements (irrespective of aspect-ratio) behave as stretch in both directions, but the height only stretches if it is considered auto which would not be the case for an element that has an intrinsic aspect ratio. The fact width gets computed first is not a novelty, this is true in all browsers and all layout modes (by width I mean inline size ofc).

Then either stretch would be an agressive normal that ignores aspect-ratio or maybe we don't even need any different with normal at all since you can get the stretch behavior using position:absolute +top/left/right/bottom:0 inside your grid area. It is even better because using stretch/normal will cause you to influence the track sizing with your image intrinsic size which is rarely what you want, especially if you want to stretch.

Contributor

FremyCompany commented Mar 24, 2017

@fantasai Ok let's rephrase slightly then.

I am currently evaluating the following idea: elements with width:auto;height:auto and an aspect ratio would behave like if they had width:auto;height: var(width * aspect-ratio) instead when alignment is normal (and maybe also stretch).

That way we could say that all elements (irrespective of aspect-ratio) behave as stretch in both directions, but the height only stretches if it is considered auto which would not be the case for an element that has an intrinsic aspect ratio. The fact width gets computed first is not a novelty, this is true in all browsers and all layout modes (by width I mean inline size ofc).

Then either stretch would be an agressive normal that ignores aspect-ratio or maybe we don't even need any different with normal at all since you can get the stretch behavior using position:absolute +top/left/right/bottom:0 inside your grid area. It is even better because using stretch/normal will cause you to influence the track sizing with your image intrinsic size which is rarely what you want, especially if you want to stretch.

@cbiesinger

This comment has been minimized.

Show comment
Hide comment
@cbiesinger

cbiesinger Mar 24, 2017

How do the test cases look if you also add min-height: 0; min-width: 0;? Just trying to clarify if the bug is in min-size handling or the actual element sizing. For example, why shouldn't the first flex-shrink testcase have a minimum width of 32px since that's the intrinsic size and there's no definitive cross-size?

I do agree that the aspect ratio should be respected.

cbiesinger commented Mar 24, 2017

How do the test cases look if you also add min-height: 0; min-width: 0;? Just trying to clarify if the bug is in min-size handling or the actual element sizing. For example, why shouldn't the first flex-shrink testcase have a minimum width of 32px since that's the intrinsic size and there's no definitive cross-size?

I do agree that the aspect ratio should be respected.

@FremyCompany

This comment has been minimized.

Show comment
Hide comment
@FremyCompany

FremyCompany Mar 27, 2017

Contributor

Your second question is the key here. The image should not be 32px because there is a cross-size due to how flex works (the flex container has a definite size in both directions, and items have their height set to whatever value the flexbox resolves to; the next step is the transfer of this size to the other direction). This was incorporated in the computation of the minimum size, see the following spec text:

In general, the automatic minimum size is the smaller of its content size and its specified size.

However, if the box has an aspect ratio and no specified size, its automatic minimum size is the smaller (15px) of its content size (32px) and its transferred size (15px * 32/32).

If the box has neither a specified size nor an aspect ratio, its automatic minimum size is the content size.

If you set min-width and min-height to 0px, this spec text does not even apply and you will be able to shrink to fit in all cases.


The actual question in this thread is whether taking the automatic-minimum-size from the content-size makes sense for boxes that have an aspect ratio and a transferred size, because it results in flexing in the second case which is unexpected. If we changed the spec text to say that if there is an aspect ratio and a transferred size, use that. Otherwise use the content size. Then the first and second test case would match in results.

Contributor

FremyCompany commented Mar 27, 2017

Your second question is the key here. The image should not be 32px because there is a cross-size due to how flex works (the flex container has a definite size in both directions, and items have their height set to whatever value the flexbox resolves to; the next step is the transfer of this size to the other direction). This was incorporated in the computation of the minimum size, see the following spec text:

In general, the automatic minimum size is the smaller of its content size and its specified size.

However, if the box has an aspect ratio and no specified size, its automatic minimum size is the smaller (15px) of its content size (32px) and its transferred size (15px * 32/32).

If the box has neither a specified size nor an aspect ratio, its automatic minimum size is the content size.

If you set min-width and min-height to 0px, this spec text does not even apply and you will be able to shrink to fit in all cases.


The actual question in this thread is whether taking the automatic-minimum-size from the content-size makes sense for boxes that have an aspect ratio and a transferred size, because it results in flexing in the second case which is unexpected. If we changed the spec text to say that if there is an aspect ratio and a transferred size, use that. Otherwise use the content size. Then the first and second test case would match in results.

@cbiesinger

This comment has been minimized.

Show comment
Hide comment
@cbiesinger

cbiesinger Mar 27, 2017

and items have their height set to whatever value the flexbox resolves to

That isn't really true though. That happens when stretch is resolved, and that happens fairly late in the algorithm:
https://drafts.csswg.org/css-flexbox/#algo-line-stretch

Notably this happens after the cross size of each line is determined. So at the point in time when we calculate the line height, the cross size is not definite and thus does not affect the aspect ratio calculation.

cbiesinger commented Mar 27, 2017

and items have their height set to whatever value the flexbox resolves to

That isn't really true though. That happens when stretch is resolved, and that happens fairly late in the algorithm:
https://drafts.csswg.org/css-flexbox/#algo-line-stretch

Notably this happens after the cross size of each line is determined. So at the point in time when we calculate the line height, the cross size is not definite and thus does not affect the aspect ratio calculation.

@FremyCompany

This comment has been minimized.

Show comment
Hide comment
@FremyCompany

FremyCompany Mar 27, 2017

Contributor

Handle 'align-content: stretch'. If the flex container has a definite cross size, align-content is stretch, and the sum of the flex lines' cross sizes is less than the flex container’s inner cross size, increase the cross size of each flex line by equal amounts such that the sum of their cross sizes exactly equals the flex container’s inner cross size.

By that logic, wouldn't you always output the image at 32x32 at minimum? align-content:stretch does not allow to shrink, and if you do not consider the flex as imposing its height on the image, the image would get 32x32 which it doesn't in any browser.

In actuality, this part of the algorithm does not even trigger here because the flex has only one line, and that line therefore takes its cross size from the element directly (see step 8).

The actual behavior we are discussing here is described further in the algorithm:

11: https://drafts.csswg.org/css-flexbox/#algo-stretch
Determine the used cross size of each flex item. If a flex item has align-self: stretch, its computed cross size property is auto, and neither of its cross-axis margins are auto, the used outer cross size is the used cross size of its flex line, clamped according to the item’s min and max cross size properties. Otherwise, the used cross size is the item’s hypothetical cross size.

If the flex item has align-self: stretch, redo layout for its contents, treating this used size as its definite cross size so that percentage-sized children can be resolved.

Note that this step does not affect the main size of the flex item, even if it has an intrinsic aspect ratio.

The last sentence seems to give Chrome the edge here, though. By that line, we should actually always have a main size of 32px irrespective of whether we shrinked-or-grew the image. So, the algorithm explicitly does not try to preserve the aspect-ratio. I guess the reasoning is that otherwise your line could overflow even if you had set the flex to wrap. That is very unfortunate though, because now you cannot preserve the aspect ratio in a flex.

@fantasai Did I miss something here? Last time you had a look at this, you felt the first rendering was the right one, which was Edge's behavior. Now based on the actual spec text I am not sure anymore.

Contributor

FremyCompany commented Mar 27, 2017

Handle 'align-content: stretch'. If the flex container has a definite cross size, align-content is stretch, and the sum of the flex lines' cross sizes is less than the flex container’s inner cross size, increase the cross size of each flex line by equal amounts such that the sum of their cross sizes exactly equals the flex container’s inner cross size.

By that logic, wouldn't you always output the image at 32x32 at minimum? align-content:stretch does not allow to shrink, and if you do not consider the flex as imposing its height on the image, the image would get 32x32 which it doesn't in any browser.

In actuality, this part of the algorithm does not even trigger here because the flex has only one line, and that line therefore takes its cross size from the element directly (see step 8).

The actual behavior we are discussing here is described further in the algorithm:

11: https://drafts.csswg.org/css-flexbox/#algo-stretch
Determine the used cross size of each flex item. If a flex item has align-self: stretch, its computed cross size property is auto, and neither of its cross-axis margins are auto, the used outer cross size is the used cross size of its flex line, clamped according to the item’s min and max cross size properties. Otherwise, the used cross size is the item’s hypothetical cross size.

If the flex item has align-self: stretch, redo layout for its contents, treating this used size as its definite cross size so that percentage-sized children can be resolved.

Note that this step does not affect the main size of the flex item, even if it has an intrinsic aspect ratio.

The last sentence seems to give Chrome the edge here, though. By that line, we should actually always have a main size of 32px irrespective of whether we shrinked-or-grew the image. So, the algorithm explicitly does not try to preserve the aspect-ratio. I guess the reasoning is that otherwise your line could overflow even if you had set the flex to wrap. That is very unfortunate though, because now you cannot preserve the aspect ratio in a flex.

@fantasai Did I miss something here? Last time you had a look at this, you felt the first rendering was the right one, which was Edge's behavior. Now based on the actual spec text I am not sure anymore.

@FremyCompany

This comment has been minimized.

Show comment
Hide comment
@FremyCompany

FremyCompany Mar 27, 2017

Contributor

https://jsfiddle.net/zcqst2pr/4/ shows that the shrinking is not due to flexing in Chrome and Firefox (since flex is set to 0). In Edge we overflow the line with two images of 128px width each.


Based on the results from that new test (where Firefox matches Chrome) and the very first one (where Firefox matches us visually), I would say that Firefox behavior seems to be a compromise between us and Chrome, as in they accept the aspect-ratio things to take less main space than before, but not more. This ensure there will never be an overflow, but tries to maintain the aspect ratio if possible.


Now to be honest, none of these behaviors please me. Firefox is inconsistent, Chrome never respects the aspect-ratio, and Edge can lead to undesired overflows. If I was designing this from scratch I would do this differently, and take this in consideration during the main size determination in case the flex element is single-line, but I don't know if the group feels like this should be changed or not. Also then it would create a difference between the single-line and the wrapped-lines cases.

Contributor

FremyCompany commented Mar 27, 2017

https://jsfiddle.net/zcqst2pr/4/ shows that the shrinking is not due to flexing in Chrome and Firefox (since flex is set to 0). In Edge we overflow the line with two images of 128px width each.


Based on the results from that new test (where Firefox matches Chrome) and the very first one (where Firefox matches us visually), I would say that Firefox behavior seems to be a compromise between us and Chrome, as in they accept the aspect-ratio things to take less main space than before, but not more. This ensure there will never be an overflow, but tries to maintain the aspect ratio if possible.


Now to be honest, none of these behaviors please me. Firefox is inconsistent, Chrome never respects the aspect-ratio, and Edge can lead to undesired overflows. If I was designing this from scratch I would do this differently, and take this in consideration during the main size determination in case the flex element is single-line, but I don't know if the group feels like this should be changed or not. Also then it would create a difference between the single-line and the wrapped-lines cases.

@fantasai

This comment has been minimized.

Show comment
Hide comment
@fantasai

fantasai Mar 30, 2017

Contributor

Note, @tabatkins and I just filed #1149 which is related here.

Contributor

fantasai commented Mar 30, 2017

Note, @tabatkins and I just filed #1149 which is related here.

@tabatkins

This comment has been minimized.

Show comment
Hide comment
@tabatkins

tabatkins Jul 18, 2017

Member

and items have their height set to whatever value the flexbox resolves to

That isn't really true though. That happens when stretch is resolved, and that happens fairly late in the algorithm:

Nope, it now happens automatically, by virtue of case 1 in https://drafts.csswg.org/css-flexbox/#definite-sizes. This gives the flex item a definite cross size immediately, before flex layout actually runs.

Member

tabatkins commented Jul 18, 2017

and items have their height set to whatever value the flexbox resolves to

That isn't really true though. That happens when stretch is resolved, and that happens fairly late in the algorithm:

Nope, it now happens automatically, by virtue of case 1 in https://drafts.csswg.org/css-flexbox/#definite-sizes. This gives the flex item a definite cross size immediately, before flex layout actually runs.

@fantasai

This comment has been minimized.

Show comment
Hide comment
@fantasai

fantasai Aug 11, 2017

Contributor

@FremyCompany Do you think this issue still needs to stay open? (#1149 was fixed recently, fwiw.)

Contributor

fantasai commented Aug 11, 2017

@FremyCompany Do you think this issue still needs to stay open? (#1149 was fixed recently, fwiw.)

@FremyCompany

This comment has been minimized.

Show comment
Hide comment
@FremyCompany

FremyCompany Aug 16, 2017

Contributor

I think it is fine to close. The topic was discussed in other issues as you mentioned.

Contributor

FremyCompany commented Aug 16, 2017

I think it is fine to close. The topic was discussed in other issues as you mentioned.

@fantasai fantasai added this to the Published css-flexbox-1 2017-10-19 milestone Mar 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment