1874826 - Avoid reconstruction due to change to block formatting context

Status: RESOLVED FIXED

(Core :: Layout: Floats, task)

Description

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

See nsBlockFrame::Init, nsStyleDisplay::CalcDifference, TryToHandleBlockFormattingContextChange as introduced by bug 1765615.

A block frame has the NS_BLOCK_DYNAMIC_BFC flag set iff effective containment is set to paint or layout.

The nsChangeHint_UpdateBFC hint indicates that that flag should be updated to due change in effective containment.

Currently, TryToHandleBlockFormattingContextChange just forces nsChangeHint_ReconstructFrame if the frame may contain any float. Emilio commented on D197043:

If we could fix this up rather than reconstructing, we could move all this logic to nsBlockFrame::DidSetComputedStyle, and remove UpdateBFC.

Comment 1

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

So to elaborate a bit more, in the past modifying the containment of a frame was forcing a frame reconstruction. After D197043, we now avoid many unnecessary reconstructions. However if the state of NS_BLOCK_DYNAMIC_BFC is changed (due to paint/layout containment change), we keep forcing reconstruction if the frame may contain floats.

This is the safest option: NS_BLOCK_DYNAMIC_BFC affects whether a frame should have its own nsFloatManager, but if the updated frame does not contain any float we don't need to worry about whether they become handled by this frame's or an ancestor's nsFloatManager. The goal of this bug is to see if we can optimize this further.

The current approach with nsChangeHint_UpdateBFC was inspired by Daniel's comment in bug 1765615 comment 1, but here the situation is a bit different from nsChangeHint_UpdateContainingBlock and fixed/absolutely positioned nodes, so we can probably do something less complex. For example just marking some frames dirty as in https://searchfox.org/mozilla-central/rev/ae3df68e9ba2faeaf76445a7650e4e742eb7b4e7/layout/generic/nsBlockFrame.cpp#93

Comment 2

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

Attachment: containment-and-floats.html

Emilio asked me to provide a testcase exhibiting the need for reconstruction. While developing my patch, attachment 9371961 [details], attachment 9371962 [details] and similar WPT tests contain-layout-dynamic-001.html/contain-paint-dynamic-001.html were failing without a reconstruction in TryToHandleBlockFormattingContextChange but that's no longer the case...

I'm attaching a more complex testcase where floats should be handled by a different float manager when enabling/disabling paint containment but still it seems just forcing a reflow is enough to make it work (without explicitly marking any frames dirty).

Comment 3

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

Attachment: Bug 1874826 - Avoid reconstruction due to change to block formatting context. r=#layout

Comment 4

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

From some preliminary debugging, the frame whose contain value is changed would be marked as dirty here:

https://searchfox.org/mozilla-central/rev/9d88be0557a5bc39d3da288f9aff71414baa303e/layout/base/RestyleManager.cpp#1740

and later in nsBlockFrame::TrialReflow the line it is contained into would be marked dirty too. But I'm not sure why that's enough to trigger the magic of repositioning floats in attachment 9373457 [details].

Comment 5

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

@Emilio: My understanding is that when changing the layout/paint containment of a frame with floats, it should be enough to mark all lines dirty for its parent's float manager (instead of forcing a reconstruction).

However, as I commented above I can't write a proper test for that. How do you think we should proceed here?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

If we switch on the outer frame, then reflowing it should also reflow all lines in the BFC, so I think that's fine.

If we switch on the inner frame, I think we should mark the relevant lines as dirty, and I don't know how you'd have a float that was somehow not intersecting with any of the lines in the old BFC. Let me know if I somehow missed something.

Given that, I'd land the patch without marking the lines as dirty, and a test-case based in comment 2, but do it after the soft freeze (so next week) so that we have some time for fuzzers etc to hit that code. Adding the dirtying should be trivial, should we need to, and hopefully having two full cycles of fuzzing should be enough to find issues before release, should they arise.

Does that sound reasonable?

Comment 7

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

OK I think that makes sense. Will prepare a patch to land for after the code freeze then.

Comment 8

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

Attachment: Bug 1874826 - Add more tests for paint/layout containment change and floats. r=#layout

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a43bc7dfb19
Add more tests for paint/layout containment change and floats. r=layout-reviewers,emilio

Comment 10

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

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

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5a43bc7dfb19

Comment 12

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

Upstream PR merged by moz-wptsync-bot

Comment 13

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

Upstream PR was closed without merging

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1133b5c97587
Avoid reconstruction due to change to block formatting context. r=layout-reviewers,emilio

Comment 15

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/1133b5c97587