Bug 224742 – [css-contain] Support contain:paint

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

Support contain:paint.

[Rob Buis]

Created attachment 426401 [details]
Patch

[Rob Buis]

Created attachment 426677 [details]
Patch

[Rob Buis]

Created attachment 426694 [details]
Patch

[Rob Buis]

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?

[Radar WebKit Bug Importer]

<rdar://problem/77143622>

[Rob Buis]

Created attachment 427421 [details]
Patch

[Rob Buis]

Created attachment 428168 [details]
Patch

[Darin Adler]

Comment on attachment 428168 [details]
Patch

Does this need a feature flag?

[Darin Adler]

(In reply to Darin Adler from comment #8)
> Does this need a feature flag?

Or is it already covered by an existing feature flag?

[Rob Buis]

Created attachment 428269 [details]
Patch

[Rob Buis]

Created attachment 428275 [details]
Patch

[Rob Buis]

Created attachment 428290 [details]
Patch

[Rob Buis]

(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.

[Darin Adler]

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.

[Rob Buis]

Created attachment 428829 [details]
Patch

[Rob Buis]

(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 :-)

[Rob Buis]

Created attachment 431253 [details]
Patch

[Rob Buis]

Created attachment 431264 [details]
Patch

[Rob Buis]

Created attachment 431272 [details]
Patch

[Rob Buis]

Created attachment 431273 [details]
Patch

[Rob Buis]

Created attachment 431275 [details]
Patch

[Rob Buis]

Created attachment 431280 [details]
Patch

[Rob Buis]

Created attachment 431283 [details]
Patch

[Sergio Villar Senin]

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.

[Rob Buis]

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.

[Rob Buis]

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

[Rob Buis]

Created attachment 431542 [details]
Patch

[Rob Buis]

Comment on attachment 431542 [details]
Patch

Patch has some improvements now and I removed the unrelated cmake changes.

[Rob Buis]

Created attachment 433212 [details]
Patch

[Rob Buis]

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

[Rob Buis]

Comment on attachment 433212 [details]
Patch

overflow:clip should go in first, then this can be rebased.

[Rob Buis]

Created attachment 434695 [details]
Patch

[Rob Buis]

Created attachment 434892 [details]
Patch

[Rob Buis]

Created attachment 435185 [details]
Patch

[Rob Buis]

Created attachment 435336 [details]
Patch

[Rob Buis]

Created attachment 437410 [details]
Patch

[Rob Buis]

Created attachment 437851 [details]
Patch

[Rob Buis]

Created attachment 438253 [details]
Patch

[Rob Buis]

Created attachment 438331 [details]
Patch

[Rob Buis]

Created attachment 439604 [details]
Patch

[Brent Fulgham]

Is this ready for review?

[Rob Buis]

(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).

[Simon Fraser (smfr)]

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

[Rob Buis]

Created attachment 442373 [details]
Patch

[Rob Buis]

(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.

[alan baradlay]

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.

[Rob Buis]

Created attachment 443783 [details]
Patch

[Rob Buis]

Created attachment 443791 [details]
Patch

[Rob Buis]

Created attachment 443802 [details]
Patch

[Rob Buis]

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.

[EWS]

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].

[Ryan Haddad]

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

[Ryan Haddad]

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>

[Ryan Haddad]

(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.

[Rob Buis]

Created attachment 444364 [details]
Patch

[Rob Buis]

(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!

[Rob Buis]

Created attachment 444389 [details]
Patch

[Rob Buis]

@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.

[EWS]

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].