Closed Bug 1622935 Opened 1 year ago Closed 11 months ago

Support pushing and splitting flex items for single-line (and some multi-line) flex container

Categories

(Core :: Layout: Flexbox, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [frag2020_v78][layout:backlog])

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
996 bytes, text/html
Details
84.50 KB, image/png
Details
47 bytes, text/x-phabricator-request
Details | Review

To reduce the scope of bug 939897, we can support shifting or splitting flex items for single-line column and single-line row flex container.

Per 10.1. Sample Flex Fragmentation Algorithm in the spec, both single-line row and single-line column flex container needs to run the flex algorithm without regards to pagination, which can be easier to implement.

Depends on: 1623225
Priority: -- → P3
Assignee: nobody → aethanyc
Status: NEW → ASSIGNED
Whiteboard: [frag_v77]
Depends on: 1625362
Depends on: 1627125
Whiteboard: [frag_v77] → [frag_v77][layout:backlog:77]
Whiteboard: [frag_v77][layout:backlog:77] → [frag_v77][layout:backlog]
Summary: Support shifting and splitting flex items on single-line column and single-line row flex container → Support pushing and splitting flex items for single-line flex container
Whiteboard: [frag_v77][layout:backlog] → [frag_v78][layout:backlog]

We need to do extra work when children is incomplete and that is also
what nsContainerFrame::PushIncompleteChildren() returns (we will call it
in Part 3).

Also, add relevant bits to be able to use them in the flex container.

Depends on D73165

Note: my current patches haven't considered advanced fragmentation properties like break-before, break-after, break-inside, nor dynamically inserting/appending/removing flex items. But they should be good enough to fix printing data loss.

Attachment #9144513 - Attachment description: Bug 1622935 Part 2 - Move grid container's NormalizeChildLists() and related helpers to container frame. → Bug 1622935 Part 2 - Move nsGridContainerFrame's NormalizeChildLists() and related helpers to nsContainerFrame.

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #7)

Note: my current patches haven't considered advanced fragmentation properties like break-before, break-after, break-inside, nor dynamically inserting/appending/removing flex items. But they should be good enough to fix printing data loss.

Thanks for that clarification.

Also: I think it's implicit in the [frag_v78] whiteboard status, but just to be extra-sure: we should wait to land this until Monday 5/4, after the merge (after the soft freeze is lifted & nightly version is bumped to version 78). This would definitely benefit from a full Nightly cycle for testing.

