1278058 - [css-grid] Block-axis margin on fragmented grid item renders incorrectly

Status: NEW

(Core :: Layout: Grid, defect, P3)

Description

[Mats Palmgren (inactive)]

Attachment: testcase

Steps to reproduce:
1. load the attached testcase

Actual results:
There is no gap (margin) between the blue top border and the enclosing border.
There is a gap at the top of the second column and the blue border overlaps
the enclosing border on the bottom side.

Expected results
A 10px gap between the blue top border and its enclosing border.
The blue and black side borders in the first column should extend
the same length.
There should be no gap at the top of the second column.
There should be a 10px gap between the blue bottom border and its
enclosing border in the second column.

Comment 1

[Brad Werth [:bradwerth]]

Attachment: gridFrag1.patch

A partial fix (not run through rigorous tests at time of submission) that makes sure sizes are fixed, but still work to do with margins. Please comment on whether use of if GetNextInFlow() and if isConstrainedBSize is appropriate.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=096e7c85d963

Comment 2

[Brad Werth [:bradwerth]]

Attachment: gridFrag2.patch

Adding stretch to fragmented grids seems to be the primary source of weirdness in this testcase. This patch disables that and ensures children in fragmented grids get expanded or contracted to match the edge of their container. Reftest added.

Disabling stretch will likely have other effects, of course. At the point this was prepared, mozilla-central was already failing css-grid reftests! Hard for me to tell whether or how much this patch is causing reftests to fail.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63252b25b41b

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8780659 [details] [diff] [review]
gridFrag2.patch

Is the problem here is that we skip AlignSelf altogether for
incomplete frames?  Maybe we could fix this by calling it but
pass in 'start' in the incomplete case, to apply its block-
start margin?

dholbert and I discussed alignment under fragmentation in London and
concluded that it's a really hard problem to solve for values other
than 'stretch'/'start'.  So I think using 'start' block alignment
for all fragmented frames is OK for now.

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1278058 Part 1: Make AlignJustifySelf return the calculated offset.

Review commit: https://reviewboard.mozilla.org/r/93544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93544/

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1278058 Part 2: Preserve stretched grid child size across fragmentation.

Review commit: https://reviewboard.mozilla.org/r/93546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93546/

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1278058 Part 3: Added a reftest of fragmented stretched grid children.

Review commit: https://reviewboard.mozilla.org/r/93548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93548/

Comment 7

[Brad Werth [:bradwerth]]

Comment on attachment 8811463 [details]
Bug 1278058 Part 2: Preserve stretched grid child size across fragmentation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93546/diff/1-2/

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8811464 [details]
Bug 1278058 Part 3: Added a reftest of fragmented stretched grid children.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93548/diff/1-2/

Comment 9

[Mats Palmgren (inactive)]

(FYI, it might take me a few weeks before I get to this review.)

Comment 10

[Brad Werth [:bradwerth]]

(In reply to Mats Palmgren (:mats) from comment #9)
> (FYI, it might take me a few weeks before I get to this review.)

Fair enough. After reviewing my try build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb2bf077bd74, I see that the proposed patch breaks most of the layout/reftests/css-grid/grid-fragmentation-* tests. I'll work through the changes necessary to keep all those passing as well as the new test, and then you'll see a new review request push.

Comment 11

[Brad Werth [:bradwerth]]

Comment on attachment 8811462 [details]
Bug 1278058 Part 1: Make AlignJustifySelf return the calculated offset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93544/diff/1-2/

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8811463 [details]
Bug 1278058 Part 2: Preserve stretched grid child size across fragmentation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93546/diff/2-3/

Comment 13

[Brad Werth [:bradwerth]]

Comment on attachment 8811464 [details]
Bug 1278058 Part 3: Added a reftest of fragmented stretched grid children.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93548/diff/2-3/

Comment 14

[Brad Werth [:bradwerth]]

I'm not actively working on this anymore. Taking myself off the bug.