-
Notifications
You must be signed in to change notification settings - Fork 708
[css-flexbox] should a definite flex-basis always make the main size be definite? #4311
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
Comments
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+ |
The CSS Working Group just discussed 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? |
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: 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). |
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.) |
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
+1. Last thing. I'm a little worried about performance. The original testcase has <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. |
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.
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 FWIW, Gecko treats the two scenarios the same ( |
The CSS Working Group just discussed
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> |
…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
…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
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:
Then we addressed the main topic, ensuring that a definite flex basis is always treated as 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...) |
As far as I can tell, this is already tested by https://github.com/web-platform-tests/wpt/blob/master/css/css-flexbox/percentage-heights-003.html |
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:
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).
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
The text was updated successfully, but these errors were encountered: