[go: up one dir, main page]

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] [css-sizing-4] Interaction of flexbox minimum height and aspect-ratio minimum height #6069

Open
cbiesinger opened this issue Mar 3, 2021 · 12 comments

Comments

@cbiesinger
Copy link

https://drafts.csswg.org/css-flexbox/#min-size-auto
https://drafts.csswg.org/css-sizing-4/#aspect-ratio-minimum

If an item has an aspect ratio and is a flex item, both specs impose a minimum size on a flex item. Consider:

<div style="display: flex; flex-direction: column; width: 100px;">
  <div style="aspect-ratio: 1/1">
    <div style="height: 500px;"></div>
  </div>
</div>

In this case, the flex item has two constraints:

  • Flexbox's automatic minimum size gives it a minimum height of 100px (the transferred size), because it uses the minimum of the content-based size and transferred size.
  • aspect-ratio's minimum size gives it a min-height of 500px

What should the used minimum size be? max(100px, 500px)? Or should the flexbox one "override" the aspect-ratio one?

https://github.com/web-platform-tests/wpt/blob/master/css/css-sizing/aspect-ratio/flex-aspect-ratio-026.html relies on the latter interpretation.

@cbiesinger
Copy link
Author

@tabatkins @fantasai

@fantasai
Copy link
Collaborator
fantasai commented Mar 3, 2021

The Flexbox automatic minimum size rules were drafted with the assumption that only replaced elements had an aspect ratio. The Sizing rules, which make the content-based size a very strong constraint on the auto min size, are correct for non-replaced elements; but the Flexbox rules, which make the "content-based" (aka replaced element natural size) a weaker constraint, are correct for replaced elements.

Probably what we need to fix here is to distinguish content-based sizes of non-replaced elements which should be a stronger constraint than a transferred size vs content-based sizes of replaced elements (those derived from the natural size of the replaced content), which should be a weaker constraint than a transferred size.

So this needs some tweaks to merge the two into a consistent set of rules.

@fantasai
Copy link
Collaborator
fantasai commented Mar 3, 2021

Agenda+ to confirm that we want to make non-replaced elements ignore any transferred preferred sizes for the purpose of automatic minimum sizing.

Remember, this is the minimum size below which a flex item, by default, cannot shrink. We don't want to be too constraining, so that when it's sensible to shrink it can shrink.

Current spec:

  • if specified size exists: min(specified, content-based)
  • else: min(transferred, content-based)

Proposed spec:

  • for replaced:
    • if specified size exists: min(specified size, natural size)
    • else: min(transferred preferred, natural size)
  • for non-replaced:
    • if specified size exists: min(specified size, content size)
    • else: content size

In all cases, we apply an opposite-axis minimum transferred through the aspect ratio if any: min(specified size, max size, opposite-min * aspect-ratio)

Alternately we can ignore the transferred min of a non-replaced element.

[All of these are clamped by the appropriate maximum sizes.]

The various inputs into this are

  • definite min/preferred/max sizes in affected axis
  • definite min/preferred/max sizes transferred from opposite axis (if there's an aspect ratio)
  • content-based size in affected axis (if non-replaced)
  • natural size in affected axis (if replaced)

@cbiesinger
Copy link
Author

(cc @aethanyc since this affects tests they wrote)

I think that is an outcome that makes sense; it is certainly consistent with block layout in the sense that aspect ratio is "ignored" if it would cause content to overflow.

@aethanyc
Copy link
aethanyc commented Mar 3, 2021

flex-aspect-ratio-026.html was written with the assumption that the flexbox's automatic minimum size rules has a higher priority over the aspect-ratio's. I didn't aware for non-replaced element, applying the flexbox's rule min(transferred, content-based) somewhat contradict to the outcome that the aspect-ratio's automatic content-based min size is trying to produce.

The proposed change -- non-replaced elements ignore any transferred preferred sizes for the purpose of automatic minimum sizing -- looks reasonable to me.

(cc @dholbert)

@cbiesinger
Copy link
Author

(Also a reminder that grid probably needs a similar change)

@dholbert
Copy link
Member

Echoing @aethanyc :

The proposed change -- non-replaced elements ignore any transferred preferred sizes for the purpose of automatic minimum sizing -- looks reasonable to me.

Yup, seems reasonable to me, too.

fantasai added a commit that referenced this issue Mar 11, 2021
fantasai added a commit that referenced this issue Mar 11, 2021
@fantasai
Copy link
Collaborator

OK, we've updated the Flexbox and Grid specs to exclude non-replaced elements from using the transferred size suggestion:

Note: We also redrafted Flexbox’s definition to be a bit to be easier to read and to match Grid more closely in f86bc38

Keeping Agenda+ to confirm with the CSSWG.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-flexbox] [css-sizing-4] Interaction of flexbox minimum height and aspect-ratio minimum height.

