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] "Maximize Tracks" shouldn't distribute equally for flexible tracks #3693

Closed
Loirooriol opened this issue Mar 1, 2019 · 7 comments
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1 Tested Memory aid - issue has WPT tests

Comments

@Loirooriol
Copy link
Contributor

From https://drafts.csswg.org/css-grid/#algo-grow-tracks,

If the free space is positive, distribute it equally to the base sizes of all tracks, freezing tracks as they reach their growth limits (and continuing to grow the unfrozen tracks as needed).

This step used not to be relevant for flexible tracks, because their growth limit is initially set to their base size.

However, since #2177, the growth limit can be increased in https://drafts.csswg.org/css-grid/#algo-spanning-flex-items

Increase sizes to accommodate spanning items crossing flexible tracks: Next, repeat the previous step instead considering [...] all items that do span a track with a flexible sizing function while

  • treating flexible tracks as having a max track sizing function equal to their min track sizing function
  • [...]
  • distributing space to such tracks according to the ratios of their flexible sizing functions rather than distributing space equally

Therefore, consider https://jsfiddle.net/3kbz9j2g/

<div id="grid">
  <div id="item"></div>
</div>
#grid {
  display: grid;
  width: 100px;
  grid-template-columns: 1fr 3fr;
  border: solid;
}
#item {
  grid-column: 1 / 3;
  min-width: 0;
  height: 100px;
  background: cyan;
}
#item::before {
  content: '';
  display: block;
  width: 200px;
}

The minimum contribution of the item is 0 so it doesn't increase the base sizes of the tracks. But both tracks are treated as if they had an auto max track sizing function, so the max-content contribution of the item (200px) is distributed into growth limits, 50px to the first column and 150px into the 2nd one.

So we reach https://drafts.csswg.org/css-grid/#algo-grow-tracks as follows:

  • Free space: 100px
  • 1st column: base size 0px, growth limit 50px.
  • 2nd column: base size 0px, growth limit 150px.

Then if the 100px are distributed equally:

  • Free space: 0px
  • 1st column: base size 50px, growth limit 50px.
  • 2nd column: base size 50px, growth limit 150px.

And https://drafts.csswg.org/css-grid/#algo-flex-tracks is no-op since the free space is 0.

So the column ratios end up being 1:1 instead of 1:3. This doesn't look good to me. I think that

  • either the "Maximize Tracks" should not distribute free space into flexible tracks, leaving this part to "Expand Flexible Tracks",
  • or it should distribute space according to the flex ratios instead of equally (considering non-flexible tracks to have a ratio of 0, and distribute equally if the sum of ratios is 0? Not sure).
@Loirooriol
Copy link
Contributor Author

Loirooriol commented Mar 2, 2019

A more complex example:

https://jsfiddle.net/2rsLcwg8/

<div id="log"></div>
<div id="grid">
  <div id="item1">
    <div id="item1-sub1">XXXX</div>
    <div id="item1-sub2">XXXX XX XX</div>
  </div>
  <div id="item2" class="secondRowSecondColumn">
    <div id="item2-sub1">XXX XX X</div>
    <div id="item2-sub2">XXXX XXX XXXX XXX XXXX</div>
  </div>
</div>
#grid {
  font: 10px/1 Ahem;
  display: grid;
  grid: 1fr 2fr / 2fr 1fr;
  width: 300px;
  height: 300px;
  background-color: grey;
}
#item1 {
  display: grid;
  grid: 1fr 3fr / 3fr 1fr;
  grid-area: 1 / 1;
  background-color: blue;
}
#item1-sub1 {
  grid-area: 1 / 1;
  background-color: orange;
}
#item1-sub2 {
  grid-area: 2 / 2;
  background-color: lime;
}
#item2 {
  display: grid;
  grid: 3fr 4fr / 4fr 3fr;
  grid-area: 2 / 2;
  background-color: magenta;
}
#item2-sub1 {
  grid-area: 1 / 1;
  background-color: yellow;
}
#item2-sub2 {
  grid-area: 2 / 2;
  background-color: cyan;
}
function tracks(id, prop) {
  var el = document.getElementById(id);
  var cols = getComputedStyle(el)[prop];
  var ratios = cols.split(" ").map(parseFloat);
  var min = Math.min(...ratios);
  ratios = ratios.map(n => Math.round(n / min * 100) / 100);
  return `${cols} (${ratios.join(":")})`;
}
var log = document.getElementById("log");
log.append(
  "Outer grid columns: ", tracks("grid", "gridTemplateColumns"),
  document.createElement("br"),
  "Outer grid rows: ", tracks("grid", "gridTemplateRows"),
  document.createElement("hr"),
  "Inner grid 1 columns: ", tracks("item1", "gridTemplateColumns"),
  document.createElement("br"),
  "Inner grid 1 rows: ", tracks("item1", "gridTemplateRows"),
  document.createElement("hr"),
  "Inner grid 2 columns: ", tracks("item2", "gridTemplateColumns"),
  document.createElement("br"),
  "Inner grid 2 rows: ", tracks("item2", "gridTemplateRows"),
  document.createElement("hr"),
);

