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] Definiteness of flex items' main size depend on flex-basis's definiteness #1679

Closed
gsnedders opened this issue Aug 2, 2017 · 20 comments

Comments

@gsnedders
Copy link
Contributor

@gsnedders gsnedders commented Aug 2, 2017

From email:

I've been looking through a few non-interoperable parts of flexbox again recently, and have run into https://codepen.io/gsnedders/pen/WRZQrx?editors=1100#0 again. Notably, Edge and Firefox both pass this test case, and Chrome and Safari fail. Except, as it turns out, the behaviour in Chrome and Safari is right. (Actually, the behaviour in Safari is wrong, it just happens to be right for that test case, but let's never mind details!)

I can't work out is where the behaviour in Edge and Firefox comes from, though maybe it is down to interpretation of the spec prior to 2f0ad00 and whether you need to measure content in this case? Though that doesn't seem true? I haven't dug too deep into this though!

That said, what I'm really here to ask is when and why we decided to go with the behaviour the spec and Chrome now have and hence against the Edge/Firefox behaviour. This seems to be something that trips web authors up a fair bit (there's lots of bugs in the Chromium bug tracker about this!). Something to point people at as to, "hey, here's why the spec does this nowadays, and why we think this is better behaviour", ideally—though obviously a blog post summarising it may well be needed!

Note that since this was written, Safari's behaviour is now genuinely right, because it now shares its flexbox implementation with Chrome.

@gsnedders
Copy link
Contributor Author

@gsnedders gsnedders commented Aug 2, 2017

https://codepen.io/gsnedders/pen/GvjRNN is one of the original cases that I started having problems with; unlike the codepen linked above it doesn't have the flex container having a definite size.

@gsnedders
Copy link
Contributor Author

@gsnedders gsnedders commented Aug 2, 2017

The conclusion at the F2F was to get opinions from implementors: @dholbert @FremyCompany @cbiesinger.

@gsnedders
Copy link
Contributor Author

@gsnedders gsnedders commented Aug 2, 2017

@gregwhitworth asked me to see what happened with Grid in this case, so I threw together https://codepen.io/gsnedders/pen/ZJpYBY (this is essentially a minimally different example, just using grid); Firefox, Chrome, and Safari do what I expect there. (Up-to-date Edge is still downloading as I write this.)

@gsnedders
Copy link
Contributor Author

@gsnedders gsnedders commented Aug 3, 2017

Microsoft EdgeHTML 16.16257 passes both flex and grid variants.

So we have interop on grid doing what most authors seem to expect; we have Firefox and Edge doing what authors expect with flexbox and Chrome and Safari not.

@gregwhitworth
Copy link
Contributor

@gregwhitworth gregwhitworth commented Aug 4, 2017

Yeah, I would strongly oppose changing our implementation to the one that authors aren't expecting, especially since the other engines are doing this in the grid case and we've tried, in many areas to keep flex/grid consistent. So if this works in Grid, and two impl can do it - then I think that Blink/Webkit should make it work for author ease and platform consistency.

@gsnedders
Copy link
Contributor Author

@gsnedders gsnedders commented Aug 4, 2017

Yeah, I would strongly oppose changing our implementation to the one that authors expect, especially since the other engines are doing this in the grid case and we've tried, in many areas to keep flex/grid consistent.

You mean away from the one that authors expect, I assume? Edge does what authors expect today.

@gregwhitworth
Copy link
Contributor

@gregwhitworth gregwhitworth commented Aug 4, 2017

@gsnedders yes you're right, I updated the comment so people don't get confused.

@fantasai fantasai added the Agenda+ label Aug 8, 2017
@tabatkins
Copy link
Member

@tabatkins tabatkins commented Aug 8, 2017

Issue here is that Grid's behavior comes from the fact that, while there may be some definite/indefinite shenanigans while figuring out how large the tracks are, once you do so, the tracks (and thus, the grid areas) have a definite size in px. If you then stretch the grid item into that space, they're considered definite as well, so child %s resolve.

Flexbox doesn't have this two-tier notion. There's nothing in the spec currently to support the flex item's main size being treated as definite in this case; it's auto-height, and none of the special cases in Flexbox/9.8 apply. If you or a Firefox dev can tell us how you're getting it to have a definite size, that would be great. ^_^

