1990170 - grid rows are squished in Firefox with version 143's multi-pass track sizing behavior (for grids with a specified 'max-height' and whose items have effective 'min-height:0')

Status: RESOLVED FIXED

(Core :: Layout: Grid, defect)

Description

[Dario Pellegrini]

Attachment: Grid issues ver 143.0.1.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:143.0) Gecko/20100101 Firefox/143.0

Steps to reproduce:

I have a div element that has these props in order to display a grid
overflowY: 'auto',
padding: '1.5rem',
scrollbarWidth: 'none',
display: 'grid',
gridTemplateColumns: '1fr',
gap: '20px 26px',
'@media screen and (min-width: 1150px)': {
gridTemplateColumns: 'repeat(2, minmax(0, 1fr))',
},
'@media screen and (min-width: 1480px)': {
gridTemplateColumns: 'repeat(3, minmax(0, 1fr))',
},
'@media screen and (min-width: 1830px)': {
gridTemplateColumns: 'repeat(4, minmax(0, 1fr))',
},
The grid was displayed well

Actual results:

From when I got the automatic update to ver 143.0.1
The grid does not display correctly anymore it is all shrinked

Expected results:

The grid displays correctly as it happens in other browsers, see the attached picture where I compare the results from chrome and firefox

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Grid' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Robert Longson [:longsonr]]

Please attach a complete testcase to the bug using the Attach New File button. Also if this worked in Firefox before you could get a regression range using https://mozilla.github.io/mozregression/

Comment 3

[Dario Pellegrini]

Attachment: This is how it looks a simple grid in chrome & firefox with an example I will attach as index.html as follows

Comment 4

[Dario Pellegrini]

Attachment: index.html

This is the simple grid example that shows the problems I mentioned earlier

Comment 5

[Dario Pellegrini]

Attachment: FirefoxGridIssues.png

With MozRegression I found that the issue started with the version built on July 30th, the day before it was working great, see the above image.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

In Firefox 143, we enabled the new multi-pass grid sizing track algorithm in bug 1957244. If I turn off layout.css.grid-multi-pass-track-sizing.enabled (setting it to false) in about:config, it appears that the simple testcase in comment 4 renders as expected.

A wordaround for this bug is to remove overflow: hidden; in .grid-item, as it can disable the automatic minimum height of the grid item https://drafts.csswg.org/css-grid-2/#min-size-auto

Comment 7

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

Set release status flags based on info from the regressing bug 1957244

Comment 8

[Dario Pellegrini]

I have read the above suggestions, but the only thing that helped me was to go in the advanced settings of Firefox (through about:config) and change the layout.css.grid-multi-pass-track-sizing.enabled to false :(
Which I do not think is a best practice

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a more-reduced testcase (reduced from attachment 9514956 [details]).

I've simplified it a bit to have min-height:0 on the grid item (whereas the original testcase implicitly has min-height:0, from default min-height:auto behavior combined with overflow:hidden, on its grid items).

I think content is getting squished here because in one of the layout passes, we must be strictly applying the max-height as a constraint on the collective size of the grid rows, and we think they're squishable because their contents all have min-height:0. Apparently this doesn't happen in other browsers (though they match our behavior if I change the max-height to an explicit height).

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 where browsers agree on squishing the grid cell & item

Just for illustration / in case it's useful diagnostically... Here's a variant of the previous testcase, where I just swapped to use height instead of max-height to constrain the grid's height.

That change doesn't impact our rendering, but it makes Chrome go from not-squishing to squishing (i.e. they agree with us).

Comment 11

[Dianna Smith [:diannaS]]

:TYLin is there any plans to work on this for 144? Marking as fix-optional for now.

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1990170 - Don't clamp max-block-size for bSizeForResolvingRowSizes when re-resolving row sizes.

We still want to consider min-block-size. Otherwise the following tests will
fail.

• /css/css-grid/alignment/grid-gutters-016.html
• /css/css-grid/grid-definition/grid-auto-repeat-min-size-004.html

The WPTs in this patch are adapted from bug 1990170 comment 3 (testcase 2).

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

:TYLin is there any plans to work on this for 144? Marking as fix-optional for now.

Yes, uploaded a patch to fix this.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/63fc322b2400
https://hg.mozilla.org/integration/autoland/rev/0f19dcc1d124
Don't clamp max-block-size for bSizeForResolvingRowSizes when re-resolving row sizes. r=layout-reviewers,dholbert

Comment 15

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/0f19dcc1d124

Comment 16

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

The patch landed in nightly and beta is affected.
:TYLin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox144 to wontfix.

For more information, please visit BugBot documentation.

Comment 17

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: The grid rows may be collapsed if the grid container's max-block-size is small.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: It is a one line change that affects only grid containers with no block-size and specified max-block-size.
• String changes made/needed: N/A
• Is Android affected?: yes

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1990170 - Don't clamp max-block-size for bSizeForResolvingRowSizes when re-resolving row sizes.

We still want to consider min-block-size. Otherwise the following tests will
fail.

• /css/css-grid/alignment/grid-gutters-016.html
• /css/css-grid/grid-definition/grid-auto-repeat-min-size-004.html

The WPTs in this patch are adapted from bug 1990170 comment 3 (testcase 2).

Original Revision: https://phabricator.services.mozilla.com/D267003

Comment 19

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/2d672cef5d4f
https://hg.mozilla.org/releases/mozilla-beta/rev/dd087d5678dc

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/55213 for changes under testing/web-platform/tests

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot