Bug 130892 – [CSS Blending] Isolation descendant dependent flags are not updated correctly

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

[Ion Rosca]

There are some situations when Isolation descendant dependent flags might not be computed correctly.
Some problems I noticed:
1) The isolation flags should be computed for accelerated layers too. An accelerated layer might be the isolation root for some software blending, so it needs this information.
2) When adding or removing elements from dom, or when changing style, we should not count on checking the compositing state of the element, cause it might get changed due to the new style.

We should add test for all the basic operations. Possible DOM configuration I can think of now:

* S - B|E - E - B
* S - E|B - E - B
* S - E +-  S - B
* S - E +- E - B
* S - E /- S - B
* S - E /- E - B
* S - S|E - E - B
* S - E|S - E - B
* S - S - E - B
           - E - B/E

where:
S is an element being a stacking context
E is a regular element
B is an element having blending
- is a parent-child relationship
| means changing element type dynamically (left is the initial type, right is the changed type)
+ means adding the right side element chain dynamically with JS
/ means removing the element dynamically

[Ion Rosca]

Created attachment 228034 [details]
not final patch

[Ion Rosca]

Created attachment 228162 [details]
not for review

[Build Bot]

Comment on attachment 228162 [details]
not for review

Attachment 228162 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5572916125106176

New failing tests:
css3/compositing/blend-mode-background.html
css3/compositing/blend-mode-simple-composited.html
css3/compositing/blend-mode-isolated-group-1.html
css3/compositing/blend-mode-isolated-group-3.html
css3/compositing/blend-mode-layers.html
css3/compositing/blend-mode-reflection.html
css3/compositing/blend-mode-isolated-group-2.html
css3/compositing/blend-mode-simple.html

[Build Bot]

Created attachment 228169 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 228162 [details]
not for review

Attachment 228162 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6597146639859712

New failing tests:
css3/compositing/blend-mode-background.html
css3/compositing/blend-mode-simple-composited.html
css3/compositing/blend-mode-isolated-group-1.html
css3/compositing/blend-mode-isolated-group-3.html
css3/compositing/blend-mode-layers.html
css3/compositing/blend-mode-reflection.html
css3/compositing/blend-mode-isolated-group-2.html
css3/compositing/blend-mode-simple.html

[Build Bot]

Created attachment 228173 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 228162 [details]
not for review

Attachment 228162 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5675030583181312

New failing tests:
css3/compositing/blend-mode-background.html
css3/compositing/blend-mode-simple-composited.html
css3/compositing/blend-mode-isolated-group-1.html
css3/compositing/blend-mode-isolated-group-3.html
css3/compositing/blend-mode-layers.html
css3/compositing/blend-mode-reflection.html
css3/compositing/blend-mode-isolated-group-2.html
css3/compositing/blend-mode-simple.html

[Build Bot]

Created attachment 228174 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Ion Rosca]

Created attachment 228383 [details]
adding tests and ifdefs

[Ion Rosca]

Created attachment 228385 [details]
nit

[Mihnea Ovidenie]

Comment on attachment 228385 [details]
nit

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

> Source/WebCore/ChangeLog:10
> +

You should probably mention that you renamed:
m_hasBlendedElementInChildStackingContext => m_hasUnisolatedBlendingDescendants
m_hasBlendedElementInChildStackingContextStatusDirty => m_hasUnisolatedBlendingDescendantsStatusDirty

> Source/WebCore/rendering/RenderLayer.cpp:190
> +    , m_hasUnisolatedBlendingDescendants(false)

With this patch, you have 2 flags tracking isolation: m_hasUnisolatedCompositedBlendingDescendants and m_hasUnisolatedBlendingDescendants. I suggest you should add some assertions checking that if the first is true the second should also be true.

> Source/WebCore/rendering/RenderLayer.cpp:-825
> -    if (isComposited())

Why did you remove this check?

> Source/WebCore/rendering/RenderLayer.cpp:1099
> +#endif

I would rewrite the code above to be wrapped in ENABLE(CSS_COMPOSITING):
#if ENABLE(CSS_COMPOSITING)
bool hasUnisolatedBlendingDescendants = false;
#endif

> Source/WebCore/rendering/RenderLayer.cpp:1145
> +#endif

Can these lines be moved above, before:
if (m_hasVisibleDescendant && m_hasSelfPaintingLayerDescendant && m_hasOutOfFlowPositionedDescendant && hasUnisolatedBlendingDescendants)

If so, in the if clause you can use the function you added instead of testing the variable hasUnisolatedBlendingDescendants.

