Bug 130891 – [CSS Blending] Blending doesn't work if the parent stacking context is not a self painting layer

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

[Ion Rosca]

Blending does not work if its parent, being also the stacking context that isolates blending, is a normal flow only element.

The consequence for software blending is that the parent element cannot create the transparencyLayer because it's not a self painting layer, and for accelerated blending the parent cannot become accelerated for the same reason.

[Ion Rosca]

Created attachment 228032 [details]
failing use case

[Ion Rosca]

Created attachment 228828 [details]
patch #1

[Ion Rosca]

The patch #1 is based on the patch submitted for this bug: https://bugs.webkit.org/show_bug.cgi?id=130892

[Mihnea Ovidenie]

Comment on attachment 228828 [details]
patch #1

View in context: https://bugs.webkit.org/attachment.cgi?id=228828&action=review

> Source/WebCore/ChangeLog:11
> +        The stacking context layers having unisolated blending descendants should be able to create

I guess you are referring to stacking content layers in normal flow here.

> Source/WebCore/ChangeLog:21
> +        There is another affected flag for ancestors (A) of the isolating stacking context layer S:

What is this comment about?

> Source/WebCore/ChangeLog:29
> +        (WebCore::RenderLayer::updateDescendantDependentFlags): turning off the isSelfPaintgLayer flag.

isSelfPaintgLayer -> isSelfPaintingLayer

> LayoutTests/ChangeLog:3
> +        [CSS Blending] Blending doesn't work if the parent stacking context is a normal flow only element

I would like to see tests in which this behaviour- transitioning between normal flow only and self painting layer - is dynamically tested. For the layout tests, please make sure you have the right indentation.

> LayoutTests/css3/compositing/blend-mode-accelerated-parent-overflow-hidden-transparent.html:13
> +		margin-left: 10px;

Why do you need this?

> LayoutTests/css3/compositing/blend-mode-accelerated-parent-overflow-hidden-transparent.html:26
> +<p>This test checks that accelerated blending has no effect when the backdrop is a transparent normal flow only layer.</p>

Please add a line here in which you describe what success looks like when you run the test manually. For instance:
<p>On success you should see a green rectangle below.</p>

> LayoutTests/css3/compositing/blend-mode-parent-overflow-hidden.html:22
> +<p>This test checks that software blending has no effect when the backdrop is a transparent normal flow only layer.</p>

Same as above, please add a note about how success looks like when you manually load this test into a browser.

[Ion Rosca]

Created attachment 230293 [details]
Patch

[Ion Rosca]

Created attachment 230376 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 230376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230376&action=review

