WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(14.33 KB, patch)
2021-04-21 06:35 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(14.66 KB, patch)
2021-04-21 08:36 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(15.52 KB, patch)
2021-04-30 08:14 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(16.19 KB, patch)
2021-05-10 07:08 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(16.36 KB, patch)
2021-05-11 06:20 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(16.45 KB, patch)
2021-05-11 07:50 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(19.64 KB, patch)
2021-05-11 10:12 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(16.95 KB, patch)
2021-05-17 08:19 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(17.80 KB, patch)
2021-06-12 00:32 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(26.43 KB, patch)
2021-06-12 12:43 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(22.32 KB, patch)
2021-06-13 03:49 PDT
,
Rob Buis
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(22.29 KB, patch)
2021-06-13 04:27 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(22.32 KB, patch)
2021-06-13 07:09 PDT
,
Rob Buis
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(26.05 KB, patch)
2021-06-13 09:56 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(30.34 KB, patch)
2021-06-13 11:37 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(32.35 KB, patch)
2021-06-16 07:04 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(33.49 KB, patch)
2021-07-09 05:56 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(33.32 KB, patch)
2021-07-31 01:35 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(34.39 KB, patch)
2021-08-04 01:41 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(34.39 KB, patch)
2021-08-09 05:57 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(34.53 KB, patch)
2021-08-11 05:20 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(34.70 KB, patch)
2021-09-06 06:52 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(35.80 KB, patch)
2021-09-10 04:35 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(37.88 KB, patch)
2021-09-15 10:15 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(37.29 KB, patch)
2021-09-16 03:22 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(36.11 KB, patch)
2021-09-29 07:01 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(36.60 KB, patch)
2021-10-25 08:22 PDT
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(36.16 KB, patch)
2021-11-10 01:31 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(36.83 KB, patch)
2021-11-10 02:57 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(36.57 KB, patch)
2021-11-10 04:19 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Patch
(36.90 KB, patch)
2021-11-16 02:46 PST
,
Rob Buis
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(37.00 KB, patch)
2021-11-16 08:21 PST
,
Rob Buis
no flags
Details
Formatted Diff
Diff
Show Obsolete
(32)
View All
Add attachment
proposed patch, testcase, etc.
Rob Buis
Comment 1
2021-04-19 02:09:46 PDT
Created
attachment 426401
[details]
Patch
Rob Buis
Comment 2
2021-04-21 06:35:07 PDT
Created
attachment 426677
[details]
Patch
Rob Buis
Comment 3
2021-04-21 08:36:33 PDT
Created
attachment 426694
[details]
Patch
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
<
rdar://problem/77143622
>
Rob Buis
Comment 6
2021-04-30 08:14:08 PDT
Created
attachment 427421
[details]
Patch
Rob Buis
Comment 7
2021-05-10 07:08:58 PDT
Created
attachment 428168
[details]
Patch
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
Created
attachment 428269
[details]
Patch
Rob Buis
Comment 11
2021-05-11 07:50:15 PDT
Created
attachment 428275
[details]
Patch
Rob Buis
Comment 12
2021-05-11 10:12:09 PDT
Created
attachment 428290
[details]
Patch
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
Created
attachment 428829
[details]
Patch
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
Created
attachment 431253
[details]
Patch
Rob Buis
Comment 18
2021-06-12 12:43:50 PDT
Created
attachment 431264
[details]
Patch
Rob Buis
Comment 19
2021-06-13 03:49:15 PDT
Created
attachment 431272
[details]
Patch
Rob Buis
Comment 20
2021-06-13 04:27:00 PDT
Created
attachment 431273
[details]
Patch
Rob Buis
Comment 21
2021-06-13 07:09:32 PDT
Created
attachment 431275
[details]
Patch
Rob Buis
Comment 22
2021-06-13 09:56:22 PDT
Created
attachment 431280
[details]
Patch
Rob Buis
Comment 23
2021-06-13 11:37:02 PDT
Created
attachment 431283
[details]
Patch
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
Created
attachment 431542
[details]
Patch
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
Created
attachment 433212
[details]
Patch
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
Created
attachment 434695
[details]
Patch
Rob Buis
Comment 33
2021-08-04 01:41:29 PDT
Created
attachment 434892
[details]
Patch
Rob Buis
Comment 34
2021-08-09 05:57:40 PDT
Created
attachment 435185
[details]
Patch
Rob Buis
Comment 35
2021-08-11 05:20:02 PDT
Created
attachment 435336
[details]
Patch
Rob Buis
Comment 36
2021-09-06 06:52:10 PDT
Created
attachment 437410
[details]
Patch
Rob Buis
Comment 37
2021-09-10 04:35:54 PDT
Created
attachment 437851
[details]
Patch
Rob Buis
Comment 38
2021-09-15 10:15:46 PDT
Created
attachment 438253
[details]
Patch
Rob Buis
Comment 39
2021-09-16 03:22:58 PDT
Created
attachment 438331
[details]
Patch
Rob Buis
Comment 40
2021-09-29 07:01:04 PDT
Created
attachment 439604
[details]
Patch
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
Created
attachment 442373
[details]
Patch
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
Created
attachment 443783
[details]
Patch
Rob Buis
Comment 48
2021-11-10 02:57:53 PST
Created
attachment 443791
[details]
Patch
Rob Buis
Comment 49
2021-11-10 04:19:06 PST
Created
attachment 443802
[details]
Patch
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
Created
attachment 444364
[details]
Patch
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
Created
attachment 444389
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug