1302541 - [css-grid] Wrong intrinsic grid container size with percentage grid gaps

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

I think percentage grid gaps should contribute to the grid
container's intrinsic size just like percentages normally do,
i.e. using the equation: Size = SumOfCoordSizes / (1 - SumOfPercentages).

Here's a testcase:
https://people.mozilla.org/~mpalmgren/tests/grid/percent-intrinsic-sizing.html
(last two boxes are grids) and here's the suggested rendering:
https://people.mozilla.org/~mpalmgren/tests/grid/percent-intrinsic-sizing.png

The CSS Grid spec needs to be amended to allow that though:
https://github.com/w3c/csswg-drafts/issues/472

Comment 1

[Mats Palmgren (inactive)]

Tentatively marking this as blocking release; at least until the spec issue
is sorted out.

Comment 2

[Mats Palmgren (inactive)]

The CSSWG was kind enough to "leave open" how percentages work under
intrinsic sizing, explicitly mentioning "back-computing" is allowed,
with the requirement that grid-gap works the same as empty tracks:
https://github.com/w3c/csswg-drafts/issues/472#issuecomment-248655700

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Add a track state bit for percentage min-sizing that is treated as 'auto'.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Propagate track state bits and store the union of all tracks in each axis.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Back-compute percentages for the intrinsic inline size.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Back-compute percentages for the intrinsic block size.

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Back-compute percentages when calculating the number of auto-fill/fit tracks.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 6 - [css-grid] Add more reftests for percentage track sizes and grid-gaps.

Comment 9

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8648102e4fbe239040d69f426f056ff477ca2b5a&exclusion_profile=false

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8799918 [details] [diff] [review]
part 3 - [css-grid] Back-compute percentages for the intrinsic inline size.

> +      size += std::max(length, trackSize);

FYI, I believe this line could be simplified to "size += trackSize" if
we make a small change to how we initialize tracks with a min-sizing
calc() size.  When the percentage base is indefinite, we should set mBase
to the calc() length part instead of zero (but continue to treat the track
as "auto" as before).  I'll investigate and do this in a follow-up bug.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8799917 [details] [diff] [review]
part 2 - [css-grid] Propagate track state bits and store the union of all tracks in each axis.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +1811,5 @@
>    nscoord mGridGap;
>    // The first(last)-baseline for the first(last) track in this axis.
>    nscoord mBaseline[2]; // index by BaselineSharingGroup
> +  // The union of the track min/max-sizing state bits in this axis.
> +  TrackSize::StateBits mState;

Maybe name this mStateUnion*, so that it's harder to mix up this member-var (in Tracks) with the identically-named member-var in TrackSize?  (And also since this particular variable isn't meant to track a single "state" -- as its comment says, it's a union of multiple states.)

* (or mUnitedStates? :D )

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8799918 [details] [diff] [review]
part 3 - [css-grid] Back-compute percentages for the intrinsic inline size.

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

r=me, with the following minor concern addressed/explained:

::: layout/generic/nsGridContainerFrame.cpp
@@ +4873,5 @@
> +    }
> +    size += gridGapCount * gridGapLength;
> +  }
> +
> +  return std::max(0, nsLayoutUtils::AddPercents(size, percent));

Why the "std::max" here?  |size| should already be nonnegative (since it's the sum of nonnegative things), and AddPercents promises to return a strictly larger value than it was given.

(And I don't think we need to bother concerning ourselves with integer overflow, except to be sure it doesn't cause hangs/crashes.)

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8799919 [details] [diff] [review]
part 4 - [css-grid] Back-compute percentages for the intrinsic block size.

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

r=me, nits below:

::: layout/generic/nsGridContainerFrame.cpp
@@ +2003,5 @@
>    }
>  
>    /**
> +   * Calculate our track sizes.  If the given aContentBox block-axis size is
> +   * unconstrained it is assigned to the resulting intrinsic size.

Two nits:
 1) add comma after "unconstrained", for readability.
 2) s/intrinsic size/intrinsic block-axis size/, for clarity about what flavor of "intrinsic size" we're copying from here.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8799920 [details] [diff] [review]
part 5 - [css-grid] Back-compute percentages when calculating the number of auto-fill/fit tracks.

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

r=me, one nit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +1131,4 @@
>      // Note that the repeat() track size is included in |sum| in this loop.
>      nscoord sum = 0;
> +    float percentSum = 0.0f;
> +    nscoord percentBasis = aSize;

Can we declare |percentBasis| as "const" here?  Looks like it never gets modified.

Comment 15

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Why the "std::max" here?

It can be negative since we sum coord sizes separately from percentages, e.g.
grid: 0 0 / 0; grid-gap: calc(10% - 1px)
leads to |size| = -1px

Comment 16

[Mats Palmgren (inactive)]

(other nits addressed as suggested)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d7ffc24c84
part 1 - [css-grid] Add a track state bit for percentage min-sizing that is treated as 'auto'.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c761406e4c14
part 2 - [css-grid] Propagate track state bits and store the union of all tracks in each axis.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/948ea4b490cf
part 3 - [css-grid] Back-compute percentages for the intrinsic inline size.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0adfd4435d28
part 4 - [css-grid] Back-compute percentages for the intrinsic block size.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/50173aed1530
part 5 - [css-grid] Back-compute percentages when calculating the number of auto-fill/fit tracks.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/aff267eaa3b4
part 6 - [css-grid] Add more reftests for percentage track sizes and grid-gaps.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1d7ffc24c84
https://hg.mozilla.org/mozilla-central/rev/c761406e4c14
https://hg.mozilla.org/mozilla-central/rev/948ea4b490cf
https://hg.mozilla.org/mozilla-central/rev/0adfd4435d28
https://hg.mozilla.org/mozilla-central/rev/50173aed1530
https://hg.mozilla.org/mozilla-central/rev/aff267eaa3b4

Comment 19

[yisibl]

https://github.com/w3c/csswg-drafts/issues/509