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
Created attachment 228034 [details] not final patch
Created attachment 228162 [details] not for review
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
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
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
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
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
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
Created attachment 228383 [details] adding tests and ifdefs
Created attachment 228385 [details] nit
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 { ... } }
Created attachment 228491 [details] addressing Mihnea's comments
Created attachment 228494 [details] addressing Mihnea's comments 2
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.
Simon, Dean, could one of you take a look at the last patch? Thanks!
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.
Created attachment 229530 [details] Patch for landing
Comment on attachment 229530 [details] Patch for landing Clearing flags on attachment: 229530 Committed r167424: <http://trac.webkit.org/changeset/167424>
All reviewed patches have been landed. Closing bug.