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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-grid] Doubts regarding implied minimum size of images #1117

Closed
mrego opened this Issue Mar 22, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@mrego
Member

mrego commented Mar 22, 2017

Section: 6.6. Implied Minimum Size of Grid Items.

This comes from a bug reported to Firefox by @jensimmons, after taking a look I'm quite lost now and I'm not sure if I understand even the simpler cases. 馃槥

So let's start with some examples and try to explain what's going on.
You can check the examples live in a jsbin.

1) 50x50 image with width: 200px:

<div style="display: grid;">
  <img style="width: 200px;" src="http://placehold.it/50x50">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

My doubt here comes about how the size of the 1st row is calculated. In both Chrome and Firefox it's 200px.

Output in Chrome and Firefox with the first row of 200px

The rows are auto and the image has min-height: auto;.
The image has a specified width of 200px, and as it has aspect ratio its transferred size is 200px too. But its content size is 50px.
According to the spec:

if the box has an aspect ratio and no specified size, its automatic minimum size is the smaller of its content size and its transferred size

So I'm not sure if the height of the row should actually be 50px instead of 200px.
I should be missing something else, but I don't know why right now.

2) 50x50 image with width: 200px and grid container with height: 10px:

<div style="display: grid; height: 10px;">
  <img style="width: 200px;" src="http://placehold.it/50x50">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

In this case both Chrome and Firefox calculate the size of the 1st row as 50px.

Output of Chrome and Firefox showing a first row of 50px

Is that right, why is this different from 1)?

3) 50x50 image with width: 100% and grid container with width: 200px:

<div style="display: grid; width: 200px;">
  <img style="width: 100%;" src="http://placehold.it/50x50">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

I'd expect this to be equivalent to 1). As the 100% width on the image should be resolved to 200px.
However in Firefox it's like 2), the row has 50px (and in Chrome it seems there's a bug as the size of the first row is 100px, which doesn't make a lot of sense.

According to the spec, the content size is 50px and the transferred size is 200px, so the minimum is 50px. So it seems Firefox is right on this one.

Problem here is that the 2nd row will be overlapping the image which doesn't seem what the user will want.
@jensimmons commented on the bug report that this won't be what authors want.

4) 50x50 image with width: 100% and grid container with width: 200px and height: 10px:

<div style="display: grid; width: 200px; height: 10px;">
  <img style="width: 100%;" src="http://placehold.it/50x50">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

I'd expect this to be equivalent to 2). Again the 100% width would be resolved to 200px.

But here the results vary. In Chrome the first row has 50px, but in Firefox it has only 10px.

5) 500x500 image with width: 200px:

<div style="display: grid; ">
  <img style="width: 200px;" src="http://placehold.it/500x500">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

Here both browsers behave the same and have a height of 200px for the 1st row.

This makes sense according to the spec as the transferred size is 200px and the content size is 500px. So the minimum is 200px.

6) 500x500 image with width: 200px and grid container with height: 10px:

<div style="display: grid; height: 10px;">
  <img style="width: 200px;" src="http://placehold.it/500x500">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

Again both browsers have the same behavior and the result is the same than 5).

Again the transferred size 200px is smaller than the content size 500px.

7) 500x500 image with width: 100% and grid container with width: 200px:

<div style="display: grid; width: 200px;">
  <img style="width: 100%;" src="http://placehold.it/500x500">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

Here in Chrome the size of the 1st row is 200px.
In Firefox it's 500px, as it consider that the 100% width is indefinite, so it doesn't have transferred size.
Anyway it seems @MatsPalmgren agrees that this should be considered definite in the bug report. Which will lead to the same output than Chrome.

8) 500x500 image with width: 100% and grid container with width: 200px and height: 10px:

<div style="display: grid; width: 200px; height: 10px;">
  <img style="width: 100%;" src="http://placehold.it/500x500">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

Here Chrome has a size of 200px for the row. However Firefox has 10px.

I believe that 200px for the 1st row should be the expected behavior.

Could someone help to clarify this topic? I'm almost certain I'm missing something else from the specs, at least to explain case 1) but I cannot find it. 馃槙
Thank you very much.

@mrego

This comment has been minimized.

Show comment
Hide comment
@mrego

mrego Mar 22, 2017

Member

After thinking on this for a while. I think now I understand the difference between 1) and 2).

The difference is that in the track sizing algorithm in both cases the 1st row has:

  • Base size: 50px
  • Growth limit: 200px

