1533346 - Multicolumn does not balance correctly when height is small

Status: RESOLVED DUPLICATE of bug 1411422

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

Description

[violet.bugreport]

Attachment: broken-column.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.121 Safari/537.36

Steps to reproduce:

Open the attached html in Firefox

Actual results:

The columns are balanced incorrectly, the height of 3 columns are not same.

Expected results:

All columns should have the same height, this is also the behavior of Chrome and Safari

Comment 1

[violet.bugreport]

The balancing will only be correct when content height is greater than multicol's height. Because for a given attempt of balancing, if the multicol itself's height cannot be fully consumed, the implementation considers it Incomplete and refuse to balance it at this attempt.

Emilio, do you know how to check if "all its content has been consumed, even if itself's height has not yet been consumed"? I think this problem can be fixed if we can check that.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

I'm not particularly familiar with multicol balancing, though I'm happy to help. Can you be more concrete on where do you need to check for that?

Also I guess the multicol height might depend on the content height, so the check might be tricky? But that may be wrong, as I said I'm not particularly familiar with multicol.

Also, which SetIncomplete() call are you referring to?

Comment 3

[violet.bugreport]

Let's take the attached simple html as example, the outer div.a has height 120px, but its content (i.e. div.b) only has 60px. Both intuition and other browsers use 60px to distribute to 3 columns, 20 + 20 + 20. But our current implementation uses the outer 120px to balance, the first 60px has content, the remaining 60px is empty. That causes the weird balancing here.

The cause is that the current implementation uses a binary search to find the minimal height to satisfy 3 columns. If an attempted height cannot make the reflow of the multicol fully complete, binary search will try a bigger height. See https://github.com/mozilla/gecko-dev/blob/master/layout/generic/nsColumnSetFrame.cpp line 758, 814.

It checks if all the height of the multicol has been consumed in line 811, I think the correct way is to check if all its content has been completed.

I'm quite new about the codebase, it's likely there're better way to resolve it.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Ah, I see. That seems wrong to me, the Gecko behavior seems what I'd expect. But again, probably not the right person to make judgements about multicol.

Mats / David / TYLin, any opinion?

Comment 5

[Mats Palmgren (inactive)]

(In reply to violet.bugreport from comment #0)

Actual results:

The columns are balanced incorrectly, the height of 3 columns are not same.

Just a minor nitpick here -- we create 3 columns for the given testcase,
all with 100px width and (all) have 40px height. The fragments of
the <div> inside the columns have different size in column 1 and 2 though,
which is probably what you meant. (the 1st fragment fills the first column
and the 2nd fragment has 28px height)

Comment 6

[Mats Palmgren (inactive)]

Yeah, choosing 40px as the column height seems odd to me.

Arguably, the correct layout per spec is a 68px non-fragmented
<div> in the 1st column, and empty 2nd & 3rd columns. This is
because the <div> has zero 'possible breakpoints' per:
https://www.w3.org/TR/css-break-3/#possible-breaks
Also, one should minimize the number of breaks, per:
https://www.w3.org/TR/css-break-3/#best-breaks
So, given that the <div> doesn't overflow the container,
there's no need to force it to break in this case.

That said, if the <div> actually had a line box, e.g. <div>x</div>
then it would have a (Class C) break opportunity after that line,
and then it would be reasonable to produce two fragments.
Each fragment should probably have 30px content-box size in that
case to satisfy the 2nd bullet in §4.5.

Maybe we treat an empty block (with non-zero block-size) as if it
has a line box for breaking purposes and that's why we end up with
the current layout?

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #2, with the <div> replaced by <img> for comparison

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #3

That said, even after adding break opportunities, we still make
the columns 40px.

Comment 9

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

After bug 1411422 is landed, the columns in the test cases in comment 0 and comment 8 should now break evenly. This can be observed on current Nightly either with or without layout.css.column-span.enabled.

Mats, you argued in comment 6 that a <div> without any line-box shouldn't be forced to break according to the spec. But in our implementation as well as other rendering engines, column-balancing has a priority to force break the empty <div>, and this behavior seems more likely to meet author's expectation. I'd like to call this bug done. What do you think?

Comment 10

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

Per comment 9, I'd like to close this bug since the columns are now balanced at the same height as expected.