RESOLVED FIXED 224742
[css-contain] Support contain:paint
https://bugs.webkit.org/show_bug.cgi?id=224742
Summary [css-contain] Support contain:paint
Rob Buis
Reported 2021-04-19 02:06:19 PDT
Support contain:paint.
Attachments
Patch (10.83 KB, patch)
2021-04-19 02:09 PDT, Rob Buis
no flags
Patch (14.33 KB, patch)
2021-04-21 06:35 PDT, Rob Buis
no flags
Patch (14.66 KB, patch)
2021-04-21 08:36 PDT, Rob Buis
no flags
Patch (15.52 KB, patch)
2021-04-30 08:14 PDT, Rob Buis
no flags
Patch (16.19 KB, patch)
2021-05-10 07:08 PDT, Rob Buis
no flags
Patch (16.36 KB, patch)
2021-05-11 06:20 PDT, Rob Buis
no flags
Patch (16.45 KB, patch)
2021-05-11 07:50 PDT, Rob Buis
no flags
Patch (19.64 KB, patch)
2021-05-11 10:12 PDT, Rob Buis
no flags
Patch (16.95 KB, patch)
2021-05-17 08:19 PDT, Rob Buis
no flags
Patch (17.80 KB, patch)
2021-06-12 00:32 PDT, Rob Buis
no flags
Patch (26.43 KB, patch)
2021-06-12 12:43 PDT, Rob Buis
no flags
Patch (22.32 KB, patch)
2021-06-13 03:49 PDT, Rob Buis
ews-feeder: commit-queue-
Patch (22.29 KB, patch)
2021-06-13 04:27 PDT, Rob Buis
no flags
Patch (22.32 KB, patch)
2021-06-13 07:09 PDT, Rob Buis
ews-feeder: commit-queue-
Patch (26.05 KB, patch)
2021-06-13 09:56 PDT, Rob Buis
no flags
Patch (30.34 KB, patch)
2021-06-13 11:37 PDT, Rob Buis
no flags
Patch (32.35 KB, patch)
2021-06-16 07:04 PDT, Rob Buis
no flags
Patch (33.49 KB, patch)
2021-07-09 05:56 PDT, Rob Buis
no flags
Patch (33.32 KB, patch)
2021-07-31 01:35 PDT, Rob Buis
no flags
Patch (34.39 KB, patch)
2021-08-04 01:41 PDT, Rob Buis
no flags
Patch (34.39 KB, patch)
2021-08-09 05:57 PDT, Rob Buis
no flags
Patch (34.53 KB, patch)
2021-08-11 05:20 PDT, Rob Buis
no flags
Patch (34.70 KB, patch)
2021-09-06 06:52 PDT, Rob Buis
no flags
Patch (35.80 KB, patch)
2021-09-10 04:35 PDT, Rob Buis
no flags
Patch (37.88 KB, patch)
2021-09-15 10:15 PDT, Rob Buis
no flags
Patch (37.29 KB, patch)
2021-09-16 03:22 PDT, Rob Buis
no flags
Patch (36.11 KB, patch)
2021-09-29 07:01 PDT, Rob Buis
no flags
Patch (36.60 KB, patch)
2021-10-25 08:22 PDT, Rob Buis
no flags
Patch (36.16 KB, patch)
2021-11-10 01:31 PST, Rob Buis
no flags
Patch (36.83 KB, patch)
2021-11-10 02:57 PST, Rob Buis
no flags
Patch (36.57 KB, patch)
2021-11-10 04:19 PST, Rob Buis
no flags
Patch (36.90 KB, patch)
2021-11-16 02:46 PST, Rob Buis
ews-feeder: commit-queue-
Patch (37.00 KB, patch)
2021-11-16 08:21 PST, Rob Buis
no flags
Rob Buis
Comment 1 2021-04-19 02:09:46 PDT
Rob Buis
Comment 2 2021-04-21 06:35:07 PDT
Rob Buis
Comment 3 2021-04-21 08:36:33 PDT
Rob Buis
Comment 4 2021-04-21 12:59:04 PDT
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
Comment 5 2021-04-26 02:07:14 PDT
Rob Buis
Comment 6 2021-04-30 08:14:08 PDT
Rob Buis
Comment 7 2021-05-10 07:08:58 PDT
Darin Adler
Comment 8 2021-05-10 14:19:28 PDT
Comment on attachment 428168 [details] Patch Does this need a feature flag?
Darin Adler
Comment 9 2021-05-10 14:32:42 PDT
(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
Comment 10 2021-05-11 06:20:52 PDT
Rob Buis
Comment 11 2021-05-11 07:50:15 PDT
Rob Buis
Comment 12 2021-05-11 10:12:09 PDT
Rob Buis
Comment 13 2021-05-11 13:56:11 PDT
(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 14 2021-05-14 15:14:07 PDT
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
Comment 15 2021-05-17 08:19:22 PDT
Rob Buis
Comment 16 2021-05-17 11:00:09 PDT
(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
Comment 17 2021-06-12 00:32:26 PDT
Rob Buis
Comment 18 2021-06-12 12:43:50 PDT
Rob Buis
Comment 19 2021-06-13 03:49:15 PDT
Rob Buis
Comment 20 2021-06-13 04:27:00 PDT
Rob Buis
Comment 21 2021-06-13 07:09:32 PDT
Rob Buis
Comment 22 2021-06-13 09:56:22 PDT
Rob Buis
Comment 23 2021-06-13 11:37:02 PDT
Sergio Villar Senin
Comment 24 2021-06-14 09:50:23 PDT
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 25 2021-06-14 11:54:26 PDT
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
Comment 26 2021-06-14 12:04:23 PDT
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
Comment 27 2021-06-16 07:04:37 PDT
Rob Buis
Comment 28 2021-06-16 09:25:28 PDT
Comment on attachment 431542 [details] Patch Patch has some improvements now and I removed the unrelated cmake changes.
Rob Buis
Comment 29 2021-07-09 05:56:43 PDT
Rob Buis
Comment 30 2021-07-14 12:23:32 PDT
Implementing overflow: clip first (bug 198230), hopefully that will make this patch a bit smaller and easier.
Rob Buis
Comment 31 2021-07-26 01:40:59 PDT
Comment on attachment 433212 [details] Patch overflow:clip should go in first, then this can be rebased.
Rob Buis
Comment 32 2021-07-31 01:35:26 PDT
Rob Buis
Comment 33 2021-08-04 01:41:29 PDT
Rob Buis
Comment 34 2021-08-09 05:57:40 PDT
Rob Buis
Comment 35 2021-08-11 05:20:02 PDT
Rob Buis
Comment 36 2021-09-06 06:52:10 PDT
Rob Buis
Comment 37 2021-09-10 04:35:54 PDT
Rob Buis
Comment 38 2021-09-15 10:15:46 PDT
Rob Buis
Comment 39 2021-09-16 03:22:58 PDT
Rob Buis
Comment 40 2021-09-29 07:01:04 PDT
Brent Fulgham
Comment 41 2021-10-19 11:14:13 PDT
Is this ready for review?
Rob Buis
Comment 42 2021-10-21 07:37:53 PDT
(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)
Comment 43 2021-10-21 09:16:59 PDT
It seems fine to separate the behavioral changes (affecting WPT) to the perf changes, which can be iterative.
Rob Buis
Comment 44 2021-10-25 08:22:12 PDT
Rob Buis
Comment 45 2021-10-25 10:12:02 PDT
(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 46 2021-11-09 14:51:51 PST
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
Comment 47 2021-11-10 01:31:19 PST
Rob Buis
Comment 48 2021-11-10 02:57:53 PST
Rob Buis
Comment 49 2021-11-10 04:19:06 PST
Rob Buis
Comment 50 2021-11-10 08:58:34 PST
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
Comment 51 2021-11-10 09:24:51 PST
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
Comment 52 2021-11-12 14:57:16 PST
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
Comment 53 2021-11-12 15:00:50 PST
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
Comment 54 2021-11-12 21:10:16 PST
(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
Comment 55 2021-11-16 02:46:03 PST
Rob Buis
Comment 56 2021-11-16 02:48:55 PST
(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
Comment 57 2021-11-16 08:21:21 PST
Rob Buis
Comment 58 2021-11-16 09:13:50 PST
@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
Comment 59 2021-11-16 14:32:47 PST
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].
Note You need to log in before you can comment on or make changes to this bug.