1458902 - [css-grid] Resolve percentage grid-row-gaps against the sum of row sizes (in auto block-size containers)

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Mats Palmgren (inactive)]

Apparently, this was the resolution in https://github.com/w3c/csswg-drafts/issues/1921 as Rego points out in https://github.com/w3c/csswg-drafts/issues/509#issuecomment-386296399

Comment 1

[Mats Palmgren (inactive)]

Attachment: [css-grid] Re-resolve row-gap against the sum of row track sizes for auto-sized grid containers.

Comment 2

[Mats Palmgren (inactive)]

Attachment: [css-grid] Update a few WPT / reftests to new percentage row-gap layout.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c5e27537ed8c3f59b8665e4ad510aed1f9feebf

Comment 3

[Manuel Rego Casasnovas]

Comment on attachment 8973060 [details] [diff] [review]
[css-grid] Update a few WPT / reftests to new percentage row-gap layout.

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

::: testing/web-platform/tests/css/css-grid/reference/grid-collapsed-row-gutters-ref.html
@@ +37,5 @@
>          left: 110px;
>      }
>  </style>
>  
> +<p>The test passes if it has the same visual effect as reference. Column gap should be percentage of width. Row gap should be percentage of height.</p>

Nit: I think it'd be nice to rename this reference file, as it's no longer using "collapsed row gutters".

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8973059 [details] [diff] [review]
[css-grid] Re-resolve row-gap against the sum of row track sizes for auto-sized grid containers.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +5977,5 @@
>    if (!prevInFlow) {
> +    if (computedBSize == NS_AUTOHEIGHT) {
> +      // Re-resolve the row-gap now that we know our intrinsic block-size.
> +      gridReflowInput.mRows.mGridGap =
> +        nsLayoutUtils::ResolveGapToLength(stylePos->mRowGap, bSize);

Maybe we should add...
 && stylePos->mRowGap.HasPercent()
...to the condition here?  If there's no percentage, it seems like there's no reason to recalculate/modify this value, because it won't be changing.

Notably (regardless of this tweak), we *will* be modfiying the gap size if we have e.g. row-gap: calc(50px + 10%) -- I think the 50px there would already have influenced |bSize| via this line further up:
    bSize += gridReflowInput.mRows.SumOfGridGaps();
...which will then feed back into the size of the gap via this new line, which seems a little weird, but is probably fine. Can you be sure we've got a testcase that exercises that scenario, and that we're interoperable with Chrome on that? (I don't see one with calc(px + %) in the next patch right now.)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #4)
> Can you be sure we've got
> a testcase that exercises that scenario, and that we're interoperable with
> Chrome on that? (I don't see one with calc(px + %) in the next patch right
> now.)

(I guess the next patch is largely reference cases, so it's entirely possible one of the corresponding testcases does already exercise this scenario.)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8973060 [details] [diff] [review]
[css-grid] Update a few WPT / reftests to new percentage row-gap layout.

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

r=me , though the reference file's <title> needs updating. Right now it is:
> <title>CSS Grid Layout Reference: a square with a green bar</title>

...and that's no longer accurate (it doesn't have a "green bar" now)

Maybe change to "four gray squares separated by gaps, on top of a green rectangle", or something like that?

And maybe add a brief HTML comment after the <title> saying that the green rectangle is not-quite-square because we collapse the row-gap when resolving the block size (height) of the grid?  That would help clarify the rendering, and it'd justify the continued usage of "collapse" in the filename, making that naming less confusing / addressing Rego's concern. (Because we're collapsing *less* now, but we are still collapsing the gap for one key part of the algorithm, and that collapsing does affect the rendering by making the green area not-quite-square & making the gray blocks overflow off the bottom of the green area.)

Comment 7

[Daniel Holbert [:dholbert]]

Sorry, that last comment was in reference to the title of
 testing/web-platform/tests/css/css-grid/reference/grid-collapsed-row-gutters-ref.html

(I couldn't leave it as an inline comment on the <title> itself because the <title> isn't in the contextual code for the changes.)

Comment 8

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #4)
> Maybe we should add...
>  && stylePos->mRowGap.HasPercent()
> ...to the condition here?  If there's no percentage, it seems like there's
> no reason to recalculate/modify this value, because it won't be changing.

I don't think it matters much for performance since ResolveGapToLength/
ResolveToLength is inlined and the common case is "return nscoord(0)".
But sure, it makes the code clearer as to why we're re-evaluating it
I guess.

> Can you be sure we've got
> a testcase that exercises that scenario, and that we're interoperable with
> Chrome on that? (I don't see one with calc(px + %) in the next patch right
> now.)

We have reftests with calc() percentages.  I added some to the WPT tests
as well.  Chrome fails those tests -- I guess they haven't implemented
the resolution in https://github.com/w3c/csswg-drafts/issues/1921
yet, but Rego filed it and is CC:ed here so I expect him to take care
of it... :-)

I renamed the -ref file grid-percentage-gap-ref.html.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be1b4c042170
[css-grid] Re-resolve row-gap against the sum of row track sizes for auto-sized grid containers.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/3239e0d823ff
[css-grid] Update a few WPT / reftests to new percentage row-gap layout.  r=dholbert

Comment 10

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

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

Comment 11

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/be1b4c042170
https://hg.mozilla.org/mozilla-central/rev/3239e0d823ff

Comment 13

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

Upstream PR merged