Closed Bug 1564649 Opened 9 months ago Closed 9 months ago

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

Categories

(Core :: Layout: Columns, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(4 files)

Attached file 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.

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.

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

Type: defect → enhancement
Type: enhancement → defect
Attachment #9076996 - Attachment description: Bug 1564649 - Remove a duplicate variable where we had two variables for the same thing, and a little related cleanup. r?TYLin → Bug 1564649 - Remove a duplicate variable where we had two variables for the same thing, and a little related cleanup.
Attachment #9076997 - Attachment description: Bug 1564649 - Add reftest. r?TYLin → Bug 1564649 - Add reftest.
Attachment #9076998 - Attachment description: Bug 1564649 - Reflow all columns when a multicol with non-auto block-size balances its columns at a different block-size. r?TYLin → Bug 1564649 - Reflow all columns when a multicol with non-auto block-size balances its columns at a different block-size.
Pushed by dbaron@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.