As in the case 1) there's no constraint on the height of the container, the row grows up to 200px.
In case 2) the height of the container is 10px, as there's no more space the row cannot grow and stops at 50px.

Member

mrego commented Mar 22, 2017

After thinking on this for a while. I think now I understand the difference between 1) and 2).

The difference is that in the track sizing algorithm in both cases the 1st row has:

  • Base size: 50px
  • Growth limit: 200px

As in the case 1) there's no constraint on the height of the container, the row grows up to 200px.
In case 2) the height of the container is 10px, as there's no more space the row cannot grow and stops at 50px.

@mrego

This comment has been minimized.

Show comment
Hide comment
@mrego

mrego Mar 22, 2017

Member

I believe than 3) and 4) should behave the same than 1) and 2) respectively. The height of the 1st row should be 200px for 3) and 50px for 4).
And that will actually make sense from the web authors point of view.

And then for 5), 6), 7) and 8) I believe the height of the row should be 200px, matching Chrome behavior.

So summarizing IMHO there are bugs in both engines:

  • Chrome: case 3)
  • Firefox: cases 3), 4), 7) and 8).

Sorry for all the noise on this... I was getting very confused before. 馃槍

Member

mrego commented Mar 22, 2017

I believe than 3) and 4) should behave the same than 1) and 2) respectively. The height of the 1st row should be 200px for 3) and 50px for 4).
And that will actually make sense from the web authors point of view.

And then for 5), 6), 7) and 8) I believe the height of the row should be 200px, matching Chrome behavior.

So summarizing IMHO there are bugs in both engines:

  • Chrome: case 3)
  • Firefox: cases 3), 4), 7) and 8).

Sorry for all the noise on this... I was getting very confused before. 馃槍

@mrego mrego closed this Mar 22, 2017

@mrego

This comment has been minimized.

Show comment
Hide comment
@mrego

mrego Mar 22, 2017

Member

Ok, so more thoughts on this... after some discussions with @svillar. Thanks!

Coming back to case 3). I believe now it's not a Chrome bug either. Let me explain why.
The example was:

<div style="display: grid; width: 200px;">
  <img style="width: 100%;" src="http://placehold.it/50x50">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

The problem is that the item in the 2nd row was adding noise to the example. And I was missing it.
So let's start for a case without the 2nd item.

A)

<div style="display: grid; width: 200px;">
  <img style="width: 100%;" src="http://placehold.it/50x50">
</div>

Here to resolve the size of the auto column, we check our children.
auto is somehow equivalent to minmax(auto, max-content).
The image has width: 100%; which cannot be resolved as the column has auto size, so for the minimum track size we consider 0px. And we use 50px, the content size of the image as maximum track size. And for the row, we use 50px too (the content size of the image).
Then the column size is affected by the default justify-content: stretch; which makes the auto column to grow up to 200px. The image is sized against that size of the column, so it ends up being 200x200. However the row is still 50px.

If we use justify-column: start; then the column and the row are 50px and the image is 50x50 too (B).

Now the example with the item. Imagine that it has exactly 100px width:
C)

<div style="display: grid; width: 200px;">
  <img style="width: 100%;" src="http://placehold.it/50x50">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

Here, to calculate the size of the auto column, we take into account the 100px width of the item in the 2nd row. Which are bigger than the content size of the image 50px. So the column is initially 100px. For the row the minimum size is 50px, but we can grow up to 100px and as there's room enough, it's also 100px.
Then again the justify-content: stretch; makes it grow the column to 200px. And the size of the image is 200x200. While the row is still 100px.

Again if we use justify-column: start the column and the row are 100px and the image is 100x100 (D).

So we believe Chrome behavior is the expected one. Does it make sense?

Next you can see an image with the results of the cases explained here.
Output in Chrome of the examples explained in this comment

Member

mrego commented Mar 22, 2017

Ok, so more thoughts on this... after some discussions with @svillar. Thanks!

Coming back to case 3). I believe now it's not a Chrome bug either. Let me explain why.
The example was:

<div style="display: grid; width: 200px;">
  <img style="width: 100%;" src="http://placehold.it/50x50">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

The problem is that the item in the 2nd row was adding noise to the example. And I was missing it.
So let's start for a case without the 2nd item.

A)

<div style="display: grid; width: 200px;">
  <img style="width: 100%;" src="http://placehold.it/50x50">
</div>

