-
Notifications
You must be signed in to change notification settings - Fork 715
[css-grid] minmax(auto,min-content) under a max-content constraint #3565
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
I think you're right, something's not right here. Probably the automatic minimum should be limited by min-content here. |
OK, I think the correct edit here is to remove “if that is fixed” from the spec text you quote:
What do you think? |
I don't think the sentence makes sense without “if that is fixed”, because it's not clear how a length should be limited by something which is not a length or a resolveable percentage. For example, if the max track sizing function is the keyword It can also be a |
So “if that is fixed, else it's own min-content/max-content size if the max track sizing function is min-content/max-content” ? |
Basically yes, but IMO the sentence would bee too long and a bit confusing, there is already a min/max-content dichotomy for the size constraint, one could think that the this min/max-content is also with respect to that. I would prefer a refactoring like
|
… the items' contributions when sizing auto tracks under a min/max-content constraint. #3565 [Tentative pending WG review.]
OK, I split the sentence. Didn't refactor the whole thing, but did some editorial tweaking to try to make it clearer. Let me know if the edits look OK and we'll ask for WG review. :) |
Your refactoring with "limited min-/max-content contribution" looks good too. I'm just not sure about the use of "min-content/max-content size" instead of the preceding "min-/max-content contribution". Not much sure about the difference, I believe one is an inner size and the other an outer size, here you probably want the outer? And when it says "else by it’s own min-content/max-content size if that is min-content/max-content", "that" seems to refer to the own size instead of to the max track sizing function. Consider repeating "max track sizing function" to make it clear. |
You're right, that's clearly the wrong one. Done, and done. Thanks!!! |
Agenda+ to review the proposal: When sizing auto-minimum tracks under a min-/max-content constraint, we use the item's min-/max-content contribution. But we limit that contribution by the track's max size if it's fixed (length/resolved %). This change makes it so that we also limit the contribution in the case of a min-content/max-content max track size, which fixes Oriol's example above to yield more intuitive behavior. The value we're using to represent such an intrinsic max track size is the item's own contribution to such a size, which is I think a sufficient proxy for the actual max track size. |
@Loirooriol I think actually we can simplify to only do this for min-content max track sizes, limiting by max-content seems like it'd be a no-op? |
Sure, the non-fixed limit is only useful when the max track sizing function is |
Another case: Its max track sizing function is However, Chromium, Firefox and Edge produce a 50px column with |
@Loirooriol If the limited min-/max-content contribution were limited by the fit-content argument, fit-content(0) would never yield anything greater than a zero-width column. But that's not what happens. I think there is something to fix here, but it's not the same issue, exactly... |
Not true, thanks to the minimum contribution. |
Right. Forgot that. Fixed, I think? |
Wondering now whether to simplify out that no-op or leave it as-is because it more accurately reflects the principle of the thing... |
Yes, that's good. Well, the "could" sounds a bit strange to me, like implying uncertainty. Maybe slightly in favor of simplifying the no-op in order to shorten a bit the long sentence, but I don't care. |
I think this is the basic misunderstanding. The min-/max-content constraint is only used while determining the grid containers intrinsic size, not while flowing the box after that size has been determined. (i.e. the "Otherwise" part of "For auto minimums") I stepped through this in Gecko in a debugger and jotted down some notes in case it's helpful:
(I'll be happy to elaborate in more detail if needed.) AFAICT, Gecko implements exactly what the spec says and thus its rendering is correct. We could cap the item's max-content contribution also for |
I guess the problem is more float-related than Grid-related. It seems to be technically not defined yet how should the preferred width be calculated in presence of floats. My understanding is also that preferred width should account for all the floats and the content that flows around them when they are placed in the single line, but Gecko and WebKit/Blink seem to disagree. |
@MatsPalmgren You are right, I missed that layout is done again "for real" once the container size is known. Firefox follows the spec before fantasai's edits, but the result just seems weird to me. @SelenIT I floated the grid container in order to size it under a max-content constraint (assuming enough available space) instead of |
The CSS Working Group just discussed
The full IRC log of that discussion<dael> Topic: minmax(auto,min-content) under a max-content constraint<dael> github: https://github.com//issues/3565#issuecomment-461242081 <dael> astearns: Discussed last week. Sounds like the one thing waiting on is the verbiage for what to do when spans >1 <dael> astearns: Is fantasai on yet? <dael> astearns: Is there anyone else who wanted to go over last week's discussion or have anything new? <dael> astearns: Rossen it sounds like on this item we were waiting to resolve on fantasai addressing a comment from Mats. She did in the issue. Should we call for resolution? <jensimmons> oh sorry — I was totally confused. I thought you were asking for new agenda items for the call still… SORRY. That’s what I get for doing more than thing at the same time <dael> oriol: fantasai proposed edits and they're not yet in the spec. I had some complaints against them and they need to be tweaked a bit. It would be better to discuss with fantasai <dael> fantasai: I'm here <dael> astearns: oriol did you want to discuss on the call or do it in the issue? <dael> oriol: I wrote them in the issue. mostly it was that fantasai proposed to clamp as an upper limit that's the maximum of value. I thought it would make more sense to sum values rather than max of them. Some other minor corrections that maybe can be in github before making edits <dael> fantasai: It's a max instead of sum because we want it to just fit within the number of tracks it spans. If it spans 50px and then one that's min-content we want it to fit in 50px plus whatever is left. If we add to it the 50px we're giving it more space then it needs <Rossen> just a sed <dael> astearns: It sounds to me that there are 2 issues. THe one on the agenda and another that has some ramifications on this one? <Rossen> sec... having problem connecting on the phone <dael> florian: I think ramifications are a follow up to the thing on the agenda <dael> oriol: The item in the agenda was for the non-spanning case and Mats wanted to handle spanning case as a generalization of this and it make it more complicated <dael> astearns: Okay, thank you <dael> astearns: I could see two ways going forward. One is figure out the ins and outs of spanning case on the call. Sounds like discussion is between fantasai and oriol so might not be most efficient. Other is resolve on accepting these edits and if needed open a second issue for spanning case <dael> fantasai: Can resolve on principle of handling min content track sizes as a clamp on automatic sizes in a similar way fixed sizes are a clamp <dael> astearns: Make sense oriol ? <dael> oriol: Yes <dael> astearns: We're asking for 2 resolutions? One to accept the edits proposed in the issue and second is on principle? <dael> fantasai: Just principle. Deal with the edits in the issue that is open on that <dael> astearns: Issue on agenda needs resolution. Is resolving on the principle sufficient? <dael> fantasai: I think so <dael> astearns: Prop: Close the issue by resolving we will handlemin content track sizes as a clamp on automatic sizes <dael> astearns: Objections? <dael> Rossen: This is on #3565? <dael> falken: Yes <dael> s/ falken /fantasai <dael> Rossen: sgtm <dael> RESOLVED: we will handle min-content track sizes as a clamp on automatic sizes |
Some of my points from #2303:
|
A narrow fix here: The use of "limited min-/max-content contribution" in the non-spanning auto minimums step was wrong, it should have been only min all along. Unsure why we wrote it that way, but it came from 7cacfbf back in 2016, so probably we were still just trying to figure out how this works. In the OP's case this'll give us the Chrome behavior (a width of 25px). |
…ontent from above, since we didn't need to do that clamp in the first place. #3565
The CSS Working Group just discussed
The full IRC log of that discussion<fantasai> See https://github.com//issues/3565#issuecomment-1638901643<bramus> TabAtkins: if you look at very first comment, oriol has a test case here with some example and the wg decided earlier that chrome and edge are correct here. <bramus> … earlier we tried to implement that resolution in spec and we did it wrong, reverted, and did it again <bramus> … issue ended up being simpler thatn we thought <fantasai> Final fix was much simpler than we thought: https://github.com/w3c/csswg-drafts/commit/35ed93fc9296e6541e83c8cca4b705b7ba82c31a <bramus> … we believe only problem was in handling non spanning items in a max content constraint was <fantasai> s/max content/min content/ <bramus> … we were using the limitex max content size <dholbert> s/limitex/limited/ <bramus> … we should have been using limited min content size when resolving base sizes of tracks <bramus> … once we fixed that, algortihm made more sense in general <fantasai> s/of tracks/of auto tracks/ <bramus> … this text also dated back to 2016 which was early in grid spec text, so we believe this was a mistake we made early on <bramus> … this mistake was not copied over into spanning case but firefox behavior in previous issue was them doing the same thing in spanning case <bramus> … if you do it in either case, it should be ok. we think spec text is ok now. <bramus> astearns: you were nodding along? <bramus> oriol: yeah, we discussed yeasterday and I think the current solution is the correct one <bramus> astearns: other comments? <bramus> astearns: proposed resolution to accept latest change as the solution to the earlier resolution <bramus> astearns: objections? <bramus> RESOLVED: accept latest change |
Does this have a wpt test yet? |
Consider this grid: https://jsfiddle.net/xnvrk7h8/
float: left
sizes the grid under a max-content constraint (assuming the window is wide enough).The grid item has a max-content width of of 100px when the floats are in the same line, and a min-content of 50px when they are in different lines.
According to https://drafts.csswg.org/css-grid/#algo-single-span-items,
The track does have an
auto
min track sizing function and the grid container is being sized under a max-content constraint. So the track’s base size should be set to 100px. This is not limited bymin-content
because it's not fixed, and the minimum contribution is less than 100px:Here the computed
width
isauto
, and the used value ofmin-width: auto
is the content size suggestion, 50px.So the growth limit is initially set to 50px, but
So both the base size and the growth limit should be 100px, the track is frozen, and the rest of the algorithm doesn't seem relevant.
However, this is not what implementations do:
Note that in Firefox the column is 50px wide but the grid container is 100px wide, so strange, @MatsPalmgren. This is solved by using
min-width: 0
in the grid item, which alters the minimum contribution but shouldn't affect the result.Is my understanding of the spec correct? Should it be changed to match implementations?
The text was updated successfully, but these errors were encountered: