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-grid] Distribute extra space into non-intrinsic tracks instead of going beyond growth limits #3648

Closed
Loirooriol opened this issue Feb 12, 2019 · 13 comments

Comments

@Loirooriol
Copy link
Contributor

From https://drafts.csswg.org/css-grid/#extra-space,

  1. Distribute space to base sizes up to growth limits: Find the item-incurred increase for each spanned track with an affected size by: distributing the space equally among such tracks
  2. Distribute space beyond growth limits: If space remains after all tracks are frozen, unfreeze and continue to distribute space

However, the item could be spanning a track without intrinsic sizes whose base size still hasn't reached its growth limit. Wouldn't it make more sense to increase the base size of such track before increasing intrinsic tracks beyond growth limits?

For example, https://jsfiddle.net/phrwn2cd/

.grid {
  display: grid;
  grid-template-columns: minmax(0, 100px) minmax(auto, 100px);
  width: 0;
}
.item {
  grid-column: 1 / 3;
  width: 250px;
  height: 100px;
  background: cyan;
}

The columns are 0px 250px but instead I would expect 100px 150px, i.e.

  1. Distribute space to affected base sizes up to growth limits
  2. Distribute space to non-affected base sizes of spanned tracks up to growth limits
  3. Distribute space beyond growth limits
@fantasai
Copy link
Collaborator

Your proposed algorithm would assign too much space to the intrinsic spanned tracks, ignoring the fixed tracks. For example, if I distribute 100px between those two columns, I'd get 0px 100px. Is that really what's wanted?

@fantasai
Copy link
Collaborator

I guess it's how it works currently, though...

@Loirooriol
Copy link
Contributor Author

Yes, currently no contribution is distributed into such fixed tracks. I think it should happen at least if the intrinsic tracks reach their growth limits, but I'm open to taking it further and distribute equally from the very beginning.

@dholbert
Copy link
Member

CC @MatsPalmgren

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Distribute extra space into non-intrinsic tracks instead of going beyond growth limits, and agreed to the following:

  • RESOLVED: When calculating content based base sizes of spanning items, if extra space needs to be distributed past the growth limit, and there is a range available in another column, put it in there
The full IRC log of that discussion <heycam> Topic: Distribute extra space into non-intrinsic tracks instead of going beyond growth limits
<heycam> github: https://github.com//issues/3648
<heycam> fantasai: let's say there are two columsn
<heycam> ... one is minmax(0, 100px), the other is minmax(auto, 100px)
<heycam> ... normally we do a first pass to figure out hte base size
<heycam> ... min size of each column
<heycam> ... first is fixed and gets 0
<heycam> ... the seoncd is roughlty hte min content contribution of any items in that column
<heycam> ... let's support that item is 150px
<heycam> ... the max specified here is 100px
<heycam> ... since the first column says I'm 0px, we put all 100 or 150px into the second column
<heycam> ... and blow out the maximum
<heycam> ... in order to fit the content
<heycam> ... but the author did give us some space, first column has a range
<heycam> ... instead of blowing out the max on the second column, we could take those 50px and put them into the first column
<heycam> ... and it wouldn't violate any of the max constraints on the columns
<heycam> ... we ran this idea by jensimmons_ and rachelandrew_ and they both said yeah we'd expect that to happen
<heycam> ... so I'm suggesting adjusting the grid alg so that when computing hte min size of a spanning item, if you hit the growth limit, and have another that is still be low its max, you increase that one
<heycam> TabAtkins: sounds reasonable
<heycam> rachelandrew_: I like this because it is more similar to the flex behavior, which people will expect
<heycam> ... why overflow the column when there is still space there
<heycam> ... this could happen when you have a single row grid, and they're using grid for whatever reason, and they'll expect that space to be used
<heycam> iank_: what happens if the item is 150px in width? are the columns 50px and 100px?
<heycam> fantasai: yes
<heycam> jensimmons_: right now they're 0 and 150 but we're proposing they be 50 and 100
<heycam> Rossen: any objections?
<heycam> RESOLVED: When calculating content based base sizes of spanning items, if extra space needs to be distributed past the growth limit, and there is a range available in another column, put it in there
<heycam> iank_: you'll need to specify which columns take the slack
<heycam> ... 2 on the left and 2 on the right
<heycam> fantasai: all of them

@fantasai
Copy link
Collaborator