(Also, we should go through the list of dupes on bug 939897 at some point and see how many of them are fixed by this patch-stack, while keeping an eye out for e.g. catastrophic crashes or similar, to be sure we're not making things worse somehow. :))

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

Also: I think it's implicit in the [frag_v78] whiteboard status, but just to be extra-sure: we should wait to land this until Monday 5/4, after the merge (after the soft freeze is lifted & nightly version is bumped to version 78). This would definitely benefit from a full Nightly cycle for testing.

Yes, we should land this at the beginning of the Firefox 78 cycle. (We shouldn't land a major behavior change during the soft freeze anyway.)

(Also, we should go through the list of dupes on bug 939897 at some point and see how many of them are fixed by this patch-stack, while keeping an eye out for e.g. catastrophic crashes or similar, to be sure we're not making things worse somehow. :))

I'm planning to do what you've suggested! If some of the dupes on bug 937897 can be fixed by this bug, I'll make them dup over this bug instead. It will give us a better sense of the percentage of the real webpage that can be fixed, and perhaps give the reporters a heads-up that we do have some progress on it.

Duplicate of this bug: 1627642
Attached file break-test 1

So for me, this testcase is still broken with this patch-stack applied.

In the top part of this testcase (the part with a fragmented flex container), the 2nd flex item with the "def" text ends up with its text overlapping the multicol's red bottom-border (i.e. we put that item somewhere without enough available space). This results in truncation if this is printing instead of multicol.

I think this might be a signal that we're reporting too large of an availableBSize to this item, perhaps? (just a guess) So it thinks it can fit at least that first line of text, when really it cannot.

(We do successfully push the third flex item ("ghi") correctly into the second column, in this patch. It's the second flex item that's problematic here.)

Flags: needinfo?(aethanyc)

The broken testcase in comment 11 is somewhat expected for my current simple implementation because it simply lays the flex item out at where the flex algorithm tells it to and give the flex item whatever remaining available block-size, and punt the responsibility of breaking the item to the flex item itself.

(I'm aware that the test uses a more complex column-balancing scenario, so for the discussion below, I'll assume we use column-fill:auto on the multicol.)

Ideally, we could probably either do:

  1. As suggested by step 3 of the sample single-line column flex container algo, we can push the flex item immediately to the next page/column if we know the remaining available space is less than 50% of the item's height. But it can also look bad even if the remaining available space is more than 50% of the item's height, because the item might be unbreakable at all (like one line of text). We can still lose the content in printing.

  2. Ignore the 50% rule in the spec, and reflow the item anyway. If the item's desired height exceeds the remaining available height, push it to the next page/column.

If we implement pushing the entire flex item to the next page/column (maybe just for column-oriented flex container?), we'll need to reconsider some details of computing the flex container's height and the position offset of the pushed item.

In all, do you feel this should block this bug? Or maybe we can fix it in a follow-up?

Flags: needinfo?(aethanyc) → needinfo?(dholbert)

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #13)

  1. Ignore the 50% rule in the spec, and reflow the item anyway. If the item's desired height exceeds the remaining available height, push it to the next page/column.

I think we should do this ^^. The 50% rule seems bogus & I don't think it's something we should follow. As you noted -- even if 51% of the fragment would fit in the available space, that's still means 49% dataloss for that fragment, which is still bad.

But for the purposes of this bug, your current approach (putting the item wherever it falls and deferring to it to handle fragmentation) seems like an incremental improvement & indeed probably doesn't need to be blocked on this, I think.

Flags: needinfo?(dholbert)
Whiteboard: [frag_v78][layout:backlog] → [frag2020_v78][layout:backlog]
Blocks: 1637016
Component: Layout → Layout: Flexbox
Blocks: 1637091
Blocks: 1637145
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/78d4dfbda06a
Part 1 - Reverse the meaning of the children completeness returned by ReflowChildren(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/818c87e00d37
Part 2 - Move nsGridContainerFrame's NormalizeChildLists() and related helpers to nsContainerFrame. r=mats
https://hg.mozilla.org/integration/autoland/rev/5c6cc9d819b9
Part 3 - Implement fragmentation for single-line flex container. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6a21028d21a8
Part 4a - Add base single-line column/row flex container fragmentation reftests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/988074d5ce54
Part 4b - Add base single-line column-reverse/row-reverse flex container fragmentation reftests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1035de8a28ce
Part 4c - Add additional single-line flex container fragmentation reftests. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/76aa4bf4eabd
Part 4d - Add more reftests for single-line flex container in zero height multicols. r=dholbert
Regressions: 1637501

Change the title to reflect the fact that my patches can also fix print data loss of some real sites using multi-line row-oriented flex containers.

Summary: Support pushing and splitting flex items for single-line flex container → Support pushing and splitting flex items for single-line (and some multi-line) flex container
Duplicate of this bug: 1634125
Duplicate of this bug: 1613188
Duplicate of this bug: 1604390
Duplicate of this bug: 1602130
Duplicate of this bug: 1599328
Duplicate of this bug: 1591953
Duplicate of this bug: 1587515

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #18)

Change the title to reflect the fact that my patches can also fix print data loss of some real sites using multi-line row-oriented flex containers.

That's good news!

It might be worth checking in some automated testcases to cover those (in a test-only followup bug), to be sure we don't regress them in the time between now & when we "really" fix multi-line flex container fragmentation. What do you think?

Flags: needinfo?(aethanyc)
Blocks: 1637788

Re comment 26:

Sure, I can add some reftests for multi-line row-oriented flex containers in bug 1637788 as a first step to figure out what's left to do. (And maybe some multi-line column-oriented ones if I see some real sites using it need to print.)

Flags: needinfo?(aethanyc)
Duplicate of this bug: 1579358
Duplicate of this bug: 1565186
Duplicate of this bug: 1549330
Duplicate of this bug: 1493780
Duplicate of this bug: 1489012
Duplicate of this bug: 1486334
Duplicate of this bug: 1486302
Duplicate of this bug: 1464726
Duplicate of this bug: 1439156
Duplicate of this bug: 1419377
Duplicate of this bug: 1414253
Duplicate of this bug: 1375736
Duplicate of this bug: 1331151
Duplicate of this bug: 1283642
Duplicate of this bug: 1241323
Duplicate of this bug: 1131703
Duplicate of this bug: 1123633
Duplicate of this bug: 1095440
Duplicate of this bug: 1089549
Duplicate of this bug: 993362
Duplicate of this bug: 193211

\o/ Amazing! A lots of duplicated bugs!

Regressions: 1640028
Regressions: 1640051
Regressions: 1640275
Duplicate of this bug: 856235
Duplicate of this bug: 1315994
Duplicate of this bug: 1642847
Duplicate of this bug: 1660472
Duplicate of this bug: 1421462
Regressions: 1678469
Regressions: 1680406
You need to log in before you can comment on or make changes to this bug.