1602430 - min/max-height is applied incorrectly in fragmented flow

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P2)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase

STR

1. load the attached testcase
2. Print Preview (A4 size)

EXPECTED RESULT
The word "AFTER" should be in the upper half of the second page, overlapped by the inner element with the borders.

ACTUAL RESULT
The word "AFTER" is near the end of the second page, after the inner element with the borders.

Comment 1

[Mats Palmgren (inactive)]

This is a pretty straight-forward fix... if it weren't for this rule:
https://searchfox.org/mozilla-central/rev/d24696b5abaf9fb75f7985952eab50d5f4ed52ac/layout/style/res/ua.css#264
which is pretty broken. If you have a column-set with say height:100px and three columns, then the max-block-size needs to be 300px not 100px (the max-block-size applies to the sum of the fragments, not each fragment individually).
What is this declaration needed for exactly?

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Mats Palmgren (:mats) from comment #1)

This is a pretty straight-forward fix... if it weren't for this rule:
https://searchfox.org/mozilla-central/rev/d24696b5abaf9fb75f7985952eab50d5f4ed52ac/layout/style/res/ua.css#264
which is pretty broken.
If you have a column-set with say height:100px and three columns, then the max-block-size needs to be 300px not 100px (the max-block-size applies to the sum of the fragments, not each fragment individually).

You're talking about our implementation details that if we really want to set max-block-size to the column-set (or column-content?), it should be a 300px, right? Because I cannot find a spec text saying that the max-block-size specified on the multicol container should be a multiply of its block-size and column-count.

What is this declaration needed for exactly?

If I recall this correctly, this is for the legacy column container (with column-span disabled) which doesn't fully respect the column container's height when the container is fragmented. Nowadays, nsColumnSetFrame is an inner frame under ColumnSetWrapperFrame, it should fully respect the whatever available block-size passing down from ColumnSetWrapperFrame, and use it to impose the available block-size of column-content. The multicol's child contents shouldn't choose a block-size bigger than their available block-size.

Out of curiosity, I remove the max-block-size: 100%, and it breaks only three abspos dynamiclly height changing tests, which I guess is due to some optimization. I'd say we should remove the max-block-size rule.
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=280433686&revision=7f9269054bc203737ded74f38f6bc69eb820c0a2

Comment 3

[Mats Palmgren (inactive)]

Right, min/max-block-size applies to the CSS box, which means it applies to the sum of the heights of the fragments associated with that box (same as for block-size). In our column set implementation the columns are fragments, so it's no different from any other box. So, by specifying max-block-size: 100% only the first column has actual in-flow block-size and the remaining columns becomes overflow-containers, or should have anyway. The only reason that doesn't happen is that we don't actually apply min/max-block-size at all in fragmented contexts currently which I intend to fix in this bug.

I filed bug 1603088 to remove that declaration.

Comment 4

[Mats Palmgren (inactive)]

Attachment: block-min-height-001.html

A couple of tests to illustrate just how broken our layout currently is. See Chrome for correct results.

Comment 5

[Mats Palmgren (inactive)]

Attachment: block-max-height-001.html

Comment 6

[Mats Palmgren (inactive)]

Attachment: Bug 1602430 - Apply min/max-height correctly for fragmented boxes. r=TYLin

Depends on D56738

Comment 7

[Mats Palmgren (inactive)]

Attachment: Bug 1602430 - Apply min/max-height correctly for fragmented boxes with no height specified. r=TYLin

Depends on D56738

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5c8349e4e35
Apply min/max-height correctly for fragmented boxes with no height specified.  r=TYLin

Comment 9

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

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

Comment 10

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 11

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/b5c8349e4e35

Comment 12

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

Upstream PR merged by moz-wptsync-bot