The full IRC log of that discussion <dael> Topic: [css-flexbox] [css-sizing-4] Interaction of flexbox minimum height and aspect-ratio minimum height
<dael> github: https://github.com//issues/6069
<fantasai> https://github.com//issues/6069#issuecomment-790046695
<dael> Rossen_: Summary ^
<fantasai> https://github.com//issues/6069#issuecomment-797028657
<dael> Rossen_: Can you repeat it?
<dael> fantasai: Prop is a non-replaced element ignores transfered sizes for auto min sizing
<dael> fantasai: auto min siziing in flex and gri where initial value is auto. You take size of min-content so you don't shrink flex item too much. On images with aspect ratio if you set for row flexbox a height on the image we transfer that through a-r and modifies minimum
<dael> fantasai: Makes sense for images and that's what it was designed for. For non-replaced boxes this makes less sense. When we thought through we considered don't do this. Make this behavior restricted to replaced elements. Don't apply to non-replaced elements that have a-r added by a-r property
<Rossen_> q?
<dael> florian: You're left with min prior to a-r being used?
<dael> iank_: min-content or intrinsic
<dael> fantasai: For relaced element natural and min-content size is same.
<dael> fantasai: For repalced if you spec a size on the axis we clamp auto-min by that size.
<dael> fantasai: Other than that we transfer that and other conent and min that.
<dael> fantasai: Non-replaced we don't transfer
<dael> florian: If the transfer a-r would have made min size smaller clearly good to not do. If tranfer a-r makes min-size bigger is it not good to honor it?
<dael> fantasai: Trying to remember the logic
<dael> fantasai: This was because on items with an a-r...consider a block with a-r. min-height: auto. Decided that means element with an a-r will ignore a-r if content is larger than height it would have imposed by a-r. 1:1 a-r block. 100px width which gives 100h. 500px content so I have to block 500px
<dael> fantasai: When you turn it into a column flexbox we want same. But if we transfer size we can't get same behavior b/c made auto-min dictated by a-r. To have same behavior of the content blowing out the a-r in both blocks we can't use same transfer behavior we have for replaced
<dael> florian: Makes sense, thank you
<dael> fantasai: More like thanks cbiesinger
<dael> iank_: This means a min-content size will ignore tranferred min and max heights. But min-content contribution will respect them?
<dael> fantasai: min-content contribution will have to take into account...[thinks]...something complicated about this but I don't remember
<dael> fantasai: I think min-content contribution needs to take it into account. I remember complixity on how defined in a different issue and I'm not sure if it was relevent there
<dael> cbiesinger: min-content contribution doesn't matter?
<cbiesinger> s/?/in flexbox?/
<dael> cbiesinger: flexbox intrinsic sizes are defined in flexbox spec.
<dael> Rossen_: Got it
<dael> iank_: I asked about it to make sure it is applied there. grid spec does use the min-content contributions directly
<dael> iank_: In certain circumstances
<dael> fantasai: yeah
<dael> cbiesinger: I don't think we can use that in content contribution b/c don't know content-height. We would create circular
<dael> Rossen_: Yes
<dael> Rossen_: We're getting to top of hour. Doesn't sound like we're ready to resolve on this. not quite.
<dael> Rossen_: Perhaps I would suggest we take it back to the issue and answer the questions there and then resolve during the F2F. Let's make sure we're not missing any of the points raised here
<dael> Rossen_: Let's end here

@astearns astearns added this to the EUR VF2F-2021-04-06 milestone Apr 2, 2021
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-flexbox] [css-sizing-4] Interaction of flexbox minimum height and aspect-ratio minimum height, and agreed to the following:

  • RESOLVED: non-replaced elements with an aspect ratio honor the intrinsic size in flexbox for min-height:auto