Note to self: we need to be careful when editing to correctly handle minmax(0, min-content) minmax(max-content, 100px). (And should make sure there's a WPT for it.)

@tabatkins
Copy link
Member

Upon reviewing other edits, we realized this was done slightly incorrectly - the "distribute beyond limits" part was meant to only distribute to preferred tracks when the alg is invoked with preferred tracks, but we accidentally wrote it to care about whether there are any preferred tracks. That is, passing a set of preferred tracks that happens to be empty will be treated the same as not passing preferred tracks at all; both will cause the distribution algo to go beyond limits for all affected tracks, which is not what we want. (If we try to pass preferred tracks, and there happen to not be any, it simply shouldn't distribute any space beyond limits.)

So we're still working on the wording a little, but we've fixed up the language to be more explicit about distinguishing between "didn't pass that argument" and "passed the argument, even if it's an empty list".

tabatkins added a commit that referenced this issue Mar 4, 2022
@fantasai
Copy link
Collaborator

fantasai commented Mar 4, 2022

OK, so we think the edits are in. However, there's this problem:

Suppose I have three tracks with minmax(0, 10px), and 3px of available space. I put a 20px item into the first one. It doesn't care, because it only has fixed sizes. During Maximize Tracks, we distribute free space equally among all the tracks; 1px each.

Now suppose I make the 20px item span the first two tracks. Under the previous spec, none of the tracks care about this item. But under the new one, we distribute its space into the first two tracks, each getting 10px. Maximize Tracks then gives the third track 0 space.

  • Under the previous spec, we would have three 1px tracks: the item would overflow its tracks, but the grid would not overflow its container, and the tracks that have fixed-size functions would be treated all equally.

  • Under the new one, only if the item spans multiple columns, we take space for the item regardless of whether there is any free space, possibly overflowing the container in order to minimize items overflowing within the grid, and the equal fixed-size functions are not sized equally.

What do we actually want to do here? Some possible options that are at least consistent between spanning and non-spanning items:

  • Fixed-size tracks completely ignore their content, as in the first edition.
  • Fixed-size tracks prioritize accommodating content over equal distribution of free space, and may use up more space than the available free space, causing the grid to overflow its container.
  • Fixed-size tracks prioritize accommodating content over equal distribution of free space, but cannot use up more than the available free space, and will allow the item to overflow before the grid itself overflows.

@tabatkins
Copy link
Member

Temporarily reverted these changes, pending us figuring out the story for non-spanning tracks in similar situations.

@Loirooriol
Copy link
Contributor Author

IMO fixed-size tracks should completely ignore contents that don't span any intrinsic track. Only when they span some intrinsic track, but these intrinsic tracks have reached a limit, the fixed tracks may help absorbing part of the contribution. So we should not be changing whether the grid will take an intrinsic contribution of an item into account, just how it's distributed among tracks.

@atanassov atanassov moved this to Tuesday in 2022 New York Meeting Jul 31, 2022
@astearns astearns moved this from Tuesday to Unsorted in 2022 New York Meeting Aug 2, 2022
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-grid] Distribute extra space into non-intrinsic tracks instead of going beyond growth limits, and agreed to the following:

  • SUMMARY: Would benefit from whiteboard discussion
The full IRC log of that discussion <TabAtkins> fantasai: I flagged this as f2f bc it needs whiteboard discussion
<TabAtkins> [we're punting for now bc it's v complicated and wants proper set up]
<fantasai> SUMMARY: Would benefit from whiteboard discussion

@atanassov atanassov added Agenda+ Whiteboard This is a large discussion that will benefit from whiteboard discussion. and removed Agenda+ F2F labels Mar 13, 2023
tabatkins added a commit that referenced this issue Jul 17, 2023
…space from spanning items when intrinsic-range tracks are filled up, if possible. #3648
@tabatkins
Copy link
Member

Okay, after review together with @fantasai and @Loirooriol we believe we fixed this issue satisfactorily, without unintended side effects as far as we can tell.

@tabatkins tabatkins removed Agenda+ Whiteboard This is a large discussion that will benefit from whiteboard discussion. Target Revision: Next Remove when done. labels Jul 17, 2023
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-grid] Distribute extra space into non-intrinsic tracks instead of going beyond growth limits, and agreed to the following:

  • RESOLVED: Accept edits
