1381512 - CSS grid: max-height inconsistent with Chrome

Status: RESOLVED DUPLICATE of bug 1526567

(Core :: Layout, defect, P3)

Description

[ashley]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170717030207

Steps to reproduce:

1. Visit https://www.scirra.com/labs/bugs/grid-100p-2/
2. Observe whether middle cell (cell 2) scrolls


Actual results:

In Firefox, the middle cell scrolls. In Chrome, the middle cell expands vertically to fit its content. Firefox behaves like Chrome if you remove the 'max-height: 100%' style on #cell2.


Expected results:

I'm not sure which behavior is correct - it probably needs someone familiar with the spec to examine it. I am only filing this for Firefox since its CSS grid implementation appears to be newer. If Chrome is deemed to be incorrect instead, I will happily re-file the issue with them.

Comment 1

[Manuel Rego Casasnovas]

It seems there's a problem with how "1fr" rows are computed.

Check the following example:
https://codepen.io/mrego/pen/JyXwYJ

The 2nd row is 100px, but it should be bigger (like in Chrome).
"1fr" is "minmax(auto, 1fr)", so the row size should be big enough
to fit its contents.
Spec: https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-flex

Comment 2

[Jet Villegas (inactive)]

Mats: please have a look and let us know if we should take a fix here. Thx!

Comment 3

[Kumar Harsh]

+1. I'm also facing this issue.

Here is a reproducible test: https://codepen.io/khs/pen/pWWWxz

I was confused by why Firefox was not respecting the navbar height (set as 100%). Then I set the min-height and it started to magically work on Firefox. But then it's broken on Chrome.

Referring the to spec, I found that the sizing does depend upon min-height in a few cases: 

https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-auto
>    As a maximum, identical to max-content. As a minimum, represents the largest minimum size (as specified by min-width/min-height) of the grid items occupying the grid track. 

I had accidentally followed this spec, and got it working for firefox. Later, I realized that the spec was talking about 'auto' sizing, and not free-space based sizing. So, it seems like those rare cases that Firefox is the one with the bug.

Comment 4

[Kumar Harsh]

Hi :jet - does the wontfix label this mean Firefox's implementation is correct? Should an issue be filed with the Blink team?

Comment 5

[Daniel Holbert [:dholbert]]

Sorry, bugzilla's flags are a bit confusing.  Jet only tagged it as wontfix *for Firefox version 57* (currently on beta, shipping to release in a month) -- the flag doesn't mean anything beyond that.

Firefox 57 is only taking extremely-low-risk patches (or patches for extremely severe issues) at this point -- hence that wontfix status -- but if there's a change needed here [pending mats' thoughts], we can still take a fix on Nightly when a patch is ready.

Comment 6

[Mats Palmgren (inactive)]

(In reply to Manuel Rego Casasnovas from comment #1)
> "1fr" is "minmax(auto, 1fr)", so the row size should be big enough
> to fit its contents.
> Spec: https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-flex

Right, but:
https://drafts.csswg.org/css-grid/#algo-single-span-items
"For each track with an intrinsic track sizing function and
not a flexible sizing function"
so the 'auto' min-sizing function have no effect in this part
of the algo for the row since it has "a flexible sizing function".

The '1fr' max-sizing function comes in to play here:
https://drafts.csswg.org/css-grid/#algo-flex-tracks
In particular: "If the free space is a definite length:"
which it is since the grid container has 'height: 300px'
in the example in comment 1 and 'height: 600px' in comment 0.
In that algo, the "leftover space" is the containers content
size minus "the base sizes of the non-flexible grid tracks",
so the '1fr' does its job of growing the row to fill the container.

I believe we handle this according to spec and that Chrome doesn't.

Rego, could you elaborate on which part of the Grid spec you base
your comment 1 on please?

Comment 7

[Manuel Rego Casasnovas]

(In reply to Mats Palmgren (:mats) from comment #6)
> (In reply to Manuel Rego Casasnovas from comment #1)
> > "1fr" is "minmax(auto, 1fr)", so the row size should be big enough
> > to fit its contents.
> > Spec: https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-flex
> 
> Right, but:
> https://drafts.csswg.org/css-grid/#algo-single-span-items
> "For each track with an intrinsic track sizing function and
> not a flexible sizing function"
> so the 'auto' min-sizing function have no effect in this part
> of the algo for the row since it has "a flexible sizing function".

Ok, this change is a recent change on the spec:
https://github.com/w3c/csswg-drafts/commit/25e3f631310207ed83746530b4066b6278c3234f
So what I said 10 months ago is no longer true. :-)

I agree that it shouldn't apply in this case now.
Chromium hasn't been updated yet to follow the changes discussed in:
https://github.com/w3c/csswg-drafts/issues/2177

Anyway I'm not completely sure it was the intention of the CSSWG
to modify the behavior for non-spanning items on that issue.
So I'm adding a comment there to clarify it.

Comment 8

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #7)
> (In reply to Mats Palmgren (:mats) from comment #6)
> > (In reply to Manuel Rego Casasnovas from comment #1)
> > > "1fr" is "minmax(auto, 1fr)", so the row size should be big enough
> > > to fit its contents.
> > > Spec: https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-flex
> > 
> > Right, but:
> > https://drafts.csswg.org/css-grid/#algo-single-span-items
> > "For each track with an intrinsic track sizing function and
> > not a flexible sizing function"
> > so the 'auto' min-sizing function have no effect in this part
> > of the algo for the row since it has "a flexible sizing function".
> 
> Ok, this change is a recent change on the spec:
> https://github.com/w3c/csswg-drafts/commit/
> 25e3f631310207ed83746530b4066b6278c3234f
> So what I said 10 months ago is no longer true. :-)

Ok, so it seems we missed the new step that was added as part of that change.

So in https://drafts.csswg.org/css-grid/#algo-single-span-items
we ignore the "auto" minimum doesn't have any effect.
But in the new step https://drafts.csswg.org/css-grid/#algo-spanning-flex-items
this will be considered, and the size of the track
should adapt to its contents and we shouldn't have overflow.

Does it make any sense?

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit BugBot documentation.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Manuel Rego Casasnovas from comment #1)

It seems there's a problem with how "1fr" rows are computed.

Check the following example:
https://codepen.io/mrego/pen/JyXwYJ

The 2nd row is 100px, but it should be bigger (like in Chrome).

At this point it looks like our rendering matches Chrome (380px tall), so I think this became fixed at some point. Fix range:
First good revision: 826b59e57fe4274954088e7a9ed9bab092203e1c (2019-02-22)
Last bad revision: e92ff56d2be21676b447c6fbb87b4c4479539bc9 (2019-02-21)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e92ff56d2be21676b447c6fbb87b4c4479539bc9&tochange=826b59e57fe4274954088e7a9ed9bab092203e1c

--> Looks like this was fixed by bug 1526567.

(In reply to Kumar Harsh from comment #3)
+1. I'm also facing this issue.

Here is a reproducible test: https://codepen.io/khs/pen/pWWWxz

For this one, the behavior hasn't changed in Firefox since comment 3, as far as I can tell; and current Chrome seems to match us. So whatever the issue was with this codepen, it seems our behavior is interoperable at this point, I guess?