> Source/WebCore/rendering/RenderLayer.cpp:6432
> +        if (isStackingContext && parent()) {

1. When is parent() null here?
2. You can rewrite to smth like:
if (parent()) {
    if (isstackingContent) { ... }
    else { ... }
}

[Ion Rosca]

Created attachment 228491 [details]
addressing Mihnea's comments

[Ion Rosca]

Created attachment 228494 [details]
addressing Mihnea's comments 2

[Ion Rosca]

Comment on attachment 228385 [details]
nit

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

>> Source/WebCore/rendering/RenderLayer.cpp:190
>> +    , m_hasUnisolatedBlendingDescendants(false)
> 
> With this patch, you have 2 flags tracking isolation: m_hasUnisolatedCompositedBlendingDescendants and m_hasUnisolatedBlendingDescendants. I suggest you should add some assertions checking that if the first is true the second should also be true.

I added an assert in RenderLayerCompositor::computeCompositingRequirements

>> Source/WebCore/rendering/RenderLayer.cpp:-825
>> -    if (isComposited())
> 
> Why did you remove this check?

The isolation flag should be correct for all layers regardless of their compositing state. Moreover, if updateAncestorChainHasBlendigDescendants() is called from styleChanged(), the compositing state might not be accurate, as it's going to be recalculated afterwards. (I added this info to ChangeLog).

>> Source/WebCore/rendering/RenderLayer.cpp:1099
>> +#endif
> 
> I would rewrite the code above to be wrapped in ENABLE(CSS_COMPOSITING):
> #if ENABLE(CSS_COMPOSITING)
> bool hasUnisolatedBlendingDescendants = false;
> #endif

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:1145
>> +#endif
> 
> Can these lines be moved above, before:
> if (m_hasVisibleDescendant && m_hasSelfPaintingLayerDescendant && m_hasOutOfFlowPositionedDescendant && hasUnisolatedBlendingDescendants)
> 
> If so, in the if clause you can use the function you added instead of testing the variable hasUnisolatedBlendingDescendants.

I reworked this so that it matches the style the other flags are used, but using ifdefs.

>> Source/WebCore/rendering/RenderLayer.cpp:6432
>> +        if (isStackingContext && parent()) {
> 
> 1. When is parent() null here?
> 2. You can rewrite to smth like:
> if (parent()) {
>     if (isstackingContent) { ... }
>     else { ... }
> }

Done.

[Ion Rosca]

Simon, Dean, could one of you take a look at the last patch? Thanks!

[Dean Jackson]

Comment on attachment 228494 [details]
addressing Mihnea's comments 2

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

Patch is ok, but the tests need a fair amount of cleanup.

> LayoutTests/css3/compositing/blend-mode-isolation-flags-append-non-stacking-context-blending.html:8
> +	.stacking-context {
> +		-webkit-isolation: isolate;
> +		isolation: isolate;
> +	}

Many of these test files are using 8 space indents.

> LayoutTests/css3/compositing/blend-mode-isolation-flags-append-non-stacking-context-blending.html:54
> +		<!-- <div class="append-root">
> +			<div class="blending leaf">
> +			</div>
> +		</div> -->

Did you mean to leave this in?

> LayoutTests/css3/compositing/blend-mode-isolation-flags-append-stacking-context-blending.html:46
> +    function change() {
> +    	var target = document.getElementById("target");
> +    	var toAppend = document.createElement('div');
> +    	var blendingElement = document.createElement('div');
> +    	blendingElement.className = "blending leaf";
> +    	toAppend.appendChild(blendingElement);
> +    	toAppend.className = "stacking-context append-root"
> +    	target.appendChild(toAppend);
> +
> +        if (window.testRunner)
> +            window.testRunner.notifyDone();
> +    }

Indentation is weird. Also, you're mixing ' and "

> LayoutTests/css3/compositing/blend-mode-isolation-flags-append-stacking-context-blending.html:54
> +		<!-- <div class="stacking-context append-root">
> +			<div class="blending leaf">
> +			</div>
> +		</div> -->

Again, intentional?

> LayoutTests/css3/compositing/blend-mode-isolation-flags-remove-non-stacking-context-blending.html:41
> +    function change() {
> +    	var toremove = document.getElementById("toremove");
> +    	toremove.parentNode.removeChild(toremove);
> +
> +        if (window.testRunner)
> +            window.testRunner.notifyDone();
> +    }

More weirdness.

> LayoutTests/css3/compositing/blend-mode-isolation-flags-turn-off-blending-no-isolation.html:36
> +		<div class="">

Please remove class="" (it's in a few places)

> LayoutTests/css3/compositing/blend-mode-isolation-flags-turn-off-blending.html:31
> +    function change() {
> +    	var target = document.getElementById("target");
> +    	target.className = "stacking-context";
> +        if (window.testRunner)
> +            window.testRunner.notifyDone();
> +    }

More weirdness.

[Mihnea Ovidenie]

Created attachment 229530 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 229530 [details]
Patch for landing

Clearing flags on attachment: 229530

Committed r167424: <http://trac.webkit.org/changeset/167424>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.