The full IRC log of that discussion <astearns> topic: [css-flexbox] [css-sizing-4] Interaction of flexbox minimum height and aspect-ratio minimum height
<astearns> github: https://github.com//issues/6069
<fremy> cbiesinger: the aspect ratio spec says that min-height auto and you compute it using the aspect ratio, you expand to the content height
<fremy> cbiesinger: but in a column flex box, the height is the minimum of the transferred height and the content height
<fremy> cbiesinger: it's not clear to me which one should win
<fremy> cbiesinger: fantasai clarified the aspect ratio version should win
<fremy> cbiesinger: but does the working group agree to this?
<fremy> fantasai: this is different for replaced and non-replaced elements
<fremy> fantasai: because the intrinsic size of replaced elements is not as strict
<fremy> fantasai: so we need to keep that behavior for them
<fremy> fantasai: but for non-replaced elements, the intrinsic size is more meaningful
<fremy> florian: I'm not sure about what the alternative is?
<fremy> cbiesinger: width:100px + aspect ratio 1:1 but intrinsic-height:200px
<fremy> florian: ah ok, got it
<fremy> cbiesinger: because it used to be the minimum
<fremy> jensimmo_: the default of flexbox is that it's shrink then grow
<fremy> jensimmo_: but the proposal is that for replaced elements, we make aspect ratio "work"
<fremy> fantasai: yes, that is the heart of the issue
<fremy> (some discussion about magic css jokes)
<fremy> astearns: last week there was more contention
<fantasai> [jen's explanation was that if there's a bunch of elements you want to be squares, unless too much content want to grow, that's the behavior we specced for block boxes with aspect-ratio; and the issue is making sure it works the same for flexbox]
<fremy> iank_: that was me last week
<fremy> iank_: I am now fine with this
<fremy> astearns: is the spec edit correct for what we discussed
<fremy> fantasai: I think it is correct
<fremy> fantasai: but we need to update the spec
<fremy> fantasai: proposal is non-replaced elements with an aspect ratio honor the intrinsic size in flexbox
<cbiesinger> for min-height:auto
<fremy> jensimmo_: I am seeing a lot of tutorials about aspect ratio on replaced elements
<fremy> jensimmo_: in particular content-size fit etc
<fremy> fantasai: that should continue to work
<cbiesinger> s/content-size fit/object-fit/ I think?
<fremy> fantasai: that is why we don't do this for replaced elements
<fremy> fantasai: non-replaced elements have an intrinsic size that depends on content, that might not be easy to resize
<fremy> jensimmo_: the previous text for images would still apply then?
<fremy> fantasai: yes
<fremy> astearns: any objection to keep fantasai's edit in the ED?
<fremy> RESOLVED: non-replaced elements with an aspect ratio honor the intrinsic size in flexbox for min-height:auto
<fremy> <br length=15min />
<leaverou_> Partial regrets for Chris, he's having a migraine. May join later
<fremy> TabAtkins: might be a dwarf propaganda map, hiding their continent from the view of others then! that would be smart of them...
<TabAtkins> House Sivis is gnomish, not dwarvish
<fantasai> ScribeNick: fantasai

@bfgeek
Copy link
bfgeek commented May 21, 2021

I'm a little skeptical by the grid behaviour now - and it doesn't seem correct to me.

E.g. consider this testcase:
https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=9297

is it really correct that we ignore the transferred size suggestion for non-replaced elements?

@fantasai @tabatkins

@astearns astearns moved this from Layout to Math in EUR July 27 2021 vFTF Meeting Jul 24, 2021
@astearns astearns removed this from the EUR VF2F-2021-04-06 milestone Jul 24, 2021
aethanyc referenced this issue in web-platform-tests/wpt Sep 2, 2021
… element.

If the flex item is a non-replaced element and its min-width/min-height
is 'auto', the spec has changed so that it has no transferred size
suggestion now. https://drafts.csswg.org/css-flexbox-1/#min-size-auto

This patch also updates WPT tests to fix
#27878

Differential Revision: https://phabricator.services.mozilla.com/D112830

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1703304
gecko-commit: fb14e6d29a44a79efc296cc727f92aa58ea4dacc
gecko-reviewers: dholbert
@fantasai
Copy link
Collaborator
fantasai commented Sep 7, 2021

@bfgeek I think it is. The goal of the automatic minimum size is to prevent things from becoming so small that their contents are unreadable (overflowing or shrunk to zero). The preferred size still accounts for the aspect ratio, so if the column is allowed to grow wider the item will be square as intended, but when there isn't enough space in the column (and the item's own contents are small enough to fit), it won't overflow the column. That's a good behavior for the automatic minimum size, imho.

If the author wants the aspect ratio to force overflow, they can set the width or min-width to max-content instead of auto.

pull bot pushed a commit to jamlee-t/chromium that referenced this issue Sep 22, 2021
That was a WG resolution from quite a few months ago:
w3c/csswg-drafts#6069

Also update bug numbers of some tests that were assigned to this bug but
still do not pass.

Fixed: 1204827
Change-Id: I7ff892e751a21a4348d2bc7c18ca5eb1b085f940
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173718
Commit-Queue: David Grogan <dgrogan@chromium.org>
Auto-Submit: David Grogan <dgrogan@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/main@{#923654}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
That was a WG resolution from quite a few months ago:
w3c/csswg-drafts#6069

Also update bug numbers of some tests that were assigned to this bug but
still do not pass.

Fixed: 1204827
Change-Id: I7ff892e751a21a4348d2bc7c18ca5eb1b085f940
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173718
Commit-Queue: David Grogan <dgrogan@chromium.org>
Auto-Submit: David Grogan <dgrogan@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/main@{#923654}
NOKEYCHECK=True
GitOrigin-RevId: a9952c3d0b0e9db370849df4d9b6aae41c0ca9b1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

7 participants