> Source/WebCore/rendering/RenderLayer.cpp:825
> +    if (m_hasUnisolatedBlendingDescendantsStatusDirty) {

Do an early return instead of having the whole body indented.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1108
> +#if ENABLE(CSS_COMPOSITING)
> +    layer.updateIsolationForBlendingDescendantsIfNeeded();
> +#endif
>      layer.updateDescendantDependentFlags();

Can layer.updateIsolationForBlendingDescendantsIfNeeded() be rolled into layer.updateDescendantDependentFlags()? That would avoid adding an extra child walk.

> Source/WebCore/rendering/RenderLayerModelObject.cpp:157
> +#if ENABLE(CSS_COMPOSITING)
> +        if (oldStyle->hasBlendMode())
> +            layer()->parent()->dirtyAncestorChainHasBlendingDescendants();
> +#endif

Can't we do this in RenderLayer::styleChanged()?

> LayoutTests/ChangeLog:19
> +        * css3/compositing/blend-mode-isolation-accelerated-overflow-hidden-expected.txt: Added.
> +        * css3/compositing/blend-mode-isolation-accelerated-overflow-hidden.html: Added.
> +        * css3/compositing/blend-mode-isolation-overflow-hidden-expected.html: Added.
> +        * css3/compositing/blend-mode-isolation-overflow-hidden.html: Added.
> +        * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer-expected.txt: Added.
> +        * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer.html: Added.
> +        * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer1-expected.txt: Added.
> +        * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer1.html: Added.
> +        * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer2-expected.txt: Added.
> +        * css3/compositing/blend-mode-isolation-turn-off-self-painting-layer2.html: Added.
> +        * css3/compositing/blend-mode-isolation-turn-on-self-painting-layer-expected.txt: Added.
> +        * css3/compositing/blend-mode-isolation-turn-on-self-painting-layer.html: Added.

Can we make these ref tests please?

[Ion Rosca]

Created attachment 230417 [details]
Patch

[Ion Rosca]

Comment on attachment 230376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230376&action=review

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1108
>>      layer.updateDescendantDependentFlags();
> 
> Can layer.updateIsolationForBlendingDescendantsIfNeeded() be rolled into layer.updateDescendantDependentFlags()? That would avoid adding an extra child walk.

Done. I had to change some code for the other flags, but not the logic.

>> Source/WebCore/rendering/RenderLayerModelObject.cpp:157
>> +#endif
> 
> Can't we do this in RenderLayer::styleChanged()?

On this branch RenderLayer is going to be removed for this RenderObject and RenderLayer::styleChanged will never get called.

>> LayoutTests/ChangeLog:19
>> +        * css3/compositing/blend-mode-isolation-turn-on-self-painting-layer.html: Added.
> 
> Can we make these ref tests please?

It's hard to make ref tests for blending. There is a bug regarding wrong color results: https://bugs.webkit.org/show_bug.cgi?id=130582 and we don't really know what colors to expect. The blending output colors have slightly diferrent  on retina vs non retina displays and among OS versions. I think layer tree is the more appropriate way to test isolation, because isolation flag is also dumped in RenderTreeAsText.

[Ion Rosca]

Comment on attachment 230417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230417&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1129
>                  break;

We can optimize this descendant walk in updateDescendantDependentFlags by leaving the loop if just dirty flags are set, not all of them. Also, we can skip entering updateDescendantDependentFlags() on a child if some conditions are met. For example if we know that a child has blendMode, we are not interested if it has other descendants with blend modes; if a child is self painting layer, we are not interested if it has descendants with that property. I can address this in a follow up patch.

[Dirk Schulze]

Comment on attachment 230417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230417&action=review

The final review should definitely be done by someone with more insight. In general, the patch looks good to me. Some snippets.

> Source/WebCore/ChangeLog:27
> +        When hasUnisolatedBlendingDescendants gets set we make sure that isSelfPaintingLayer flag is set too.

I think it looks better to replace Unisolated with NotIsolated . Seems easier to read.

> Source/WebCore/ChangeLog:34
> +        This change should not affect the logics behind the other flags.

I hope our test coverage is good enough to assume that.

> Source/WebCore/ChangeLog:39
> +        Changing isolatesBlending() to check isStackingContext() first, this will avoid
> +        the assertion in hasUnisolatedBlendingDescendants() for non-stacking context elements.

Wow, there should probably a comment in RenderLayer.h to warn others.

>> Source/WebCore/rendering/RenderLayer.cpp:1129
>>                  break;
> 
> We can optimize this descendant walk in updateDescendantDependentFlags by leaving the loop if just dirty flags are set, not all of them. Also, we can skip entering updateDescendantDependentFlags() on a child if some conditions are met. For example if we know that a child has blendMode, we are not interested if it has other descendants with blend modes; if a child is self painting layer, we are not interested if it has descendants with that property. I can address this in a follow up patch.

Can/Should it be done before this patch then? To be honest, I am not really into the code to judge.


After reading though the code again, I think it is fine to do this in a second patch afterwards.

> Source/WebCore/rendering/RenderLayer.h:805
> +    bool isolatesBlending() const { return isStackingContext() && hasUnisolatedBlendingDescendants(); }

Please put a comment why it is necessary to do that.

[Ion Rosca]

Comment on attachment 230417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230417&action=review

>> Source/WebCore/ChangeLog:27
>> +        When hasUnisolatedBlendingDescendants gets set we make sure that isSelfPaintingLayer flag is set too.
> 
> I think it looks better to replace Unisolated with NotIsolated . Seems easier to read.

Ok.

>> Source/WebCore/ChangeLog:39
>> +        the assertion in hasUnisolatedBlendingDescendants() for non-stacking context elements.
> 
> Wow, there should probably a comment in RenderLayer.h to warn others.

Actually I think we should remove the ASSERT from hasUnisolatedBlendingDescendants() for now. I still hit it in some circumstances and I've heard other people complaining about that too. Asserts have been removed for the other flags as well (see comments in RenderLayer.h: https://bugs.webkit.org/show_bug.cgi?id=71044).
I hit this assertion mostly on minibrowser and in layout tests. On safari flags are updated correctly after changing the style because computeCompositingRequirements calls updateDescendantDependentFlags.

We need a generic mechanism for updating all types of dirty flags on ancestors after the style gets changed and turn the asserts back on. I will remove this change and I'll comment the assert, addressing the issue #71044 with another patch.

[Ion Rosca]

Created attachment 230903 [details]
Patch

[Dean Jackson]

Comment on attachment 230903 [details]
Patch

"LGTM" - but please give smfr a chance to reject my review.

[Simon Fraser (smfr)]

Comment on attachment 230903 [details]
Patch

Looks fine.

[WebKit Commit Bot]

Comment on attachment 230903 [details]
Patch

Clearing flags on attachment: 230903

Committed r168462: <http://trac.webkit.org/changeset/168462>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.