1574046 - A div containing floats and clear, fragmented into multiple columns, has unconstrained block-size

Status: RESOLVED FIXED

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

Description

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

I follow this review comment to add an assertion in nsBlockReflowInput::nsBlockReflowInput that mConsumedBSize is not NS_UNCONSTRAINEDSIZE, but this try run shows float-clear-001.html and a few other tests can trigger this assertion.

From the frame tree dump for float-clear-001.html, I can see that <div class="container">, which has floats & clear, and being fragmented to multiple columns, has unconstrained block-size.

Comment 1

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

Seems like there might be a behavior regression in those tests from bug 1569701, then, so we should probably fix this in the same cycle.

Comment 2

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

The behavior change from bug 1569701 (bug 1569701 Part2 specifically) should be in effect only when column-span is enabled. However, this bug can be observed on current release Firefox 68's devtool by inspecting <div class="container"> in float-clear-001.html, so this is not a recent regression at least.

From a quick debugging, I see that when reflowing the <div class="container">, nsFloatManager::ClearFloats returns nscoord_MAX, and later the nscoord max value is set on BlockReflowInput::mBCoord, which becomes the container's block-size when computing in nsBlockFrame::ComputeFinalSize.

David, do you recall why we return nscoord_MAX offhand? I have difficulty figuring out by reading bug 563584 patch 9 that added it.

Comment 3

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

One of the rules for placement of floats is that the top of a later float can't be higher than the top of an earlier float. So if we've pushed an earlier float to the next column, that means we can't try to fit any later floats in that column either. In some of those functions it handles that by telling the caller that a float won't fit at all -- but that's not the right thing if the caller wants to know what the right block height is to include floats. So probably either more information needs to be returned to the callers rather than just nscoord_MAX, or arguments need to be added to some of the functions (or functions split up).

Comment 4

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 5

[janet [:janet]]

Are you planning to fix this for 70?

Comment 6

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

I can take a look at this later, but I don't have a plan to fix this in Firefox 70. I open float-clear-001.html in Firefox 39, and this bug was already there since we enabled float fragmentation in bug 1148104. I feel it doesn't worth tracking for current release.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Bug 1574046 - Wallpaper the effect of this bug in nsBlockFrame::ComputeFinalSize. r=TYLin

Depends on D56416

Comment 8

[Mats Palmgren (inactive)]

I'm just wallpapering over one effect of this bug that blocks bug 1602430.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44689b5ff306
Wallpaper the effect of this bug in nsBlockFrame::ComputeFinalSize.  r=TYLin

Comment 10

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/44689b5ff306

Comment 11

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

Attachment: Bug 1574046 Part 1 - Upstream float clear reftests to wpt, and add column-fill:auto counterparts.

According to the description in the tests, "The orange line should be
halfway down the third column", I think they meant to fill the column
sequentially when fragmenting the float elements.

Comment 12

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

Attachment: Bug 1574046 Part 2 - Move calling nsFloatManager::ClearContinues to BlockReflowInput::ClearFloats().

All the callers in nsBlockFrame call BlockReflowInput::ClearFloats(), I
don't feel we need to pass the DONT_CLEAR_PUSHED_FLOATS down to
nsFloatManager. Besides, I'll remove the flag in a later patch.

Depends on D74536

Comment 13

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

Attachment: Bug 1574046 Part 3 - Make BlockReflowInput::ClearFloats() return ClearFloatsResults.

This change doesn't change the behavior yet. It just changes all the
callers by having them catch the ClearFloatsResults.

Some of the callers will be revised in next patch by utilizing the
returned results. Some of the return values are not being used, and may
produce warnings, they will be suppressed in the next patch, too.

Depends on D74537

Comment 14

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

Attachment: Bug 1574046 Part 4 - Use BlockReflowInput::ClearFloatsResult to replace coordinate comparison.

The rule I use when replacing the block-dir coordinates comparison
between the one passing to ClearFloats() and the one returned by
ClearFloats().

• If they are equal, check the result has BCoordNoChange.

• If they are not equal, check the result has BCoordAdvanced or
  FloatsPushedOrSplit.

We want to do this conversion because in the next patch, I want to stop
ClearFloats() from returning nscoord_MAX, which is equivalent to
checking FloatsPushedOrSplit.

Depends on D74538

Comment 15

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

Attachment: Bug 1574046 Part 5 - Prevent BlockReflowInput::ClearFloat from returning nscoord_MAX.

This effectively makes ClearFloat() act like it always has
DONT_CLEAR_PUSHED_FLOATS. Thus, the flag is no longer needed.

We need to add an early return condition when the block cannot fit in
ReflowBlockFrame(). Because we now don't return nscoord_MAX when floats
are pushed or split, the availSpace.BSize(wm) might equal to zero
rather than negative in this case. It's needed for
layout/reftests/pagination/float-clear-003.html and
layout/reftests/pagination/float-clear-003-print.html

Depends on D74539

Comment 16

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

Here a try run of my current patches.

Comment 17

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

Attachment: Bug 1574046 Part 3a - Remove Unused introduced in Part 4.

If this looks good, it should be merged into Part 3 so that those
Unused is not needed in the first place.

Making ClearFloats returning a struct containing nscoord and an enum has
been considered, but sadly it cannot be used with std::tie().

Depends on D74540

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa23badb0824
Part 1 - Upstream float clear reftests to wpt, and add column-fill:auto counterparts. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/4140c68594e3
Part 2 - Move calling nsFloatManager::ClearContinues to BlockReflowInput::ClearFloats(). r=dbaron
https://hg.mozilla.org/integration/autoland/rev/b217eaadc9d9
Part 3 - Make BlockReflowInput::ClearFloats() return ClearFloatsResults. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/2490a3ca2481
Part 4 - Use BlockReflowInput::ClearFloatsResult to replace coordinate comparison. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/e1232e2aeacf
Part 5 - Prevent BlockReflowInput::ClearFloat from returning nscoord_MAX. r=dbaron

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/24179 for changes under testing/web-platform/tests

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 21

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/aa23badb0824
https://hg.mozilla.org/mozilla-central/rev/4140c68594e3
https://hg.mozilla.org/mozilla-central/rev/b217eaadc9d9
https://hg.mozilla.org/mozilla-central/rev/2490a3ca2481
https://hg.mozilla.org/mozilla-central/rev/e1232e2aeacf

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot