1871412 - Grid doesn't follow the spec quite correctly in terms of stretching of grid items.

Status: RESOLVED FIXED

(Core :: Layout: Grid, defect)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1871412 - Fix scroll container checks for automatic minimum size and grid stretching checks. r=dholbert,tylin

The spec says scroll container, so overflow: visible and clip shouldn't
be different. Also Without this fix the other patch causes a failure in
grid-item-overflow-stretch-005.html.

As noted in the comments, we should probably not look at the computed
value but the box type instead, but that's probably worth fixing in a
separate bug.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1871412 - Make a nsIFrame method aware that that contain:paint clips children in both axes. r=#layout

This is a minor perf optimization that I noticed while going through
this code. No behavior change.

Depends on D197052

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e4346863660
Fix scroll container checks for automatic minimum size and grid stretching checks. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/fa6dd9a5ca90
Make a nsIFrame method aware that that contain:paint clips children in both axes. r=dholbert

Comment 4

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

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

Comment 5

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0e4346863660
https://hg.mozilla.org/mozilla-central/rev/fa6dd9a5ca90

Comment 6

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

Upstream PR merged by moz-wptsync-bot

Comment 7

[Daniel Holbert [:dholbert]]

Sorry for not noticing during review -- it looks like our changes go against a CSSWG resolution (which is still pending a spec update), as discussed in https://github.com/w3c/csswg-drafts/issues/7714#issuecomment-1879319762

We changed here to depend on scroll-container-ness but should instead be depending on the computed overflow (even if it doesn't create a scroll container). Or, if you feel strongly about scroll-container-ness, we need to push for a spec change.

We should probably backout (or revert the behavior in a followup that preserves some of the worth-keeping things here) -- emilio, would you mind doing that?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

How so? IsScrollableOverflow is checking the computed value, not scroll-container-ness, right?

What you're describing is what the original version of the path did, but I changed to use IsScrollableOverflow as per the resolution and offline discussion. I even left a comment in https://hg.mozilla.org/mozilla-central/rev/0e4346863660#l2.14 right?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I guess your comment was about https://github.com/w3c/csswg-drafts/issues/7714#issuecomment-1879407674

Comment 10

[Daniel Holbert [:dholbert]]

Indeed it was. Thanks for the clarification; I misinterpreted our <input> behavior (and read too much into the extended-commit-message's mention of "The spec says scroll container...") and misremembered what the patch here was doing.

I think we're good here after all. I'm going to file a followup on amending stretch-grid-item-button-overflow.html to avoid depending on unspecified behavior, though.