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] should a definite flex-basis always make the main size be definite? #4311

Closed
cbiesinger opened this issue Sep 16, 2019 · 9 comments
Labels
css-flexbox-1 Current Work Tested Memory aid - issue has WPT tests

Comments

@cbiesinger
Copy link

https://drafts.csswg.org/css-flexbox/#definite-sizes

Currently, a main size is only made definite if the flex container has a definite height.

However, browsers do not quite implement that. See https://crbug.com/1003506 for some discussion, but basically:

<div style="display: flex; flex-direction: column; width: 100px; background: yellow;">
  <div style="height: 100px; flex: 1 1 auto; background: green; min-height: 0;">
    <div style="height: 100%; background: red;"></div>
  </div>
</div>

An element that would have a definite height outside of flex now suddenly has an indefinite height when put in a flexbox, because it's flexible (at least according to the spec, not in impls).

  • Should that change?
  • Would implementing this spec-compliant be web-compatible?

Also note that Chrome currently resolves percentages only if the main size property is definite; a definite flex-basis is not enough. That should probably be changed.

@dholbert @fantasai @tabatkins

@cbiesinger cbiesinger added the css-flexbox-1 Current Work label Sep 16, 2019
@fantasai
Copy link
Collaborator

The main reason we restrict percentages from resolving in various indefinite circumstance is that, at least historically, it was hard to do. Afaict, generally authors prefer if percentages resolve. So if implementations are resolving in more cases than specced here... seems like we should just update the spec to match implementations?

Agenda+

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Does definite flex-basis always cause main size definite?.

The full IRC log of that discussion <fantasai> Topic: Does definite flex-basis always cause main size definite?
<fantasai> github: https://github.com//issues/4311
<fantasai> TabAtkins: There's an example in the issue
<fantasai> TabAtkins: Column flexbox with a flexible child
<fantasai> TabAtkins: child has a definite height, but it is flexible so can become larger or smaller
<fantasai> TabAtkins: Flex item has a child with a percentage height
<TabAtkins> my phone hung up, one sec
<TabAtkins> OKAY I CAN'T MAKE A CALL ANY MORE
<TabAtkins> welp
<fantasai> fantasai: Spec says this case is indefinite, and percentage won't resolve
<fantasai> fantasai: But implementations do resolve
<fantasai> fantasai: so should we change the spec to match implementations?
<fantasai> iank_: I think all of the implementations are following the spec here
<fantasai> iank_: in that they are not resolving ...
<fantasai> cbiesinger: Not true
<fantasai> cbiesinger: Testcase is red, which means it is resolving
<fantasai> cbiesinger: Interesting thing here is that the percentage would resolve outside a flexbox
<fantasai> cbiesinger: it doesn't resolve if you put it inside the flexbox (per spec)
<fantasai> cbiesinger: I doubt authors expect that
<fantasai> fantasai: Originaly definiteness was identifying cases where percentages are very simple to calculate
<fantasai> fantasai: and we restricted percentage resolution to these cases
<fantasai> fantasai: but authors would be happier to resolve more often
<fantasai> fantasai: so if implementers are already doing it, might as well match the spec
<fantasai> iank_: this seems fine to me
<fantasai> fantasai: does mean that concept of definiteness is more complicated
<fantasai> cbiesinger: Want to mention, in Chrome we only take into account the width/height property, not the flex-basis. But that's probably a Chrome bug
<fantasai> Rossen_: Do we know if this is safe to change?
<jensimmons> q+
<fantasai> fantasai: Planning to match spec to implementations, so shouldn't have any concerns
<dholbert> q+
<Rossen_> ack jensimmons
<fantasai> jensimmons: Seems folks doing a good job of raising flex bugs and addressing them
<fantasai> jensimmons: Would love to ask if filing bugs against WebKit as well, seems our implementation is same as Chrome
<fantasai> iank_: No bug required here, spec is aligning ot implementations
<fantasai> jensimmons: on this issue, but not last one
<fantasai> cbiesinger: should be a wpt test also
<fantasai> Rossen_: WPT tests should raise to webkit
<jensimmons> no
<Rossen_> ack jensimmons
<Rossen_> ack dholbert
<fantasai> dholbert: Wanted to clarify what the proposed change is here. Are we making the spec match implementations in this case?
<cbiesinger> I think change is: definite flex-basis makes the size definite, even if the item is flexible
<fantasai> dholbert: I think that doesn't match what we do
<fantasai> dholbert: It might be that this case is triggering a more subtle situation
<fantasai> fantasai: That's interesting, we should investigate
<fantasai> dholbert: I think intent of our behavior is to match the spec
<fantasai> dholbert: We do check if flex item is flexible when testing for flexible flex-basis, to see if we want to make item definite
<fantasai> cbiesinger: I feel like you and I had a discussion about this years ago and you were going to match our behavior
<fantasai> dholbert: We fixed a number of bugs in last 6 months also
<fantasai> dholbert: so maybe previously did something more esoteric
<fantasai> fantasai, rossen: maybe need to go back to GH to figure out
<Rossen_> q?

@dholbert
Copy link
Member

