[css-contain] Paint containment shouldn't create a stacking context for elements it doesn't apply |
|||||||
Issue descriptionCheck the attached example, it shows the difference between a regular div element vs a ruby-text element. Paint containment applies on the regular for that reason it creates a stacking context. However it doesn't apply on ruby-text elements, however it still creates the stacking context. The problem is the code at ComputedStyle::UpdateIsStackingContext() is using directly "ContainsPaint()" which only checks that the element has "contain: paint". We should use LayoutObject::ShouldApplyPaintContainment() instead which is the one in charge of checking if paint containment should apply or not to the element.
,
Aug 1
I was taking a look to this as I thought it could be a simple fix, but it's not that simple for me. :-/ This is a CL of what I tried but doesn't work, I'm uploading it as the tests in the CL should be valid anyway: https://chromium-review.googlesource.com/c/chromium/src/+/1158365 As you can see in that patch I'm removing the ContainsPaint() call in ComputedStyle::UpdateIsStackingContext(). And I'm adding new code in LayoutTreeBuilderForElement::CreateLayoutObject() to set IsStackingContext to true if the element has paint containment. But it seems that's too late and the stacking context is not created. The problem is that we cannot determine if the element creates or not a stacking context without having the LayoutObject. For example an element with "display: inline" won't have paint containment unless it's blockified (for example if it's a flex or grid item) in that case paint containment would apply to it. I don't know a lot about all this stacking context stuff, so if anyone has ideas or suggestions about how to fix this (or know the people with knowledge in this area) please tell me. If someone wants to take this and fix it by themselves that's also fine with me.
,
Aug 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dbc976dff2f1d80d5244de1efa91c4fdd5815066 commit dbc976dff2f1d80d5244de1efa91c4fdd5815066 Author: Manuel Rego Casasnovas <rego@igalia.com> Date: Mon Aug 06 15:29:29 2018 [css-contain] Link failures in WPT tests to the related bugs Two of the failures are related to stacking contexts: * external/wpt/css/css-contain/contain-paint-021.html * external/wpt/css/css-contain/contain-layout-016.html The other one was a mistake that has been already fixed in WPT (see https://github.com/web-platform-tests/wpt/pull/12314): * external/wpt/css/css-contain/contain-paint-001.html TBR=eae@chromium.org BUG= 870811 , 868102 , 870157 Change-Id: I60e89decd2f939d1d9a850a1b8b556d31658e872 Reviewed-on: https://chromium-review.googlesource.com/1163673 Reviewed-by: Manuel Rego <rego@igalia.com> Commit-Queue: Manuel Rego <rego@igalia.com> Cr-Commit-Position: refs/heads/master@{#580878} [modify] https://crrev.com/dbc976dff2f1d80d5244de1efa91c4fdd5815066/third_party/WebKit/LayoutTests/TestExpectations
,
Aug 6
The tests are now in WPT so I'm abandoning my CL. I'll be on holidays until September starting next week, so I don't think I'll have time to fix these before holidays.
,
Aug 10
,
Aug 10
,
Aug 10
Rune would you mind giving Rego some help with this? Then he can be unblocked also on crbug.com/870157 .
,
Aug 13
,
Aug 13
The test-case uses ruby-text which is a display-inside value in css3-display which we don't support, so it will not pass unless we start implementing that.
,
Aug 13
They're defined in https://drafts.csswg.org/css-ruby/#ruby-display as well. The ruby elements in the HTML spec are styled in the UA sheet with these values and Edge and Firefox supports them. We probably should too.
,
Sep 5
Ok, I didn't realize about that. :-( So I've reported a different bug for that particular topic: issue #880802. Regarding this issue, I've created a different test using "display: inline" in WPT, which works as expected in Chromium: https://github.com/web-platform-tests/wpt/pull/12847 I couldn't find any failing case so far, so I guess current code is good enough and we can close this issue. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by r...@igalia.com
, Jul 26509 bytes
509 bytes View Download