Bug 130892

Summary:

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

Product:

WebKit

Reporter:

Ion Rosca <rosca>

Component:

CSS

Assignee:

Mihnea Ovidenie <mihnea>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, dino, esprehn+autocc, glenn, kondapallykalyan, mihnea, rniwa, simon.fraser, WebkitBugTracker

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

95614

Attachments:

Description	Flags
not final patch	none
not for review	none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion	none
adding tests and ifdefs	none
nit	none
addressing Mihnea's comments	none
addressing Mihnea's comments 2	none
Patch for landing	none

Ion Rosca

Reported 2014-03-28 03:06:46 PDT

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

Attachments
not final patch (12.89 KB, patch) 2014-03-28 03:20 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
not for review (13.92 KB, patch) 2014-03-31 02:53 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (2.18 MB, application/zip) 2014-03-31 03:52 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (2.20 MB, application/zip) 2014-03-31 04:20 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (2.20 MB, application/zip) 2014-03-31 05:17 PDT, Build Bot	no flags	Details
adding tests and ifdefs (54.90 KB, patch) 2014-04-02 04:53 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
nit (54.90 KB, patch) 2014-04-02 05:08 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
addressing Mihnea's comments (56.48 KB, patch) 2014-04-03 02:06 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
addressing Mihnea's comments 2 (56.21 KB, patch) 2014-04-03 03:34 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
Patch for landing (59.50 KB, patch) 2014-04-17 01:05 PDT, Mihnea Ovidenie	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Ion Rosca

Comment 1 2014-03-28 03:20:19 PDT

Created attachment 228034 [details] not final patch

Ion Rosca

Comment 2 2014-03-31 02:53:00 PDT

Created attachment 228162 [details] not for review

Build Bot

Comment 3 2014-03-31 03:52:30 PDT

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

Comment 4 2014-03-31 03:52:33 PDT

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 5 2014-03-31 04:20:01 PDT

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

Comment 6 2014-03-31 04:20:05 PDT

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 7 2014-03-31 05:17:04 PDT

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

Comment 8 2014-03-31 05:17:07 PDT

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

Comment 9 2014-04-02 04:53:07 PDT

Created attachment 228383 [details] adding tests and ifdefs

Ion Rosca

Comment 10 2014-04-02 05:08:11 PDT

Created attachment 228385 [details] nit

Mihnea Ovidenie

Comment 11 2014-04-03 00:34:49 PDT

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

Comment 12 2014-04-03 02:06:31 PDT

Created attachment 228491 [details] addressing Mihnea's comments

Ion Rosca

Comment 13 2014-04-03 03:34:39 PDT

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

Ion Rosca

Comment 14 2014-04-03 03:43:27 PDT

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

Comment 15 2014-04-08 10:18:57 PDT

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

Dean Jackson

Comment 16 2014-04-16 13:17:53 PDT

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 > +  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 > +  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

Comment 17 2014-04-17 01:05:03 PDT

Created attachment 229530 [details] Patch for landing

WebKit Commit Bot

Comment 18 2014-04-17 01:43:24 PDT

Comment on attachment 229530 [details] Patch for landing Clearing flags on attachment: 229530 Committed r167424: <http://trac.webkit.org/changeset/167424>

WebKit Commit Bot

Comment 19 2014-04-17 01:43:29 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.