1564649 - multicol reflow doesn't dirty enough when being reflowed without any dirty bits

Status: RESOLVED FIXED

(Core :: Layout: Columns, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

While debugging the test failures from the patch in bug 1351924, I found one set of them (in the two locations in which we run multicol-zero-height-001.xht) to be an existing bug that was simply exposed by the patch.

I'm frankly, skeptical of all of the optimizations that allow the multicolumn code to skip reflowing one of the columns if the height has changed (i.e., when changingBSize and colBSizeChanged are true in nsColumnSetFrame::ReflowChildren); it's not clear to me why we can ever guarantee that one of the children won't be a different size given a different column height. In this particular case, however, I think it was exposed by the combination of this code in nsColumnSetFrame::Reflow:

  // Our children depend on our block-size if we have a fixed block-size.
  if (aReflowInput.ComputedBSize() != NS_AUTOHEIGHT) {
    AddStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);
  } else {
    RemoveStateBits(NS_FRAME_CONTAINS_RELATIVE_BSIZE);
  }

combined with the fact that before the patch, the block-resize flag was set, thus leading to ShouldReflowAllKids() being true before the patch.

However, while I'm tempted to just rip out many of the optimizations in that function, given that so far I've only demonstrated that they're broken when the ComputedBSize() is not auto, I'm inclined to just change the definition of skipIncremental to also consider that case.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The basic sequence of events with the testcase was that:

• on the first reflow, the multicol would try reflowing itself three times: the first time with height 0, the second time with height 599 (just under 10px), and the third time with height 60 (1px). All three reflows would reflow every column (the first two everything was dirty and v-resize; not entirely sure about the third)
• on the second reflow, the multicol would try reflowing itself twice: the first time with height 0, and the second time with height 60 (1px). The first one would reflow all of the columns at a 0 height; it gave the float inside those columns a height of 0 in the first 9 columns and 600 (10px) in the last column. For the second reflow of the columns, we'd then skip the first nine columns, and distribute 1px of the float's height across each of columns 10 through 19, creating 9 extra columns.

This patch makes the second reflow of the columns -- either in the testcase here, or in the original testcase combined with the patch in bug 1351924, reflow all of the columns.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Here's a try run.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Bug 1564649 - Remove a duplicate variable where we had two variables for the same thing, and a little related cleanup.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Bug 1564649 - Add reftest.

Depends on D37515

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Bug 1564649 - Reflow all columns when a multicol with non-auto block-size balances its columns at a different block-size.

Depends on D37516

Comment 6

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

Bugbug thinks this bug is a enhancement, but please change it back in case of error.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Here's a new try run with the revisions.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c34163e3ba0b
Remove a duplicate variable where we had two variables for the same thing, and a little related cleanup. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/fed851fb8e83
Add reftest. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/ac4398269328
Reflow all columns when a multicol with non-auto block-size balances its columns at a different block-size. r=TYLin

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c34163e3ba0b
https://hg.mozilla.org/mozilla-central/rev/fed851fb8e83
https://hg.mozilla.org/mozilla-central/rev/ac4398269328