1527539 - [css-grid] Include grid item's margins and grid container's padding in the grid container's overflow area

Status: RESOLVED FIXED

(Core :: Layout: Grid, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase #1

I haven't check the specs yet, but our rendering differs to Chrome
in both the grid and flexbox cases...

Comment 1

[Mats Palmgren (inactive)]

Daniel, can you take a look at the flexbox case?

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #1

(slightly clearer version)

Comment 3

[Mats Palmgren (inactive)]

Oh, nice... :(

https://drafts.csswg.org/css-overflow-3/#scrollable
"The UA may additionally include the margin areas of boxes for which it is the containing block.
The conditions under which such margin areas are included is undefined in this level.
ISSUE 6. This needs further testing and investigation; is therefore deferred in this draft."

Comment 4

[Mats Palmgren (inactive)]

I'm guessing that the rendering in Chrome makes sense to authors,
since it's compatible with block layout at least for this trivial
example. I guess we'll have to reverse-engineer what they are
doing if we want to be compatible... Sigh.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Slightly more complex test for grid/flex layout.
It looks like Chrome is using the margin-boxes of children when calculating the overflow.

Comment 6

[Mats Palmgren (inactive)]

Does anyone have an opinion about the spec issue (cited in comment 3)?
Otherwise, I'm leaning towards proposing to the CSSWG that we resolve
in favor of using margin-boxes.

Comment 7

[Daniel Holbert [:dholbert]]

I don't have a strong opinion.

Comment 8

[Mats Palmgren (inactive)]

Some background about this: we intentionally included margins in bug 665597
and then reverted that partially in bug 724352 and eventually backed it out
in bug 750293 due to regressions. The part that did survive lives in
nsBlockFrame::ComputeFinalSize in the form of 'blockEndEdgeOfChildren'
which we propagate to ConsiderBlockEndEdgeOfChildren for inclusion into
the overflow area. So it appears that we only include the block-end margin,
but maybe only in some cases? - I haven't looked at the details yet...
I'll try to file a spec issue to sort this out unless there is one already.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

bug 1487927 is about the block-end margin rendering difference on columns.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #3 (inline-axis version)

Comment 12

[Mats Palmgren (inactive)]

Filed spec issue: https://github.com/w3c/csswg-drafts/issues/3653

Comment 13

[fantasai]

Fwiw spec issue concluded that margins/padding of flex/grid layout should always be included in scrollable overflow. (It's what authors expect, and as long as we're not constrained by webcompat, we should make it work as expected.)

Comment 14

[fantasai]

Pushed testcases for Flex in https://github.com/web-platform-tests/wpt/pull/23445

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I morph this bug into adding grid item's margins and grid container's padding in the grid container's overflow area because there are some sample tests for grid in this bug. Flexbox is handling in bug 1697349.

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Relevant tests on WPT:
http://wpt.live/css/css-grid/grid-model/grid-overflow-padding-001.html
http://wpt.live/css/css-grid/grid-model/grid-overflow-padding-002.html
http://wpt.live/css/css-grid/grid-model/grid-areas-overflowing-grid-container-001.html
http://wpt.live/css/css-grid/grid-model/grid-areas-overflowing-grid-container-002.html
http://wpt.live/css/css-grid/grid-model/grid-areas-overflowing-grid-container-003.html
...

Per offline chat with Ian, blink has implemented this new behavior in LayoutNG Grid. It can be enabled via command flag --enable-blink-features=LayoutNGGrid or go to URL chrome://flags/#enable-container-queries to turn it on.

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1527539 Part 1 - Add helper functions to calculate the sum of grid tracks sizes (and gaps).

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1527539 Part 2 - Incorporate grid items' margins and grid container's paddings when computing grid container's overflow area.

This patch updated the grid container's scrollable overflow areas to
match the spec. This also fixed Bug 1532383 - include space in empty
explicit grid tracks in the scrollable overflow area.

Depends on D111994

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6de6fc3205c0
Part 1 - Add helper functions to calculate the sum of grid tracks sizes (and gaps). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3cac3147fcd5
Part 2 - Incorporate grid items' margins and grid container's paddings when computing grid container's overflow area. r=dholbert

Comment 21

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1527539) for WPT failures in /css/css-grid/alignment/grid-content-alignment-overflow-002.html. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=341723159&repo=autoland&lineNumber=2879

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=3cac3147fcd551b1e394647ecd724bb688a2a03e

Backout:
https://hg.mozilla.org/integration/autoland/rev/eb306ed24a7b50e3f20eec6eba7594dec0af6078

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05e001cd9117
Part 1 - Add helper functions to calculate the sum of grid tracks sizes (and gaps). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/adc96c7ed268
Part 2 - Incorporate grid items' margins and grid container's paddings when computing grid container's overflow area. r=dholbert

Comment 23

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

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

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/05e001cd9117
https://hg.mozilla.org/mozilla-central/rev/adc96c7ed268

Comment 25

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

Upstream PR merged by moz-wptsync-bot