Closed Bug 1302541 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- affected
firefox52 --- fixed

People

(Reporter: mats, Assigned: mats)

References

Details

(Keywords: testcase)

Attachments

(6 files)

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
Tentatively marking this as blocking release; at least until the spec issue
is sorted out.
Blocks: 1217086
Assignee: nobody → mats
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 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.
Attachment #8799916 - Flags: review?(dholbert) → review+
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 )
Attachment #8799917 - Flags: review?(dholbert) → review+
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.)
Attachment #8799918 - Flags: review?(dholbert) → review+
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.
Attachment #8799919 - Flags: review?(dholbert) → review+
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.
Attachment #8799920 - Flags: review?(dholbert) → review+
(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
(other nits addressed as suggested)
Pushed by mpalmgren@mozilla.com:
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.
Depends on: 1314664
You need to log in before you can comment on or make changes to this bug.