1874823 - Merge NS_BLOCK_STATIC_BFC and NS_BLOCK_DYNAMIC_BFC into a single bit

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Frédéric Wang (:fredw)]

In bug 1765615, two flags were introduced to describe block formatting context:

• NS_BLOCK_STATIC_BFC for frames that are always BFC after construction/initialization.
• NS_BLOCK_DYNAMIC_BFC for frames to set the BFC property dynamically after a restyle (currently due to CSS paint/layout containment change)

We can probably merge them into a single flag.

Comment from Emilio on D198530:

So I guess the idea behind having both of these (instead of just one) is basically that we don't know otherwise whether a frame is meant to always be a BFC (because of NS_NewBlockFormattingContext or so) vs. due to a style change, right?
That seems fixable nowadays, tho... A lot of the anonymous blocks that we create like ::-moz-fieldset-content, etc could use display: flow-root rather than display: block for example.

Comment 1

[Frédéric Wang (:fredw)]

Attachment: Bug 1874823 - Use NS_BLOCK_DYNAMIC_BFC flag on multi-column containers. r=#layout

Per https://drafts.csswg.org/css-multicol/#columns:

"A multi-column container therefore is a regular block container that
establishes a new independent formatting context"

IsColumnContainerStyle() and BeginBuildingColumns() are currently
used in three places and always together (namely
ConstructFieldSetFrame, ConstructFrameFromItemInternal and the
generic ConstructBlock).

BeginBuildingColumns() asserts that aColumnContent is an nsBlockFrame
and that aComputedStyle corresponds to multi-column container. It
always adds the NS_BLOCK_STATIC_BFC to aColumnContent.

This patch instead makes StyleEstablishesBFC() return true for
IsColumnContainerStyle() so that the NS_BLOCK_DYNAMIC_BFC bit is
set at init and after further style updates. It thus removes the
need for to explicitly set NS_BLOCK_STATIC_BFC in
BeginBuildingColumns().

There is no behavior changes.

Comment 2

[Frédéric Wang (:fredw)]

Attachment: Bug 1874823 - Make nsBlockFrame::Init set NS_BLOCK_STATIC_BFC by checking classes. r=#layout

The following classes deriving from nsBlockFrame always set
NS_BLOCK_STATIC_BFC in their constructors or initializers:

• nsComboboxControlFrame
• nsFileControlFrame
• nsSelectsAreaFrame
• ColumnSetWrapperFrame
• nsMathMLmathBlockFrame

Do that in nsBlockFrame::Init instead, by attempting to cast to these
classes. Behavior is unchanged.

Comment 3

[Frédéric Wang (:fredw)]

With the two attached patches, the remaining AddStateBits(NS_BLOCK_STATIC_BFC) are:

• NS_NewBlockFormattingContext
• nsBlockFrame::Init
• nsBlockFrame::SetMarkerFrameForListItem
• nsFieldSetFrame::SetInitialChildList/InsertFrames
• nsCSSFrameConstructor::ConstructNonScrollableBlock

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a38e1f388fd2
Use NS_BLOCK_DYNAMIC_BFC flag on multi-column containers. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/f5d297181393
Make nsBlockFrame::Init set NS_BLOCK_STATIC_BFC by checking classes. r=layout-reviewers,AlaskanEmily

Comment 5

[Sandor Molnar[:smolnar]]

Backed out for causing assertion failures at builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/db6542a5ca6182386e394ed6199d33e0b0480e36

Push with failures

Failure log -> Assertion failure: false (MOZ_ASSERT_UNREACHABLE: aFrame should be a block), at /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:7768

Comment 6

[Frédéric Wang (:fredw)]

(In reply to Sandor Molnar[:smolnar] from comment #5)

Backed out for causing assertion failures at builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/db6542a5ca6182386e394ed6199d33e0b0480e36

Push with failures

Failure log -> Assertion failure: false (MOZ_ASSERT_UNREACHABLE: aFrame should be a block), at /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:7768

Mmh, indeed this new code is reachable, it's expected that there are other frame types that should render false. Will reland with that fixed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c0d41730484
Use NS_BLOCK_DYNAMIC_BFC flag on multi-column containers. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/4e0e2448d7a5
Make nsBlockFrame::Init set NS_BLOCK_STATIC_BFC by checking classes. r=layout-reviewers,AlaskanEmily

Comment 8

[Sandor Molnar[:smolnar]]

Backed out for causing assertion failures at builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/ce20634f32a1b78384fbbc028b4f86266056e7fb

Push with failures

Failure log -> Assertion failure: id != GetFrameId() (A frame failed to QueryFrame to its own type. It may be missing NS_DECL_QUERYFRAME, or a NS_QUERYFRAME_ENTRY() line with its own type name),

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/301bb1f39c5e
Use NS_BLOCK_DYNAMIC_BFC flag on multi-column containers. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/c6fb12843569
Make nsBlockFrame::Init set NS_BLOCK_STATIC_BFC by checking classes. r=layout-reviewers,AlaskanEmily

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/301bb1f39c5e
https://hg.mozilla.org/mozilla-central/rev/c6fb12843569

Comment 11

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #3)

With the two attached patches, the remaining AddStateBits(NS_BLOCK_STATIC_BFC) are:

• NS_NewBlockFormattingContext
• nsBlockFrame::Init
• nsBlockFrame::SetMarkerFrameForListItem
• nsFieldSetFrame::SetInitialChildList/InsertFrames
• nsCSSFrameConstructor::ConstructNonScrollableBlock

The remaining cases seem a bit more involved. I'm unassigning myself in case someone wants to take over.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1874823 - Remove NS_BLOCK_STATIC_BFC and NS_BLOCK_CLIP_PAGINATED_OVERFLOW. r=fredw,#layout

Put all the "is this block a BFC" logic in nsBlockFrame.cpp.

The CLIP_PAGINATED_OVERFLOW flag is also redundant, it can just be "has
non-propagated overflow styles" check in ShouldApplyOverflowClipping(),
and frees another bit.

Shouldn't change behavior.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18a81dc9dbb2
Remove NS_BLOCK_STATIC_BFC and NS_BLOCK_CLIP_PAGINATED_OVERFLOW. r=fredw

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/18a81dc9dbb2