Here to resolve the size of the auto column, we check our children.
auto is somehow equivalent to minmax(auto, max-content).
The image has width: 100%; which cannot be resolved as the column has auto size, so for the minimum track size we consider 0px. And we use 50px, the content size of the image as maximum track size. And for the row, we use 50px too (the content size of the image).
Then the column size is affected by the default justify-content: stretch; which makes the auto column to grow up to 200px. The image is sized against that size of the column, so it ends up being 200x200. However the row is still 50px.

If we use justify-column: start; then the column and the row are 50px and the image is 50x50 too (B).

Now the example with the item. Imagine that it has exactly 100px width:
C)

<div style="display: grid; width: 200px;">
  <img style="width: 100%;" src="http://placehold.it/50x50">
  <div style="background: yellow;">Item (2nd row)</div>
</div>

Here, to calculate the size of the auto column, we take into account the 100px width of the item in the 2nd row. Which are bigger than the content size of the image 50px. So the column is initially 100px. For the row the minimum size is 50px, but we can grow up to 100px and as there's room enough, it's also 100px.
Then again the justify-content: stretch; makes it grow the column to 200px. And the size of the image is 200x200. While the row is still 100px.

Again if we use justify-column: start the column and the row are 100px and the image is 100x100 (D).

So we believe Chrome behavior is the expected one. Does it make sense?

Next you can see an image with the results of the cases explained here.
Output in Chrome of the examples explained in this comment

@mrego mrego reopened this Mar 22, 2017

mrego added a commit to mrego/csswg-test that referenced this issue Mar 23, 2017

[css-grid] Add test to verify the minimum size of images
This is a test created from issue w3c/csswg-drafts#1117.
The test checks how the autmoatic minimum size of images affect
to the grid container sizing and the track sizes.
The test also verifies the size of the image in the different cases.

The test combines the following cases:
* A grid container with fixed width (smaller and bigger than the image).
* A constrained grid container.
* A grid container without default "justify-content: stretch".
* A grid container with an extra test item.
* An image with a fixed width (smaller and bigger than the image original size).
* An image with a percentage width.
@fantasai

This comment has been minimized.

Show comment
Hide comment
@fantasai

fantasai Mar 23, 2017

Contributor

Short version: percentages against indefinite sizes are nasty. :(

Wrt #1117 (comment) comment A
Your logic here seems sound, but it seems like we might want to consider stretch somehow more intelligently when calculating the row height? Although I'm not sure how to do that given the algorithm cycles between axes.

Wrt comment B, I'm confused how we got to a row height of 100px.

Contributor

fantasai commented Mar 23, 2017

Short version: percentages against indefinite sizes are nasty. :(

Wrt #1117 (comment) comment A
Your logic here seems sound, but it seems like we might want to consider stretch somehow more intelligently when calculating the row height? Although I'm not sure how to do that given the algorithm cycles between axes.

Wrt comment B, I'm confused how we got to a row height of 100px.

@svillar

This comment has been minimized.

Show comment
Hide comment
@svillar

svillar Mar 24, 2017

Wrt comment B, I'm confused how we got to a row height of 100px.

I guess you mean C because in B the cell is 50x50. As rego explains the item in the second row (the one with text) forces the column to be 100px wide (so the image on the first row). Then when computing rows we end up with a first row of 100px (because the image respects the aspect ratio).

After running the track sizing algorithm we proceed to compute the alignment. The default value for justify-content is stretch, so the column becames 200px wide (the size of the grid container), but the rows are unaffected (we don't run the track sizing algorithm again).

After doing, once the tracks sizes are set, we proceed to layout/reflow the grid items. The image becomes 200px wide (as it has width:100%) and so the height (to respect the aspect ratio). That indeed causes an overflow, but that's the expected behavior in this case.

svillar commented Mar 24, 2017

Wrt comment B, I'm confused how we got to a row height of 100px.

I guess you mean C because in B the cell is 50x50. As rego explains the item in the second row (the one with text) forces the column to be 100px wide (so the image on the first row). Then when computing rows we end up with a first row of 100px (because the image respects the aspect ratio).

After running the track sizing algorithm we proceed to compute the alignment. The default value for justify-content is stretch, so the column becames 200px wide (the size of the grid container), but the rows are unaffected (we don't run the track sizing algorithm again).

After doing, once the tracks sizes are set, we proceed to layout/reflow the grid items. The image becomes 200px wide (as it has width:100%) and so the height (to respect the aspect ratio). That indeed causes an overflow, but that's the expected behavior in this case.

@fantasai fantasai added the css-grid-1 label Mar 30, 2017

@tabatkins

This comment has been minimized.

Show comment
Hide comment
@tabatkins

tabatkins Mar 30, 2017

Member

I'm a bit confused here: looking at the JSBin from the first comment, Chrome's behavior is identical for 1, 2, and 5-8 (the row is 200px tall). For 3 and 4, the row is 50px tall, and the image is squished to 200x50.

Member

tabatkins commented Mar 30, 2017

I'm a bit confused here: looking at the JSBin from the first comment, Chrome's behavior is identical for 1, 2, and 5-8 (the row is 200px tall). For 3 and 4, the row is 50px tall, and the image is squished to 200x50.

@tabatkins

This comment has been minimized.

Show comment
Hide comment
@tabatkins

tabatkins Mar 30, 2017

Member

Never mind, my Chrome was out of date. Now that I've let it update, it matches what Manuel described.

Member

tabatkins commented Mar 30, 2017

Never mind, my Chrome was out of date. Now that I've let it update, it matches what Manuel described.

@fantasai

This comment has been minimized.

Show comment
Hide comment
@fantasai

fantasai Mar 30, 2017

Contributor

After some thought, @tabatkins and I think we found what's wrong in the spec, and how to fix it.

With respect to examples 1/2/5/6:

Grid is using the same "automatic minimum size" algo that Flexbox does, which sets the min to the smaller of the content size and the specified/transferred size. This is correct for Flexbox - it gives us a reasonable minimum size that the image won't shrink below. But it serves a different purpose in Grid, effectively setting the layout size of the track while the item itself sizes into that containing block, potentially overflowing it. (Or, in other words, Flexbox actually sizes the item with the information; Grid sizes the track, and then the item lays itself out, and we don't want the item to overflow its track by default.)

Therefore, in Grid it doesn't make as much sense to pay attention to the content size when a specified/transferred size exists. So, we think the right fix is to just change Grid's automatic minimum size to be the specified size if it exists, else the transferred size if it exists, else the content size.

This will make examples 1, 2, 5, and 6 all render basically the same way, with a 200px tall first row, and the image filling that row. (Examples 3, 4, 7, and 8 rely on a different issue, so we won't worry about them here right now.) This gives more sensible results in size-constrained grids when the item's transferred size is larger than its content size, by keeping the contents of the track in sync and avoiding overlap of adjacent grid items.

Note: A similar consideration about overlapping overflow was used in resolving issue 283.

Refiling this particular issue as #1149

The second problem is in examples 3/4/7/8:

@mrego writes:

<div style="display: grid; width: 200px;">
 <img style="width: 100%;" src="http://placehold.it/50x50">
</div>

Here to resolve the size of the auto column, we check our children.
auto is somehow equivalent to minmax(auto, max-content).
The image has width: 100%; which cannot be resolved as the column has auto size, so for the minimum track size we consider 0px. And we use 50px, the content size of the image as maximum track size. And for the row, we use 50px too (the content size of the image).
Then the column size is affected by the default justify-content: stretch; which makes the auto column to grow up to 200px. The image is sized against that size of the column, so it ends up being 200x200. However the row is still 50px.

The problem here is that the row height of 50px is entirely unexpected. The author would expect this to behave similar to fr sizing: the content and the row height are calculated based on the stretched track size, not the initial track size.

The responsible spec prose is in https://drafts.csswg.org/css-grid/#algo-overview where stretch is processed at the end of the grid sizing algorithm. It should instead be integrated as a final step of track sizing, after fr sizing, in order to be looped into later track-sizing passes.

Refiling this issue as #1150

Please let us know if this seems to address all of the doubts or if we missed something. :)

Contributor

fantasai commented Mar 30, 2017

After some thought, @tabatkins and I think we found what's wrong in the spec, and how to fix it.

With respect to examples 1/2/5/6:

Grid is using the same "automatic minimum size" algo that Flexbox does, which sets the min to the smaller of the content size and the specified/transferred size. This is correct for Flexbox - it gives us a reasonable minimum size that the image won't shrink below. But it serves a different purpose in Grid, effectively setting the layout size of the track while the item itself sizes into that containing block, potentially overflowing it. (Or, in other words, Flexbox actually sizes the item with the information; Grid sizes the track, and then the item lays itself out, and we don't want the item to overflow its track by default.)

Therefore, in Grid it doesn't make as much sense to pay attention to the content size when a specified/transferred size exists. So, we think the right fix is to just change Grid's automatic minimum size to be the specified size if it exists, else the transferred size if it exists, else the content size.

This will make examples 1, 2, 5, and 6 all render basically the same way, with a 200px tall first row, and the image filling that row. (Examples 3, 4, 7, and 8 rely on a different issue, so we won't worry about them here right now.) This gives more sensible results in size-constrained grids when the item's transferred size is larger than its content size, by keeping the contents of the track in sync and avoiding overlap of adjacent grid items.

Note: A similar consideration about overlapping overflow was used in resolving issue 283.

Refiling this particular issue as #1149

The second problem is in examples 3/4/7/8:

@mrego writes:

<div style="display: grid; width: 200px;">
 <img style="width: 100%;" src="http://placehold.it/50x50">
</div>

Here to resolve the size of the auto column, we check our children.
auto is somehow equivalent to minmax(auto, max-content).
The image has width: 100%; which cannot be resolved as the column has auto size, so for the minimum track size we consider 0px. And we use 50px, the content size of the image as maximum track size. And for the row, we use 50px too (the content size of the image).
Then the column size is affected by the default justify-content: stretch; which makes the auto column to grow up to 200px. The image is sized against that size of the column, so it ends up being 200x200. However the row is still 50px.

The problem here is that the row height of 50px is entirely unexpected. The author would expect this to behave similar to fr sizing: the content and the row height are calculated based on the stretched track size, not the initial track size.

The responsible spec prose is in https://drafts.csswg.org/css-grid/#algo-overview where stretch is processed at the end of the grid sizing algorithm. It should instead be integrated as a final step of track sizing, after fr sizing, in order to be looped into later track-sizing passes.

Refiling this issue as #1150

Please let us know if this seems to address all of the doubts or if we missed something. :)

@fantasai fantasai closed this Mar 30, 2017

gsnedders added a commit to web-platform-tests/wpt that referenced this issue Mar 31, 2017

[css-grid] Add test to verify the minimum size of images
This is a test created from issue w3c/csswg-drafts#1117.
The test checks how the autmoatic minimum size of images affect
to the grid container sizing and the track sizes.
The test also verifies the size of the image in the different cases.

The test combines the following cases:
* A grid container with fixed width (smaller and bigger than the image).
* A constrained grid container.
* A grid container without default "justify-content: stretch".
* A grid container with an extra test item.
* An image with a fixed width (smaller and bigger than the image original size).
* An image with a percentage width.
@mrego

This comment has been minimized.

Show comment
Hide comment
@mrego

mrego Apr 3, 2017

Member

Yeah it sounds good to split this into 2 different issues. Thanks for taking a look!

Member

mrego commented Apr 3, 2017

Yeah it sounds good to split this into 2 different issues. Thanks for taking a look!

mrego added a commit to mrego/wpt that referenced this issue Apr 4, 2017

[css-grid] Add test to verify the minimum size of images
This is a test created from issue w3c/csswg-drafts#1117.
The test checks how the automatic minimum size of images affect
to the grid container sizing and the track sizes.
The test also verifies the size of the image in the different cases.

The test combines the following cases:
* A grid container with fixed width (smaller and bigger than the image).
* A constrained grid container.
* A grid container without default "justify-content: stretch".
* A grid container with an extra test item.
* An image with a fixed width (smaller and bigger than the image original size).
* An image with a percentage width.

mrego added a commit to mrego/wpt that referenced this issue May 24, 2017

[css-grid] Add test to verify the minimum size of images
This is a test created from issue w3c/csswg-drafts#1117.
The test checks how the automatic minimum size of images affect
to the grid container sizing and the track sizes.
The test also verifies the size of the image in the different cases.

The test combines the following cases:
* A grid container with fixed width (smaller and bigger than the image).
* A constrained grid container.
* A grid container without default "justify-content: stretch".
* A grid container with an extra test item.
* An image with a fixed width (smaller and bigger than the image original size).
* An image with a percentage width.

mrego added a commit to mrego/wpt that referenced this issue May 25, 2017

[css-grid] Add test to verify the minimum size of images
This is a test created from issue w3c/csswg-drafts#1117.
The test checks how the automatic minimum size of images affect
to the grid container sizing and the track sizes.
The test also verifies the size of the image in the different cases.

The test combines the following cases:
* A grid container with fixed width (smaller and bigger than the image).
* A constrained grid container.
* A grid container without default "justify-content: stretch".
* A grid container with an extra test item.
* An image with a fixed width (smaller and bigger than the image original size).
* An image with a percentage width.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment