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

RESOLVED FIXED in Firefox 62

Status

()

P3
minor
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 1 bug)

Trunk
mozilla62
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox61 wontfix, firefox62 fixed)

Details

Attachments

(2 attachments)

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 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.)
Attachment #8973059 - Flags: review?(dholbert) → review+
(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 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.)
Attachment #8973060 - Flags: review?(dholbert) → review+
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.)
(Assignee)

Comment 8

10 months ago
(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

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

Updated

10 months ago
Flags: in-testsuite+
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/11079 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 12

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be1b4c042170
https://hg.mozilla.org/mozilla-central/rev/3239e0d823ff
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
status-firefox61: affected → wontfix
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.