Closed
Bug 1458902
Opened 6 years ago
Closed 6 years ago
[css-grid] Resolve percentage grid-row-gaps against the sum of row sizes (in auto block-size containers)
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla62
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
1.37 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
5.75 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c5e27537ed8c3f59b8665e4ad510aed1f9feebf
Attachment #8973060 -
Flags: review?(dholbert)
Comment 3•6 years ago
|
||
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•6 years ago
|
||
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+
Comment 5•6 years ago
|
||
(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•6 years ago
|
||
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+
Comment 7•6 years ago
|
||
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•6 years 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.
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•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/be1b4c042170 https://hg.mozilla.org/mozilla-central/rev/3239e0d823ff
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•