[go: up one dir, main page]

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] Don't expand flexible tracks under a min-content constraint #3683

Closed
Loirooriol opened this issue Feb 27, 2019 · 2 comments
Closed
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
Loirooriol commented Feb 27, 2019

Consider this code: https://jsfiddle.net/wjthokzf/

#wrapper {
  width: 0;
}
#grid {
  display: grid;
  float: left;
  grid-template-columns: minmax(0, 1fr);
  border: solid;
}
#item::before, #item::after {
  content: '';
  float: left;
  height: 50px;
  width: 50px;
}
<div id="wrapper">
  <div id="grid">
    <div id="item"></div>
  </div>
</div>

The grid is sized under a min-content constraint. The sizing algorithm reaches https://drafts.csswg.org/css-grid/#algo-flex-tracks with a base size and growth limit of 0 for the column.

The free space is indefinite, so the used flex fraction is the result of finding the size of an fr, called with the column and the max-content contribution of the item (100px) as the space to fill. The leftover space is 100px, the flex factor sum is 1, the hypothetical fr size is 100px, and this is what the algorithm returns. So the flex fraction is 100px. And the base size of the column is set to 100px.

However, neither Chromium, Firefox nor Edge do this. The column is 0px wide.

So I think that this "Expand Flexible Tracks" step should only happen if the available space is definite, or if the grid is being sized under a max-content constraint. But not when under a min-content constraint.

fantasai added a commit that referenced this issue Jun 6, 2019
@fantasai
Copy link
Collaborator
fantasai commented Jun 6, 2019

Fixed to match implementations. I wonder why we did this for Maximize Tracks but not Expand Flexible Tracks. Agenda+ to request review, esp from @atanassov and @tabatkins. r=Oriol

@fantasai fantasai added Agenda+ Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. Target Revision: Current Remove when done. labels Jun 6, 2019
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Don't expand flexible tracks under a min-content constraint, and agreed to the following:

  • RESOLVED: Accept proposal in https://github.com/w3c/csswg-drafts/issues/3683
The full IRC log of that discussion <dael> Topic: Don't expand flexible tracks under a min-content constraint
<dael> github: https://github.com//issues/3683
<dael> fantasai: Case where spec forgot to consider min content correctly. Impl do logical and don't expand track to take up space. hcanging spec to match impl and do thing you expect which is size to smaller end when under min-content constraint.
<dael> fantasai: If you can shrink something down without overflow then min-content constraint should be that amount and not bigger. spec violated concept, impl did correct. Trying to match them up
<dael> astearns: Any comment? I not you asked for TabAtkins or Rossen_ to comment
<dael> fantasai: I'd prefer to get their +
<dael> TabAtkins: I'll review shortly
<dael> astearns: Resolve or wait?
<dael> TabAtkins: I trust fantasai so resolve. If I find a mistake I'll say something
<dael> Rossen_: On the same page. Proposed doesn't seem crazy, just need to look at overall algo fit. I'm sure fantasai spent more cycles so I trust her
<dael> astearns: Other comments?
<dael> astearns: Objections to this change?
<dael> RESOLVED: Accept proposal in https://github.com//issues/3683
<dael> astearns: All three of these look like they need tests or need tests verified. Have ther ebeen any?
<dael> fantasai: None in wpt yet. I'll check with oriol and he might know more
<dael> astearns: TabAtkins as you review can you check in tests?
<dael> TabAtkins: Sounds good
<dael> astearns: Once we have tests anything to keep us from updating CR?
<dael> fantasai: Prob tests for other things. I think most that should be fixed is but there might be one or two not.
<dael> astearns: I suspect no DoC yet.
<dael> fantasai: Right. Bulk of work is that and changes section
<dael> astearns: Anything else on grid?
<dael> fantasai: I'm going to say no

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