1359060 - [css-grid] fit-content unexpectedly reserves space for full clamp size in repeat()

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Snook]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

Created grid layout using the following CSS:

```
#container {
  display: grid;
  grid-template-columns: repeat(auto-fit, fit-content(400px));
}
```

and the following HTML:
```
<div id="container">
  <div>Text A</div>
  <div>Text B</div>
  <div>Text C</div>
  <div>Text D</div>
</div>
```


Actual results:

Once the container is sized below 1600px, the grid items start flowing into subsequent rows.


Expected results:

Since each container's content is well below the clamp size of 400px, I expected all grid items to remain on the same row until the container was sized below total actual size of all items. (In my case, it was approximately 172px wide.)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Reporter's testcase

Comment 2

[Mats Palmgren (inactive)]

According to the CSS Grid spec, grid-template-columns: repeat(auto-fit, fit-content(400px))
is actually invalid, because <fixed-size> doesn't include fit-content():
https://drafts.csswg.org/css-grid/#typedef-auto-repeat
"repeat( [ auto-fill | auto-fit ] , [ <line-names>? <fixed-size> ]+ <line-names>? )"
https://drafts.csswg.org/css-grid/#typedef-fixed-size
"<fixed-size> = <fixed-breadth> | 
                minmax( <fixed-breadth> , <track-breadth> ) | 
                minmax( <inflexible-breadth> , <fixed-breadth> )"

It looks like Chrome (correctly) rejects this declaration as invalid.

IIRC, we implemented fit-content() (in bug 1281320) before the spec had a clear definition of
what sizes were allowed in an auto-repeat track size, so currently we interpret 400px as
a "definite track max size" and use that to calculate how many tracks to create.
This is probably not a very useful behavior, so I'll just fix this by rejecting
these values.  But feel free to open a spec issue at https://github.com/w3c/csswg-drafts/issues/new
if you want to argue for a spec change (please put [css-grid] first in the title).

Comment 3

[Mats Palmgren (inactive)]

Actually, after re-reading bug 1281320 and its patch, I think this is just a case that
we missed to reject somehow.  My bad.  Anyway, it's a pretty simple fix...

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=410ad33ec71474ca7b8bdf9e75447ff96114fdf2

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8862799 [details] [diff] [review]
fix

Review of attachment 8862799 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

(Consider adding ", per spec" to the end of the commit message, in the spirit of bz's recent dev.platform post about commit message best-practices. ('The commit message should describe not just the "what" of the change but also the "why".')

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8efdd4a24dd
[css-grid] Disallow fit-content() in repeat(auto-fill/fit) track sizes (per the CSS Grid spec).  r=dholbert

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8efdd4a24dd