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

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: svillar, Assigned: mats)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla51
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Created attachment 8786340 [details]
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)
(Reporter)

Comment 1

2 years ago
Created attachment 8786341 [details]
Rendering comparison (Chromium left, Firefox right)

Updated

2 years ago
Blocks: 616605
Component: Untriaged → Layout
Product: Firefox → Core

Updated

2 years ago
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 2

2 years ago
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
Flags: needinfo?(svillar)
(Reporter)

Comment 3

2 years ago
(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?
Flags: needinfo?(svillar)
(Assignee)

Comment 4

2 years ago
> 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.
(Reporter)

Comment 5

2 years ago
(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)
(Assignee)

Comment 6

2 years ago
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.
Assignee: nobody → mats
Blocks: 1217086
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: testcase
(Assignee)

Comment 7

2 years ago
Created attachment 8789628 [details] [diff] [review]
fix

Take grid gaps into account as if they were tracks.
Attachment #8789628 - Flags: review?(dholbert)
(Reporter)

Comment 9

2 years ago
(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 on attachment 8789628 [details] [diff] [review]
fix

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

r=me
Attachment #8789628 - Flags: review?(dholbert) → review+
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

2 years ago
Pushed by mpalmgren@mozilla.com:
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.
(Assignee)

Comment 14

2 years ago
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)
Flags: needinfo?(mats) → needinfo?(wkocher)
Okay, looks like backfilling puts this on bug 1302225.
Flags: needinfo?(wkocher)
(Assignee)

Comment 16

2 years ago
OK, please re-land the changesets in comment 12.
Keywords: checkin-needed

Comment 17

2 years ago
Pushed by ryanvm@gmail.com:
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.
Keywords: checkin-needed

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/294732982075
https://hg.mozilla.org/mozilla-central/rev/132d071458b7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

2 years ago
Flags: in-testsuite+
(Assignee)

Updated

2 years ago
Blocks: 1302839
You need to log in before you can comment on or make changes to this bug.