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

ASSIGNED
Assigned to

Status

()

P3
normal
ASSIGNED
3 years ago
a year ago

People

(Reporter: mats, Assigned: bradwerth)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8759980 [details]
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → bwerth
(Assignee)

Comment 1

2 years ago
Created attachment 8776197 [details] [diff] [review]
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
Attachment #8776197 - Flags: feedback?(mats)
(Assignee)

Comment 2

2 years ago
Created attachment 8780659 [details] [diff] [review]
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
Attachment #8776197 - Attachment is obsolete: true
Attachment #8776197 - Flags: feedback?(mats)
Attachment #8780659 - Flags: feedback?(mats)
(Reporter)

Comment 3

2 years ago
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.
Attachment #8780659 - Flags: feedback?(mats)
(Assignee)

Updated

2 years ago
Blocks: 1303154
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8780659 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

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

Comment 10

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8811462 - Flags: review?(mats)
Attachment #8811463 - Flags: review?(mats)
Attachment #8811464 - Flags: review?(mats)
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.