The full IRC log of that discussion <fantasai> We're discussing this case:
<fantasai> grid-template-columns: minmax(0, 100px) minmax(auto, 100px);
<fantasai> item spanning both columns
<dbaron> ScribeNick: emilio
<emilio> oriol: In the case we have an item with 2 tracks, one with an intrinsic min but fixed max, which spans also a track with both minmax
<emilio> ... instead of forcing the intrinsic track to grow beyond, it could be absorbed by the other track
<emilio> ... we have a resolution on this but edits missing
<emilio> ... yesterday we met and changed the spec
<fantasai> Edits to the specification: https://github.com/w3c/csswg-drafts/commit/0cadf5297d3fd3584ca8a74ac0a86d6f28ab4ca2
<emilio> ... it'd be great if you could review and agree with it
<TabAtkins> in the above case, the old spec would give as much space as it could do the second track (the intrinsic one), then if there's space leftover it would keep growing the intrinsic one past its max.
<emilio> ... the idea would be that we size the intrinsic tracks, then if all of them reach their limit then we check if there are other tracks that the item spans that could grow, and if so they'd absorb some contribution. If they also hit the limit then the intrinsic tracks grow beyond their limit
<emilio> ... so it's just inserting that middle step
<emilio> q+
<TabAtkins> now we'll first give space to the intrinsic track, then give space to the fixed track, *then* blow past the intrinsic track's limit only if we're *still* out of space
<astearns> ack hober
<emilio> fantasai: [explains the problem in a whiteboard]
<emilio> astearns: for this example how are we chosing to fit the right col first?
<emilio> fantasai: track sizing has different phases
<emilio> ... the first one is the minimum
<emilio> ... then the max
<emilio> ... we're figuring out the minimum
<emilio> ... and we know the min of the previous track is 0
<astearns> ack emilio
<iank_> q+
<TabAtkins> emilio: does this require any etra layout passes? expl sounds lik eno
<TabAtkins> fantasai: right, no extra pass
<emilio> fantasai: when we distribute space for intrinsic sizes the algorithm distributing across multiple tracks goes through two phases
<emilio> ... [explains the case with an extra auto column]
<florian> q+
<astearns> ack dbaron
<emilio> dbaron: some of what you were talking about reminds me about column-spanning cells in tables
<emilio> ... in particular about what happens w/ multiple spanning things that are partially overlapping but that could distribute across multiple columns
<emilio> ... curious what you do here to avoid ...
<emilio> ... I haven't heard talking about the order of the passes here or if you require multiple passes ...
<emilio> ... or how you avoid to favor one track over other ...
<emilio> fantasai: there are three phases, but we don't want to favor one item over another
<emilio> ... the algorithm tracks the current size and the planned increase separately
<emilio> ... if I have multiple items spanning tracks
<emilio> ... in order to prevent the ordering dependency we check the current size and then the increase
<emilio> ... so if I have item 1 spanning cols 1 and 2 and item 2 spanning columns 2 and 3
<TabAtkins> (note that elika is currently describing how the grid algo has worked for many years; we didn't change this)
<emilio> ... item 1 is 100px and it divides evenly 50px into each track, so we track the planned increase is 50px each. item 2 is 150px and it wants to distribute 75px per track
<emilio> ... so we adjust the tracked increase on the columns
<emilio> dbaron: I think the interesting case is if item 2 is smaller
<emilio> TabAtkins: it's not order-dependent for items within the same span
<emilio> dbaron: that's roughly the table solution
<astearns> ack iank_
<emilio> iank_: conceptually you're tracking two sets of tracks, one if you don't need to blow out the limits and one if you don't
<emilio> fantasai: no, blowing out the limits happens per span
<emilio> ... we only grow the content once and up to the limit. As we hit the limit we freeze those and keep growing
<emilio> ... if we run out of space is when we unfreeze all the tracks
<emilio> ... we propose to add a new phase to see if any of the fixed (ignored) tracks are able to grow
<emilio> iank_: I think the way we impl this is tracking the items in (1) and (3) separately
<emilio> iank_: need to think a bit more
<emilio> fantasai: one of the side-effects of this is that you're a lot less likely to blow out the limits, which seems good
<emilio> ... the other side effect is that all of the tracks that have fixed sizes could end up having a different size that the tracks that have an identical track-sizing function because of the spanning item
<emilio> ... the width of the grid doesn't change because the extra space would be going into the other track
<emilio> ... so we're not changing the total size of the grid
<emilio> dholbert: I wonder if an item spanning both auto-sized tracks and an item spanning the previous non-auto-sized track would end up growing the grid
<emilio> ... I think right now the first item would inflate track (3) to 220px and not contribute to the first track
<florian> q?
<emilio> ... that is if a later auto track ends up getting inflated due to other items
<emilio> ... but this change would also inflate the (1)
<emilio> fantasai: for that case if we got that right for (1) and (3) then (2) shouldn't be a problem
<florian> pre-existing issue on sizing with multiple spanners: https://github.com//issues/2356
<emilio> florian: I think this change doesn't introduce new problems but there are existing ones
<emilio> florian: I believe we have a problem for that case but this proposal doesn't make it worse
<emilio> dbaron: based on my experience with tables you can't fix both the non-order-dependent and the minimal answer
<emilio> iank_: it'd be still true that if there's an fr track we'd still stuff everything there?
<emilio> TabAtkins: yes, if there's an fr track we don't do this intrinsic sizing step at all
<emilio> ack florian
<astearns> ack florian
<emilio> florian: I agree there's a conflict with minimizing the space and not taking the ordering dependence. not sure I agree on which one to prioritize
<TabAtkins> (rather, if an item spans an fr track it is completely skipped by these intrinsic steps)
<emilio> fantasai: but that's a great q dholbert because looking at the algo I'm not sure we got that correct
<emilio> astearns: sounds like 1 way forward would be to accept this change and continue looking at the other cases
<emilio> fantasai: I think we're not introducing a problem but there might be a pre-existing problem
<emilio> florian: this is the one I pasted. If we find a break this week to revisit we should consider revisiting
<emilio> ... I think it's addressable, but not sure how without breaking the non-ordered-dependency
<emilio> fantasai: I'm sure it's addressable with that because I figured it out a long time ago
<emilio> ... so proposal is to accept edits and continue looking at the overlapping spans issue
<emilio> dbaron: [tries to convince florian of the ordering vs. optimal width conflict in the whiteboard]
<emilio> RESOLVED: Accept edits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Unsorted
Development

No branches or pull requests

7 participants