1299133 - [css-grid] wrong fit-content() behavior with gaps and spanning items

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Sergio Villar]

Attachment: Test case

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/602.1 (KHTML, like Gecko) Version/8.0 Safari/602.1 Debian/buildd-unstable (3.20.3-2) Epiphany/3.20.3

Steps to reproduce:

I'm attaching a test case which is rendered differently in Firefox and Chromium (with the patch enabling fit-content() https://codereview.chromium.org/2287113004/).

I think that the track sizing algorithm is not properly applied whenever we have items spanning through fit-content() tracks in the presence of gaps.


Actual results:

In the test case I'm attaching Firefox calculates the sizes of the two columns as 22.5px 52.5px. Chromium on the other hand spits a value of 5px 70px.


Expected results:

This is my interpretation of the track sizing algorithm behavior for columns for this test case:

0- Both items have min-content: 30px max-content: 110px
1- Initialize: (0,inf) (0,inf)
2- Resolve non spanning items: (0, inf) (30,80)
3- Resolve spanning items: (0, 5) (30, 80)
4- Maximize tracks: (5, 5) (70, 80)

Comment 1

[Sergio Villar]

Attachment: Rendering comparison (Chromium left, Firefox right)

Comment 2

[Mats Palmgren (inactive)]

As you know, fit-content(X) is short for minmax(auto, max-content) with
an additional clamping of the maximum size to X.  Unfortunately, that
means that fit-content() in Firefox is affected by bug 1255393.
The given testcase have min-width:auto so is affected by that bug.

However, if I add min-width:30px to the two items then I think
we should get a comparable testcase.  Here's how we calculate that
compared to your values:

> 0- Both items have min-content: 30px max-content: 110px

Agreed, and Firefox does the same.

> 1- Initialize: (0,inf) (0,inf)

Agreed, and Firefox does the same.

> 2- Resolve non spanning items: (0, inf) (30,80)

Agreed, and Firefox does the same.

> 3- Resolve spanning items: (0, 5) (30, 80)

No this is wrong, it should be: (0, 30) (30, 80).
The first 30 there comes from distributing the max-content size 110px.
Only 80px of that can go in track 2, so 30px (110-80) goes to track 1.

> 4- Maximize tracks: (5, 5) (70, 80)

The free space to distribute is 75px (100px - 25px grid-gap).
We substract the track min sizes (0+30) => 45px remaining free space.
Both tracks are still growable => we'll try to add 22.5px per track.
Adding 22.5px to track 1 is less than the limit 30px, now we have:
  (22.5, 30) (30, 80)
Adding 22.5px to track 2 is less than the limit 80px, now we have:
  (22.5, 30) (52.5, 80)
All space is distributed so we're done.


So, modulo bug 1255393, this seems like a bug in Chrome.
Perhaps you forgot the "limit the growth of any fit-content() tracks
by their fit-content() argument" in Step 2.6 "For max-content maximums" ?
Or the remaining 30px after clamping wasn't distributed properly to
the growth limit of track 1?
https://drafts.csswg.org/css-grid/#algo-content

Comment 3

[Sergio Villar]

(In reply to Mats Palmgren (:mats) from comment #2)

> > 3- Resolve spanning items: (0, 5) (30, 80)
> 
> No this is wrong, it should be: (0, 30) (30, 80).
> The first 30 there comes from distributing the max-content size 110px.
> Only 80px of that can go in track 2, so 30px (110-80) goes to track 1.

So I think you're forgetting about the existence of a 25px grid-column-gap. That's why we only could grow the 1st track to 5px (30px - 25px). Does it make sense for you?

Comment 4

[Mats Palmgren (inactive)]

> So I think you're forgetting about the existence of a 25px grid-column-gap.

No, the "Resolve spanning items" step is part of "12.5. Resolve Intrinsic Track Sizes"
https://drafts.csswg.org/css-grid/#algo-content
Grid gaps are NOT part of the algorithm in 12.5 at all, so taking them into
account there makes no sense.  12.5 is about distributing min/max contributions
*from the items* so that each track gets a min and max value assigned to it.

The grid gaps comes into play later, in 12.6/12.7, where they reduce the free space
there is to distribute.

Comment 5

[Sergio Villar]

(In reply to Mats Palmgren (:mats) from comment #4)
> > So I think you're forgetting about the existence of a 25px grid-column-gap.
> 
> No, the "Resolve spanning items" step is part of "12.5. Resolve Intrinsic
> Track Sizes"
> https://drafts.csswg.org/css-grid/#algo-content
> Grid gaps are NOT part of the algorithm in 12.5 at all, so taking them into
> account there makes no sense.  12.5 is about distributing min/max
> contributions
> *from the items* so that each track gets a min and max value assigned to it.

I fundamentally disagree with your interpretation. Grid gaps ARE part of the algorithm. On 11.1 it's clearly specified that "For the purpose of track sizing, each gutter is essentially treated as an extra, empty track of the specified size. Negative values are invalid."

This means that the example I attached should show the same output as if I were using 
grid-template-columns: fit-content(80px) 25px fit-content(80px) (adjusting the grid placement properties)

Comment 6

[Mats Palmgren (inactive)]

That's a good point; I agree this is a bug in Gecko.
After fixing it I get the same results, point for point, as you reported.

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix

Take grid gaps into account as if they were tracks.

Comment 8

[Mats Palmgren (inactive)]

Attachment: reftests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25a1de0673dc

Comment 9

[Sergio Villar]

(In reply to Mats Palmgren (:mats) from comment #6)
> That's a good point; I agree this is a bug in Gecko.
> After fixing it I get the same results, point for point, as you reported.

Awesome!

We all have bugs, the important thing is that this is another step forward in terms of interoperability between our implementations.

Comment 10

[Daniel Holbert [:dholbert]]

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

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

r=me

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8789629 [details] [diff] [review]
reftests

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

::: layout/reftests/css-grid/grid-track-fit-content-sizing-002-ref.html
@@ +5,5 @@
> +-->
> +<html><head>
> +  <meta charset="utf-8">
> +  <title>CSS Grid Test: fit-content() track sizing</title>
> +  <link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1281320">

Nit: wrong bug number in the URL in the reference case here. (It's correct in the testcase, but not in the reference case).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8898636baa30
[css-grid] Subtract the grid-gaps in the span when collecting growable tracks.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa010785b1ed
[css-grid] fit-content() with grid-gap reftests.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for reftest failures on Windows VM: https://treeherder.mozilla.org/logviewer.html#?job_id=35724265&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/89b72566e838

Comment 14

[Mats Palmgren (inactive)]

This bug is only changing nsGridContainerFrame.cpp which is only used for
display:grid/inline-grid boxes.  The failed tests don't use grid at all.
Are you sure it's this bug that broke those tests?

(the only thing I can think of is that adding a new grid reftest changed
the "reftest chunking" somehow and that uncovered an existing issue
with the masking reftests that failed, but that seems pretty unlikely)

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Okay, looks like backfilling puts this on bug 1302225.

Comment 16

[Mats Palmgren (inactive)]

OK, please re-land the changesets in comment 12.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/294732982075
[css-grid] Subtract the grid-gaps in the span when collecting growable tracks.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/132d071458b7
[css-grid] fit-content() with grid-gap reftests.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/294732982075
https://hg.mozilla.org/mozilla-central/rev/132d071458b7