Bug 130891

Summary: [CSS Blending] Blending doesn't work if the parent stacking context is not a self painting layer
Product: WebKit Reporter: Ion Rosca <rosca>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kondapallykalyan, simon.fraser, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 95614    
Attachments:
Description Flags
failing use case
none
patch #1
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ion Rosca 2014-03-28 02:42:22 PDT
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.
Comment 1 Ion Rosca 2014-03-28 02:46:53 PDT
Created attachment 228032 [details]
failing use case
Comment 2 Ion Rosca 2014-04-08 04:38:26 PDT
Created attachment 228828 [details]
patch #1
Comment 3 Ion Rosca 2014-04-08 04:39:49 PDT
The patch #1 is based on the patch submitted for this bug: https://bugs.webkit.org/show_bug.cgi?id=130892
Comment 4 Mihnea Ovidenie 2014-04-17 04:00:38 PDT
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.
Comment 5 Ion Rosca 2014-04-28 05:30:47 PDT
Created attachment 230293 [details]
Patch
Comment 6 Ion Rosca 2014-04-29 07:29:32 PDT
Created attachment 230376 [details]
Patch
Comment 7 Simon Fraser (smfr) 2014-04-29 11:20:03 PDT
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?
Comment 8 Ion Rosca 2014-04-29 14:22:08 PDT
Created attachment 230417 [details]
Patch
Comment 9 Ion Rosca 2014-04-29 14:34:34 PDT
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.
Comment 10 Ion Rosca 2014-04-29 14:57:32 PDT
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.
Comment 11 Dirk Schulze 2014-05-06 01:25:48 PDT
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.
Comment 12 Ion Rosca 2014-05-06 04:30:15 PDT
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.
Comment 13 Ion Rosca 2014-05-06 07:03:16 PDT
Created attachment 230903 [details]
Patch
Comment 14 Dean Jackson 2014-05-07 13:58:51 PDT
Comment on attachment 230903 [details]
Patch

"LGTM" - but please give smfr a chance to reject my review.
Comment 15 Simon Fraser (smfr) 2014-05-07 14:01:03 PDT
Comment on attachment 230903 [details]
Patch

Looks fine.
Comment 16 WebKit Commit Bot 2014-05-07 22:42:50 PDT
Comment on attachment 230903 [details]
Patch

Clearing flags on attachment: 230903

Committed r168462: <http://trac.webkit.org/changeset/168462>
Comment 17 WebKit Commit Bot 2014-05-07 22:42:55 PDT
All reviewed patches have been landed.  Closing bug.