1527734 - {inc} [css-grid] Dynamic changes to "min-height" produce inconsistent results, on a table-as-a-grid-item

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load attached testcase.
2. Hover the second grid.

EXPECTED RESULTS:
It should look identical to the first grid (because they have the same styling at that point).

ACTUAL RESULTS:
It's still much taller (and in particular, its gray child is taller) as compared to the first grid.

Current Firefox Nightly (and a local build with bug 1526567's patch) gives ACTUAL RESULTS.
Chrome gives EXPECTED RESULTS.

(I ran into this when investigating the test failure mentioned in bug 1526567 comment 3. The testcase is derived from reftest 'grid-item-table-stretch-002.html'. I'm not sure whether or not this particular bug is the cause of the android test-failure that mats noted there, though.)

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

EXPECTED RESULTS:
It should look identical to the first grid (because they have the same styling at that point).

(I just noticed the styling doesn't quite end up the same -- I used min-height:0px in one case vs. unitless 0 in the other -- but it doesn't make a difference; the semantics are the same and the test's behavior doesn't change if I edit it to make that consistent.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (simpler, no interaction required)

Here's a testcase that doesn't require any interaction. In Firefox Nightly, the second grid/table here doesn't change its height after the dynamic tweak happens.

Note: if you have devtools open when loading this testcase, then we do get expected results. This is presumably because devtools triggers extra [or more thorough] reflows, which are sufficient to produce the correct layout.

Comment 3

[Mats Palmgren (inactive)]

Attachment: testcase 1b

Thanks for the investigation / testcases. Here's a variation on
"testcase 1" that removes the intrinsic sizing of the grid container
itself (by setting an explicit width/height) and also use a definite
column size. I noticed that the difference in height when hovering
is exactly the height of the caption (I made it smaller here to
illustrate). The problem appears to go away if I remove the caption.
Also, the bug occurs for any of auto/min-content/max-content track
min-sizing.

Comment 4

[Mats Palmgren (inactive)]

So the reason the inner table frame get some height here is that we
stretch it in nsFrame::ComputeSize and thus compute a definite
ComputedBSize in the ReflowInput.
https://searchfox.org/mozilla-central/rev/01b4b3830ea3cae2e9e431019afa6391b471c6da/layout/generic/nsFrame.cpp#5513
This was supposed to NOT happen since MeasuringReflow set that
nsIFrame::eUseAutoBSize bit, through COMPUTE_SIZE_USE_AUTO_BSIZE here:
https://searchfox.org/mozilla-central/rev/01b4b3830ea3cae2e9e431019afa6391b471c6da/layout/generic/nsGridContainerFrame.cpp#3378
However, this bit is only set on the TableWrapperFrame's state,
not on the (inner) TableFrame's. Propagating it seems to fix
this bug. It also fixes bug 1492315.

Comment 5

[Mats Palmgren (inactive)]

I'm still a bit puzzled why the reftest failure in bug 1526567
only occurs on Android and not other platforms, oh well.

Comment 6

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

While investigating relevant code related to table caption's block-size in bug 1674302, I notice grid container stores the containing block-size (percentBasis) [1] on table wrapper grid item in GridItemCBSizeProperty, and later transfer (with some modification) to inner table frame. However, percentBasis is already passed into ReflowInput's constructor [2], and the size is stored in ReflowInput::mContainingBlockSize [3].

By removing GridItemCBSizeProperty and using ReflowInput::mContainingBlockSize instead, and doing necessary computing only if block-size is unconstrained, we can solve this bug, and part of bug 1521088.

I'll upload a patch with tests.

[1] https://searchfox.org/mozilla-central/rev/f982032c7c7618c626165bb557968f478a1952dc/layout/generic/nsGridContainerFrame.cpp#7262-7272
[2] https://searchfox.org/mozilla-central/rev/f982032c7c7618c626165bb557968f478a1952dc/layout/generic/nsGridContainerFrame.cpp#7247-7248
[3] https://searchfox.org/mozilla-central/rev/f982032c7c7618c626165bb557968f478a1952dc/layout/generic/ReflowInput.cpp#2373-2374

Comment 7

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

Attachment: Bug 1527734 - Use ReflowInput::mContainingBlockSize instead of GridItemCBSizeProperty().

Grid container stores the containing block size on a table wrapper grid
item in GridItemCBSizeProperty(), but the size is also passed as a
parameter when creating table wrapper's ReflowInput, and is stored in
ReflowInput::mContainingBlockSize.

Switching to mContainingBlockSize also fixed bug 1527734, and part of
bug 1521088. See Part 2 for the reftests.

The test 002.html is inspired from the testcases in bug 1527734.
003.html and 004.html are inspired from the testcase in bug 1521088.
004.html is 003.html with 'width:500px' added to <table>, which we still
fail with this patch.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ab10a81eb56
Use ReflowInput::mContainingBlockSize instead of GridItemCBSizeProperty(). r=mats

Comment 9

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

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

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9ab10a81eb56

Comment 11

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

Upstream PR merged by moz-wptsync-bot