1799530 - tall flex container gets unnecessarily pushed to the next page (treated as monolithic or unfragmentable for some reason)

Status: VERIFIED FIXED

(Core :: Layout: Flexbox, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1: row/wrap flex container, flex items 40px tall

STR:

1. Load attached testcase.
2. Print-preview

ACTUAL RESULTS:
The black-bordered flex container gets pushed to page 2, indicating that we think it's un-fragmentable/monolithic.

EXPECTED RESULTS:
It should be fragmentable, I think?

NOTE 1: We're not leaning too heavily on the fragmentation fallback codepath here; I still see essentially the same results (flex container pushed to page 2, and then fragmented to many pages of output) if I turn that off by setting layout.display-list.improve-fragmentation to false.

NOTE 2: We seem to be clipping some text at the bottom of pages (and then painting it at the top of the next page if I enable fragmentation fallback), if the line sizing is right such that text lands at the bottom of a page.

NOTE 3: with certain specified height values (e.g. 200px), the flex container does end up on the first page (instead of getting pushed to page 2). I'll post some additional testcases to demonstrate.

This seems to be part of what's going wrong in bug 1799130; it's why page 2 is mostly blank there, I think.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: column flex container, flex items 40px tall

I get the same results for a column-oriented flex container as shown here.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 3: column flex container, flex items 200px tall (works better for some reason)

As shown here: with 200px tall flex items, we do end up matching expected-results and starting on the first page for some reason.

(This one is using a column-oriented flex container; I see the same improvement for a similar row/wrap scenario as well.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 4: column flex container, flex items 300px tall (broken)

...but it's broken again if I continue increasing the size; e.g. this testcase with 300px is broken (flex container pushed to page 2) just like with 40px.

Comment 4

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

This bug is similar to bug 1743890. We are currently unable to push a monolithic flex item to the next page if the flex container is running out of available block-size. (We treat a line of text as monolithic.) Therefore, the last unable-to-fit flex item in current page contributes to the flex container's block-size, which makes the entire flex container's block-size exceeds the available block-size. In these testcases, the line containing the flex container is not the first line in a block frame (the first line is "Does the following content start on this same page?"), nsBlockReflowContext::PlaceBlock() [1] will return false to stop the reflow, and the block frame will push the line containing flex container to the next-in-flow [2].

[1] https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/layout/generic/nsBlockReflowContext.cpp#413-422
[2] https://searchfox.org/mozilla-central/rev/3c194fa1d6f339036d2ec9516bd310c6ad612859/layout/generic/nsBlockFrame.cpp#4326-4328

Comment 5

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

Attachment: Bug 1799530 - Add flexbox fragmentation printing tests in wpt. r?dholbert

These tests are adapted from Daniel Holbert's testcases attached in the bug.

Depends on D165192

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:TYLin, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 7

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

The r+ patch only adds testcases, and I'm planning to land it with patches in Bug 1743890.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f68d325800b
Add flexbox fragmentation printing tests in wpt. r=dholbert

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/38342 for changes under testing/web-platform/tests

Comment 10

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

Since my patches in Bug 1743890 need more time to cook, I landed the testcases in this bug first to reduce the chance of naming clash with upsteam wpt. Note this bug is not fixed yet, but it will be fixed by bug 1743890.

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7f68d325800b

Comment 12

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

Upstream PR merged by moz-wptsync-bot

Comment 13

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

With Bug 1743890 closed, this bug is now fixed.

Comment 14

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-03-15 Nightly build on macOS 12 using the attached testcases.
Verified as fixed on Firefox 113.0b5(build ID: 20230418175842) and Nightly 114.0a1(build ID: 20230419214510) on macOS 12, Windows 10, Ubuntu 22.