@dbaron
Copy link
Member

@dbaron dbaron commented Aug 16, 2017

/cc @dholbert

@css-meeting-bot
Copy link
Member

@css-meeting-bot css-meeting-bot commented Aug 16, 2017

The CSS Working Group just discussed Definiteness of flex items' main size depend on flex-basis's definiteness.

The full IRC log of that discussion <dael> Topic: Definiteness of flex items' main size depend on flex-basis's definiteness
<fantasai> https://github.com//issues/1679
<fantasai> github: https://github.com//issues/1679
<dael> fantasai: The grid has a behavior where we calc size of rows and columns whcih could depend on content we then do a pass to layout the items. So if the grid item is stretch or % it will resolve, as will any child.
<dael> fantasai: Seems like there are impl of flexbox with a similar effect where a flex item is considered definite even though spec says if flex-item size depends on content the % wont' resolve. TabAtkins and I are happy to define what impl want, but we need to know if this was intentional or not. It does require another pass.
<dael> fantasai: Basically, we want impl feedback.
<dael> TabAtkins: Specfically FF and Edge.
<fantasai> s/feedback/to say what they want us to put in the spec/
<dael> gregwhitworth: I was going to ask for another week if possible.
<dael> gregwhitworth: I was going to look into our code more. Unless Gecko has feedback now.
<dael> fantasai: Next week is good.
<dael> Chris: Sounds good gregwhitworth. ANyone from FF, can they do input?
<dael> fantasai: dbaron CCed dholbert.
<dael> Chris: Great. We'll defer.
@dholbert
Copy link
Member

@dholbert dholbert commented Aug 19, 2017

Quoting @tabatkins :

There's nothing in the spec currently to support the flex item's main size being treated as definite in this case; it's auto-height, and none of the special cases in Flexbox/9.8 apply. If you or a Firefox dev can tell us how you're getting it to have a definite size, that would be great. ^_^

In Firefox, we simply haven't gotten to implementing the relevant spec change/clarification (the change that said main-size definiteness depends on flex-basis's definiteness). bug 1092007 is filed on that, though.

I tend to think it's a change we should make, but I just haven't gotten to it.

@gregwhitworth
Copy link
Contributor

@gregwhitworth gregwhitworth commented Aug 22, 2017

We also haven't updated our implementation since 2015 for this section of spec text. By simply applying a definite flex-basis and a height of 0 in the demo above, this makes Chrome interoperable with Firefox and Edge. This leads me to wonder if the performance argument (what was brought up ~3 years ago) is worth the author confusion here, and while I understand why inline & block direction are different - currently FF/Edge react the same for the author in either row or column directions which further makes me desire the implementations of FF/Edge. I think we may have done a bit of pre-optimization when discussing this in 2014, I truly think we should remove the constraints for 2 under section 9.8 as that's what we're doing.

@css-meeting-bot
Copy link
Member

@css-meeting-bot css-meeting-bot commented Aug 23, 2017

The CSS Working Group just discussed Definiteness of flex items' main size depend on flex-basis's definiteness, and agreed to the following resolutions:

  • RESOLVED: Allow % to resolve, re-open issue if dholbert or others want to rediscuss.
