Closed Bug 1548118 Opened 1 year ago Closed 1 year ago

Fix nested column balancing after enabling column-span

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(4 files)

The first set of nested columns in layout/reftests/columns/column-balancing-nested-001.html is failed after enabling column-span like shown in reftest-analyzer.

The problem in the first set of nested columns happens when the inner column-fill:auto ColumnSetWrapper (the twos) is reflowed in the column #0 of the outer column, which doesn't have enough height already occupied by ones. So the inner ColumnSetWrapper's fill the twos in two columns, and it is put in the overflow line in column #0.

Later the inner ColumnSetWrapper is drained into column #1 of the outer column which has enough height. However, it doesn't get mark as dirty beacuse this is considered as ReparentingDirection::Forwards according to [1], so it's ColumnSet children do not have a change to reflow because a non-dirty block won't mark its lines dirty [2]. If the inner ColumnSetWraper is dirty, it should be able to fill the twos in just one column.

[1] https://searchfox.org/mozilla-central/rev/f0ef51bfafc8ad0c3a2f523bf076edc57dc4891a/layout/generic/nsBlockFrame.cpp#621-625
[2] https://searchfox.org/mozilla-central/rev/f0ef51bfafc8ad0c3a2f523bf076edc57dc4891a/layout/generic/nsBlockFrame.cpp#2188

We use columnCount == aConfig.mBalanceColCount - 1 in other places to
determine if we are at the last columns. Advancing the column count at
the end of the loop make the condition consistent.

I don't see how any of these patches are related to each other, really... though the first two patches look fine.

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #5)

I don't see how any of these patches are related to each other, really... though the first two patches look fine.

Right, Part 1 and Part 2 are not directly related to the solution in this bug. They're merely refactoring during my investigation of column balancing that could be helpful in other bugs. If it's OK, I'd like them to land sooner rather than stay in my patch queue, so they end up here ...

Attachment #9061795 - Attachment description: Bug 1548118 Part 3 - Always mark inner columns dirty before reflowing it if the outer columns is balancing. → Bug 1548118 Part 3 - Always mark inner ColumnSetWrapper's children dirty before reflowing it if the outer ColumnSetWrapper is balancing.
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f3821529e276
Part 1 - Advance the column count at the end of the while loop. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/40a074be8bf0
Part 2 - Extract computing column balancing depth logic to a function. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/516cc2c504f5
Part 3 - Always mark inner ColumnSetWrapper's children dirty before reflowing it if the outer ColumnSetWrapper is balancing. r=dbaron

Oops, column-balancing-nested-001.html fail on macOS and Windows, and I usually run just Linux tests in my try push ...

Anyway, column-balancing-nested-001.html uses margin-bottom: 1em, and per bug 1487927, margin-bottom is not always rendered as expected with our column balancing algorithm. Depending on column count and the number of balancing iterations, the result may vary. What's more, column-balancing-nested-001.html is already failed locally on my macbook without my patch and with column-span disabled, I don't know how it got passed on our CI. I think removing margin-bottom: 1em doesn't affect what the test is trying to cover, and can yield more stable test results.

Flags: needinfo?(aethanyc)

Per bug 1487927, margin-bottom value is not always rendered as expected
with our column balancing algorithm. I'd like to remove it from
column-balancing-nested-001.html, and add <br> to separate each cases.

Attachment #9063363 - Attachment description: Bug 1548118 Part 3.1 - Remove margin-bottom from column-balancing-nested-001.html and its reference. → Bug 1548118 Part 3 - Remove margin-bottom from column-balancing-nested-001.html and its reference.
Attachment #9061795 - Attachment description: Bug 1548118 Part 3 - Always mark inner ColumnSetWrapper's children dirty before reflowing it if the outer ColumnSetWrapper is balancing. → Bug 1548118 Part 4 - Always mark inner ColumnSetWrapper's children dirty before reflowing it if the outer ColumnSetWrapper is balancing.
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3db2c18f9950
Part 1 - Advance the column count at the end of the while loop. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/152fb420ffc5
Part 2 - Extract computing column balancing depth logic to a function. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/970a59a74aed
Part 3 - Remove margin-bottom from column-balancing-nested-001.html and its reference. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/cef0cd788205
Part 4 - Always mark inner ColumnSetWrapper's children dirty before reflowing it if the outer ColumnSetWrapper is balancing. r=dbaron
You need to log in before you can comment on or make changes to this bug.