Bug 224742

Created attachment 426401 [details] Patch

Created attachment 426677 [details] Patch

Created attachment 426694 [details] Patch

After some experimentation (see the patch), almost all contain: paint tests pass, but I think there are two areas to further figure out: - setting up z-index in StyleAdjuster is not ideal, since some elements with contain: paint on them should ignore it (for example it depends on whether the element is replaced), but we can't do that in StyleAdjuster since it needs the renderer to find out if it is replaced. Same problem applies to contain: layout but I guess no test hits it. Current workaround is to check in RenderLayer whether we need a stacking context. - the patch forces display: hidden and position: relative (for default position) internally. This does what contain: paint requires, but I wonder if we should instead do more contain: paint checks at the place where style.overflowX()/overflowY() and style.position() are used. However I suspect that is a lot of places and I am not sure how many we'd need to change and where they all are. Simon, do you have some suggestions for above?

<rdar://problem/77143622>

Created attachment 427421 [details] Patch

Created attachment 428168 [details] Patch

Comment on attachment 428168 [details] Patch Does this need a feature flag?

(In reply to Darin Adler from comment #8) > Does this need a feature flag? Or is it already covered by an existing feature flag?

Created attachment 428269 [details] Patch

Created attachment 428275 [details] Patch

Created attachment 428290 [details] Patch

(In reply to Darin Adler from comment #9) > (In reply to Darin Adler from comment #8) > > Does this need a feature flag? > > Or is it already covered by an existing feature flag? Right, all of CSS Containment depends on CSSContainmentEnabled experimental feature; if this is not enabled (and it is not by default), parsing is not done and checks like shouldApplyPaintContainment will always fail.

Comment on attachment 428290 [details] Patch I’d love to review, but I don’t think I *quite* have enough knowledge to spot a mistake.

Created attachment 428829 [details] Patch

(In reply to Darin Adler from comment #14) > Comment on attachment 428290 [details] > Patch > > I’d love to review, but I don’t think I *quite* have enough knowledge to > spot a mistake. Understood Darin, thanks for letting me know. In the meantime I rebased this patch after bug 225442 landed, for any other takers :-)

Created attachment 431253 [details] Patch

Created attachment 431264 [details] Patch

Created attachment 431272 [details] Patch

Created attachment 431273 [details] Patch

Created attachment 431275 [details] Patch

Created attachment 431280 [details] Patch

Created attachment 431283 [details] Patch

Comment on attachment 431283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431283&action=review Looking great! I'll let Simon et al. to check it before approving. > Source/WebCore/ChangeLog:15 > + - implements clipping on the overflow clip edge. Nit: no need to wrap those comments that much. Long lines are welcomed > Source/cmake/OptionsGTK.cmake:73 > +WEBKIT_OPTION_DEFINE(USE_AVIF "Whether to enable support for AVIF images." PUBLIC OFF) Unrelated > Source/cmake/OptionsGTK.cmake:81 > +WEBKIT_OPTION_DEFINE(USE_SOUP2 "Whether to enable usage of Soup 2 instead of Soup 3." PUBLIC ON) Ditto.

Comment on attachment 431283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431283&action=review >> Source/cmake/OptionsGTK.cmake:73 >> +WEBKIT_OPTION_DEFINE(USE_AVIF "Whether to enable support for AVIF images." PUBLIC OFF) > > Unrelated You can usually ignore patches I do not put up for review :) I am usually experimenting and abusing the build bots to make sure all is green.

I found out a few things since Friday: - overflow: clip is not supported sadly. However I think overflow: hidden is reasonably close. - while actually looking at the spec a bit better, it turns our contain: paint *only* has an effect for overflow-x/y: visible [1]. This means a lot of checks (like overflowX() == Overflow::Scroll) can be done without taking contain: paint into account and not using effectiveOverflowX/Y. Really only comparison with Visible and Hidden need effectiveOverflowX/Y. - overflowX/Y is queried in a lot of different context, for example clipping and scroll overflow, and I do not know whether contain: paint should be queried tooin all those contexts (through effectiveOverflowX/Y), actually I only know sure in the clipping case. For example I have no idea if spatial navigation would need to take it into account. [1] https://drafts.csswg.org/css-contain-2/#containment-paint

Created attachment 431542 [details] Patch

Comment on attachment 431542 [details] Patch Patch has some improvements now and I removed the unrelated cmake changes.

Created attachment 433212 [details] Patch

Implementing overflow: clip first (bug 198230), hopefully that will make this patch a bit smaller and easier.

Comment on attachment 433212 [details] Patch overflow:clip should go in first, then this can be rebased.

Created attachment 434695 [details] Patch

Created attachment 434892 [details] Patch

Created attachment 435185 [details] Patch

Created attachment 435336 [details] Patch

Created attachment 437410 [details] Patch

Created attachment 437851 [details] Patch

Created attachment 438253 [details] Patch

Created attachment 438331 [details] Patch

Created attachment 439604 [details] Patch

Is this ready for review?

(In reply to Brent Fulgham from comment #41) > Is this ready for review? A technical review is always welcome, however we still feel we need more data from perf tests to check where the patch improves things (tracked in https://bugs.webkit.org/show_bug.cgi?id=228696).

It seems fine to separate the behavioral changes (affecting WPT) to the perf changes, which can be iterative.

Created attachment 442373 [details] Patch

(In reply to Simon Fraser (smfr) from comment #43) > It seems fine to separate the behavioral changes (affecting WPT) to the perf > changes, which can be iterative. Nice! I rebased the patch (after some of contain: style landed) and will put it up for review.

Comment on attachment 442373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442373&action=review > Source/WebCore/rendering/RenderElement.h:222 > + bool paintContainmentApplies() const { return m_paintContainmentApplies; } > + void setPaintContainmentApplies(bool b) { m_paintContainmentApplies = b; } Any particular reason for caching this values? if so, please use rare data.

Created attachment 443783 [details] Patch

Created attachment 443791 [details] Patch

Created attachment 443802 [details] Patch

Comment on attachment 442373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442373&action=review >> Source/WebCore/rendering/RenderElement.h:222 >> + void setPaintContainmentApplies(bool b) { m_paintContainmentApplies = b; } > > Any particular reason for caching this values? if so, please use rare data. They will be queried a lot because of effictiveOverflow getters. I ended up using rare data indeed, but is there really a lack of bits? It seems there are 5 in RenderElement and 1 or 2 in RenderObject.

Committed r285583 (244091@main): <https://commits.webkit.org/244091@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443802 [details].

We've seen a large number of layout tests become flaky failures right after this change has landed: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=fast%2Ftable%2Foverflow-table-collapsed-borders-section-self-painting-layer-painting.html&test=fast%2Ftable%2Foverflow-table-collapsed-borders-section-self-painting-layer-table-self-painting-layer.html&test=editing%2FexecCommand%2F4580583-2.html&test=editing%2FexecCommand%2F4747450.html&test=fast%2Fcss%2Foutline-auto-empty-rects.html&test=editing%2Finserting%2Finsert-br-quoted-005.html I can reproduce one of the failures locally with a ToT build with the following: run-webkit-tests fast/table/overflow-table-collapsed-borders-section-self-painting-layer-table-self-painting-layer.html --iterations 100 --child-processes 1 --exit-after-n-failures 1 I cannot reproduce the failure with r285577. None of the other changes in this range look like they could be related: https://trac.webkit.org/log/webkit/?action=stop_on_copy&mode=stop_on_copy&rev=285583&stop_rev=285577&limit=100&verbose=on

Reverted r285583 for reason: Seems to have caused many layout tests to become flaky failures Committed r285745 (244204@main): <https://commits.webkit.org/244204@main>

(In reply to Ryan Haddad from comment #52) > We've seen a large number of layout tests become flaky failures right after > this change has landed: > > https://results.webkit.org/?suite=layout-tests&suite=layout- > tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout- > tests&test=fast%2Ftable%2Foverflow-table-collapsed-borders-section-self- > painting-layer-painting.html&test=fast%2Ftable%2Foverflow-table-collapsed- > borders-section-self-painting-layer-table-self-painting-layer. > html&test=editing%2FexecCommand%2F4580583-2. > html&test=editing%2FexecCommand%2F4747450.html&test=fast%2Fcss%2Foutline- > auto-empty-rects.html&test=editing%2Finserting%2Finsert-br-quoted-005.html Looks like the flakiness has stopped after the revert.

Created attachment 444364 [details] Patch

(In reply to Ryan Haddad from comment #52) > We've seen a large number of layout tests become flaky failures right after > this change has landed: > > https://results.webkit.org/?suite=layout-tests&suite=layout- > tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout- > tests&test=fast%2Ftable%2Foverflow-table-collapsed-borders-section-self- > painting-layer-painting.html&test=fast%2Ftable%2Foverflow-table-collapsed- > borders-section-self-painting-layer-table-self-painting-layer. > html&test=editing%2FexecCommand%2F4580583-2. > html&test=editing%2FexecCommand%2F4747450.html&test=fast%2Fcss%2Foutline- > auto-empty-rects.html&test=editing%2Finserting%2Finsert-br-quoted-005.html > > I can reproduce one of the failures locally with a ToT build with the > following: > run-webkit-tests > fast/table/overflow-table-collapsed-borders-section-self-painting-layer- > table-self-painting-layer.html --iterations 100 --child-processes 1 > --exit-after-n-failures 1 It turns out this is caused by uninitialized rare data. I guess this is something that can't always be caught by the build bots due to system differences. Tricky!

Created attachment 444389 [details] Patch

@Alan this is ready for review again, see comment 56 for the cause. Besides the uninitialized member var fix in RenderObject.cpp, the only other change is skipping one more test because of https://bugs.webkit.org/show_bug.cgi?id=233193.

Committed r285885 (244309@main): <https://commits.webkit.org/244309@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444389 [details].