The full IRC log of that discussion <dael> Topic: Definiteness of flex items' main size depend on flex-basis's definiteness
<dael> github topic: https://github.com//issues/1679
<dael> Bert: I think gregwhitworth wanted to look at code he had to compare.
<dael> gregwhitworth: Is anybody from FF on?
<dael> dbaron: I don't know if I'll be helpful.
<dael> gregwhitworth: I put my information in the notes. I sat with our dev. We could out the same thing as dholbert. We haven't impl this aspect. I pushed him tos ee if this is right approach. Dholbert seemed to say he thought it was the right approach.
<dael> gregwhitworth: WE do because everything is auto we do layout and then a second pass to resolve %. That's what authors also expect. I'm wondering if we should remove the constraing on #2 of 1.8. If you set definte flex basis and fixed height it does the extra work. I see a lot of authors doing this though it's not what they're expecting.
<dael> fantasai: Bigger argument is we have introp on 2 impl and the authors expect that behavior. Spec says something else so we should change spec.
<dael> gregwhitworth: And what dholbert posted to the list, we originally said hey performance. It's odd for authors to know that to set flex-basis to 0 is what you need to do for chorome to look like edge and ff.
<fremy> fwiw I discussed this with gregwhitworth yesterday, and I totally +1 his request to remove the constraints on #2
<dael> gregwhitworth: Let's just removet he constraints of #2 under 9.8
<dael> fantasai: Reasonable to me.
<dael> Bert: Me too, but I'm not expert. Other opinions?
<dael> dbaron: I'm not sure how well performance bugs get tied back to this. I'm not sure if we heard there were performance bugs.
<dbaron> s/heard/would have heard/
<dbaron> s/there/if there/
<dael> gregwhitworth: That's a valid arguments. I feel like we're going to argue a theory. Now that we have grid we'll be able to have less nested flexes is my guess so I'm not as worried about the circular problem.
<dael> fantasai: IN grid % always resolved b/c grid sizing creates sizes for the tracks. Conseptually grid algo resolves the eq. code the way FF and Edge do flex right now.
<dael> Bert: I'm hearing, fantasai , that there are impl that do and some that don't do the layout based sizing?
<dael> gregwhitworth: Chrome and I believe I saw webkit and blink would have to change. As we stated the people on the thread aren't on the call.
<gsnedders> FF and Edge agree, Chrome and Safari agree (but are one impl now again)
<dael> Bert: Are we confident enough to decide? Do we need more input?
<gsnedders> WebKit's implementation of flex was literally copied over from Blink a few months ago
<dael> fantasai: Christian is away for the next couple months. We can ask dholbert that we want this resolution and ask for approval. From dholbert and whomever people want to tag.
<dael> Bert: Sounds like a good way forward. If we document it in the issue is that enough? Do we need to contact personally?
<dael> gregwhitworth: I say we just resolve since we discussed it. We can rediscuss if they want to get on a call
<dael> Bert: Okay.
<dael> Bert: wording for proposed resolution:
<dael> gregwhitworth: It's what I said in #2. Basically re-wirte #2 of section 9.8 to % would resolve.
<dael> fantasai: Allow % to resolve, re-open issue if dholbert or others want to rediscuss.
<dael> Bert: Okay, that's the proposal. Objections?
<gregwhitworth> s/it's what I said in #2/it's what I said in the issue
<dael> RESOLVED: Allow % to resolve, re-open issue if dholbert or others want to rediscuss.
@fantasai fantasai changed the title Definiteness of flex items' main size depend on flex-basis's definiteness [css-flexbox] Definiteness of flex items' main size depend on flex-basis's definiteness Sep 6, 2017
fantasai added a commit that referenced this issue Sep 6, 2017
@fantasai
Copy link
Collaborator

@fantasai fantasai commented Sep 6, 2017

Edits checked in. @gregwhitworth @gsnedders would you mind reviewing the edits to make sure that's what you intended?

@gregwhitworth
Copy link
Contributor

@gregwhitworth gregwhitworth commented Sep 6, 2017

Yep, just removal of the definite flex basis - looks good.

@gsnedders
Copy link
Contributor Author

@gsnedders gsnedders commented Sep 7, 2017

To check: in https://codepen.io/gsnedders/pen/GvjRNN .flex-container is definite, right (given the height: 0 makes it so)? If so, then I'm okay.

@fantasai
Copy link
Collaborator

@fantasai fantasai commented Sep 11, 2017

@gsnedders Yes, because both the height and the min-height are definite in this example. (height is fixed, and min-height is resolved against a definite height).

@astearns
Copy link
Member

@astearns astearns commented Sep 27, 2017

Reopening for testcase need.

@gregwhitworth
Copy link
Contributor

@gregwhitworth gregwhitworth commented Oct 23, 2017

@fantasai @gsnedders I created a testcase for this - please review at earliest convenience.

@gregwhitworth
Copy link
Contributor

@gregwhitworth gregwhitworth commented Nov 3, 2017

The test for this one has been merged, so I'll be removing the label for tests.

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
8 participants
You can’t perform that action at this time.