dholbert commented Jul 21, 2021

In the CSSWG discussion, I had misremembered that we implemented the "fully inflexible" requirement here, but it seems that in fact we tried but weren't able to do so because it's not web-compatible, as noted in these code-comments:
https://searchfox.org/mozilla-central/rev/699174544b058f13f02e7586b3c8fdbf438f084b/layout/generic/nsFlexContainerFrame.cpp#2167-2173
https://searchfox.org/mozilla-central/rev/699174544b058f13f02e7586b3c8fdbf438f084b/layout/generic/nsFlexContainerFrame.cpp#2130,2133-2135
and this chromium issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=1003506#c7

So I agree that we should just remove the "fully inflexible" restriction here, to match implementations & webcompat requirements (which are what caused this implementation behavior).

@dholbert
Copy link
Member

dholbert commented Jul 21, 2021

In the meeting, I think we were pretty much ready to resolve here, except for my pushback; and (now that I've refreshed my memory) I'm now withdrawing my pushback & am happy to resolve on removing the "fully-inflexible" requirement (and just considering the main-size to be definite for any flex item with a definite flex basis).

One observation, though - when we update the spec text for this, we probably need to write the amended condition as actual authoritative spec-text rather than in a Note. (The spec-text under discussion here is currently a "Note" and seems to be written as if it's just highlighting something that is self-evident; but the new condition here won't really be self-evident anymore.)

@astearns astearns added this to the APAC VF2F-2021-07-29 milestone Jul 24, 2021
@astearns astearns added this to Later agenda in APAC July 29 2021 vFTF Meeting Jul 24, 2021
@astearns astearns modified the milestones: APAC VF2F-2021-07-29, EUR VF2F-2021-04-06, EUR VF2F-2021-07-27 Jul 24, 2021
@davidsgrogan
Copy link
Member

This change is good.

Compat will probably be ok, especially if Blink and Gecko sync up shipping this change.

To be clear, this will be a change for Blink. Blink's existing behavior is intentional that the red block's percent height resolves in the original test case above, but that giving the item height:auto and flex-basis:100px would make it not resolve. This is intentional because my understanding of https://drafts.csswg.org/css-flexbox/#definite-sizes and the prose in https://drafts.csswg.org/css2/#the-height-property indicates this behavior complies with the current specifications.

One observation, though - when we update the spec text for this, we probably need to write the amended condition as actual authoritative spec-text rather than in a Note.

+1.
I further contend that the Note even as written today is inaccurate. Removing the Note and specifying this new behavior as normative would be wonderful. I think it would also let us close issue 4305.

Last thing. I'm a little worried about performance. The original testcase has min-height: 0. But most flex items on the web will have to rely on automatic minimum sizing like the case below. So in the below case Blink will lay out the item with an indefinite height to get its content size suggestion, which will be 0. Then Blink will lay out the item post-flexing with a definite height which will cause the subtree to need a second layout, doubling layout time. If this pattern is nested the slowdown gets exponential. Would this cause any potential perf regressions for Gecko?

<div style="display: flex; flex-direction: column; width: 100px; background: yellow;">
  <div style="flex: 1 1 100px; background: green;">
    <div style="height: 100%; background: red;"></div>
  </div>
</div>

Anyway, I haven't dug deeper on this potential performance issue, so if no one else is worried about performance, we shouldn't block this change just because of that.

@dholbert
Copy link
Member

dholbert commented Jul 29, 2021

Compat will probably be ok, especially if Blink and Gecko sync up shipping this change.
[...]
Would this cause any potential perf regressions for Gecko?

I believe Gecko is already shipping the proposed behavior -- if there's a definite flex-basis, then we treat that as making the resolved main size definite.

To be clear, this will be a change for Blink. Blink's existing behavior is intentional that the red block's percent height resolves in the original test case above, but that giving the item height:auto and flex-basis:100px would make it not resolve.

Interesting! That seems like an odd inconsistency which is nice that we can resolve here. :) (It seems quite weird to condition on the definiteness of the specified height in that example, since the specified height is otherwise ignored for the purpose of computing the actual height [aside from indirectly influencing the resolved automatic minimum height].)

FWIW, Gecko treats the two scenarios the same (flex-basis:100px;height:auto and flex-basis:auto;height:100px), from the perspective of this issue -- we treat the flex item's final height as definite, since the flex basis is definite in both cases. (Per the previous spec text, we technically should have been also checking if it was fully-inflexible, but that requirement didn't seem to be web-compatible.)

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed should definite flex-basis make main size definite?, and agreed to the following:

  • RESOLVED: accept proposal and remove note
The full IRC log of that discussion <fantasai> Topic: should definite flex-basis make main size definite?
<fantasai> github: https://github.com//issues/4311
<fantasai> dgrogan: Note in 9.8 ...
<fantasai> TabAtkins: That note should not be adding any conditions ever
<fantasai> dgrogan: Suggestion is to make this note normative and expanding cases where flex item can have a definite size
<fantasai> dholbert: This is issue that ?? brought up last meeting
<fantasai> dholbert: I wanted to check what Firefox did
<fantasai> s/??/biesi/
<fantasai> dholbert: I did follow up, and we do agree with the behavior that cbiesinger and dgrogan are proposing
<fantasai> TabAtkins: this has nothing to do with the note
<fantasai> TabAtkins: This issue is about a flexible flex item with a definite flex basis
<fantasai> dgrogan: That note is inaccurate
<fantasai> dgrogan: Definition of definite is you don't need to lay out to get its size
<fantasai> dgrogan: but many cases here has auto min size
<fantasai> dgrogan: If we go with this idea in the spec issue, can just get rid of the note
<fantasai> dgrogan: I think we're all on board with the idea behind this
<fantasai> dgrogan: do have question about a corner case
<dgrogan> https://jsfiddle.net/dgrogan/4r9npf3z/3/
<fantasai> dgrogan: the flex basis is 'content', but the height of ??? is definite
<fantasai> dholbert: you're talking about 'height: definite'
<fantasai> dholbert: In that case height property is ignored.
<fantasai> dholbert: only affects flex item if 'flex-basis: auto'
<fantasai> dgrogan: Child of the flex item with percentage
<fantasai> dgrogan: resolves against containing block
<fantasai> dgrogan: "percentage is calculated with respect to the height of the containing block. If not specified explicitly, computes to auto."
<fantasai> dgrogan: doesn't trigger behaves as auto clause
<fantasai> dgrogan: I suspect this is minor editorial oversight
<fantasai> dholbert: This is just CSS2 believing that only 'height' property can affect height
<fantasai> dholbert: whereas in flex layout, there's other factors
<fantasai> astearns: So additional normative change that cbiesinger suggested, sounds like we're in agreement to spec
<fantasai> TabAtkins: we will add 4th condition to definiteness of flex items, that definite flex basis always make correspondign axis of flex item definite
<fantasai> TabAtkins: consequently can remove the note
<fantasai> astearns: further comments?
<fantasai> RESOLVED: accept proposal and remove note
<fantasai> <br duration=12m>

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 31, 2021
…tation disagreeing with spec (now that spec has changed). r=TYLin

DONTBUILD

This patch doesn't impact behavior; it's just removing code-comments that refer
to a requirement in the spec that we don't honor, & which the CSSWG has now
resolved to remove (which makes our existing behavior correct & no longer
noteworthy).

The CSSWG resolution is in w3c/csswg-drafts#4311 .
The new text is yet-to-be-written, but the main takeaway is that the "fully
inflexible" requirement (which we ignore) has been removed; so now, any flex
item with a definite flex-basis should have its main-size considered to be
definite.  (And this matches our implementation.)

Differential Revision: https://phabricator.services.mozilla.com/D121284
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Aug 4, 2021
…tation disagreeing with spec (now that spec has changed). r=TYLin

DONTBUILD

This patch doesn't impact behavior; it's just removing code-comments that refer
to a requirement in the spec that we don't honor, & which the CSSWG has now
resolved to remove (which makes our existing behavior correct & no longer
noteworthy).

The CSSWG resolution is in w3c/csswg-drafts#4311 .
The new text is yet-to-be-written, but the main takeaway is that the "fully
inflexible" requirement (which we ignore) has been removed; so now, any flex
item with a definite flex-basis should have its main-size considered to be
definite.  (And this matches our implementation.)

Differential Revision: https://phabricator.services.mozilla.com/D121284
@tabatkins
Copy link
Member

All right, we've made a handful of edits here.

First, we rephrased the definition of "main size" a little, mostly for clarity, but added a line that makes it clear that the post-flexing main size is used when any layout algo is asking for the "used width" or "used height" (as appropriate). Small detail, but tied into this topic:

The main size of a flex container or flex item refers to its width or height, whichever is in the main dimension. Its main size property is either its width or height property, whichever is in the main dimension. Likewise, its min and max main size properties are its min-width/max-width or min-height/max-height properties, whichever are in the main dimension, and determine its min/max main size.
In flex layout, the main size is controlled by the flex property rather than directly by the main size property.

Note: This means any references to a flex item’s used size in the main dimension (width, height, inline size, block size) refers to its post-flexing main size.

Then we addressed the main topic, ensuring that a definite flex basis is always treated as definite:

  • If the flex container has a definite main size, then the post-flexing main sizes of its flex items are treated as definite.

  • If a flex-item’s flex basis is definite, then its post-flexing main size is also definite.


One side-issue that showed up in this thread was some minor confusion about applying CSS2 layout definitions, which explicitly reference the 'width' or 'height' properties' computed values for certain things, when for a flex item it's intended that you pay attention to the flex-basis instead. We're not sure how to actually fix this without going in and editting CSS2 in an invasive way and making it reference flexbox. :/ Hopefully it's okay without that clarification? (We can't just say "whenever another spec referenced the main size property, use the flex-basis instead", because things like inheritance do need to actually get the value of the 'width' or 'height' property...)

@tabatkins
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-flexbox-1 Current Work Tested Memory aid - issue has WPT tests
Projects
No open projects
Development

No branches or pull requests

7 participants