Before #2177 the track size ratios corresponded to their flex factors (because the minimum contributions are small enough). With the current spec, ratios are totally screwed. Maximizing tracks using their flex factor ratios improves them just a little bit. Not maximizing flexible tracks provides the old behavior, I think that's the expected one.

Before #2177 Current spec (maximize equally)
previously maximize-equally
Don't maximize flexible tracks Maximize with ratios
maximize-none maximize-ratios

@Loirooriol
Copy link
Contributor Author

BTW, not maximizing flexible tracks should be equivalent to not increasing their growth limit, i.e. drop this:

treating flexible tracks as having a max track sizing function equal to their min track sizing function

What was this amendment trying to solve? I don't see this getting discussed in #2177.

fantasai added a commit that referenced this issue Jun 6, 2019
…owth limit equal to their base size in all cases; and therefore not grow during Maximize Tracks. #3693
@fantasai
Copy link
Collaborator

fantasai commented Jun 6, 2019

Flexible tracks shouldn't be affected by the "Maximize Tracks" section at all... I think what we should have done instead was calculate the base sizes and then just set the growth limits to the base sizes. Updated the spec to do that. r=Oriol Agenda+ to request WG review

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed "Maximize Tracks" shouldn't distribute equally for flexible tracks, and agreed to the following:

  • RESOLVED: Accept change in https://github.com/w3c/csswg-drafts/issues/3693
The full IRC log of that discussion <dael> Topic: "Maximize Tracks" shouldn't distribute equally for flexible tracks
<dael> github: https://github.com//issues/3693
<dael> fantasai: this was another fix for errors
<dael> fantasai: There was a statement where we made a mistake saying treat max sizing same as min sizing. Trying to select a class of tracks and didn't use the right words.
<dael> astearns: Okay
<dael> fantasai: Just fixing an error. Happy is people want to look at it
<dael> astearns: Given issue discussion looks correct. oriol said it looks good
<dael> fantasai: These were co-edited with oriol so he thinks they're correct
<dael> astearns: Comments on this change? Objections?
<dael> RESOLVED: Accept change in https://github.com//issues/3693

@Loirooriol
Copy link
Contributor Author

@fantasai Thinking about this again, "treating flexible tracks as having an infinite growth limit" can make it seem that this is just a temporal thing just for step §11.5.4, and after that the growth limit is set back to its old value. But that's not the case, since §11.5.5 assumes flexible tracks will still have an infinite growth limit. So instead of "treating", maybe it should say "Set the growth limit of flexible tracks to infinity"? But then why not set that at the beginning in https://drafts.csswg.org/css-grid/#algo-init ? I think it would be equivalent, I don't remember if we considered it.

@fantasai
Copy link
Collaborator

fantasai commented Nov 1, 2019

@Loirooriol I believe your last comment was handled in #4313 so marking the issue as closed. Please mark Commenter Satisfied if you are happy with the edits here. :)

@fantasai fantasai closed this as completed Nov 1, 2019
@Loirooriol Loirooriol added the Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. label Nov 1, 2019
@Loirooriol
Copy link
Contributor Author

Yes #4313 finished this, it seems good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1 Tested Memory aid - issue has WPT tests
Projects
None yet
Development

No branches or pull requests

3 participants