Usually body's RenderBox skip to paint the background (see skipBodyBackground function in RenderBox) because it assumes HTML's Render will do. Problem happens when these 2 renders are composited (isComposited() return true). Indeed if HTML's Render does not draw its content (RenderLayerBacking::isSimpleContainerCompositingLayer() returns true) then body's render should not skip painting the background. In the following patch I improved the condition in "skipBodyBackground" function regarding explanation above. But it not enough, the CachedImage has to be marked isLoaded when CachedImage::finishLoading is called and before notifying observers. Moreover the RenderLayerBacking has to redisplay its content once the background is loaded. Also see layout test in the following attached patch. The negative zIndex and the video tag are there to hit the call "layer.setIndirectCompositingReason(RenderLayer::IndirectCompositingReason::BackgroundLayer);" in RenderLayerCompositor::computeCompositingRequirements which one makes the body's RenderBox to be composited. Without the patch the background of this layout test is not displayed.
Created attachment 244160 [details] Render: properly update body's background image Patch with unit test
Comment on attachment 244160 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=244160&action=review > LayoutTests/ChangeLog:9 > + When HTML and BODY renderers are both composited the > + skipBodyBackground condition should also take into account > + if the HTML's layer can draw its contents. > + Also ensure the background is displayed when loaded. There are lots of other places in the code that need to know about root background propagation, and I wonder which other of them need similar logic. > LayoutTests/compositing/backgrounds/background-image-layer.html:21 > +function createVideo() { > + var video = document.createElement("video"); > + video.style.width = "320px"; > + video.style.height = "180px"; > + var source = document.createElement("source"); > + source.src = "../../media/content/test-25fps.mp4"; > + source.type = "video/mp4"; > + video.appendChild(source); > + > + return video; > +} It would be better to us something other than video to trigger compositing. Video tests are notoriously unreliable. > Source/WebCore/rendering/RenderBox.cpp:115 > + if (skip && bodyElementRenderer->isComposited() && documentElementRenderer->isBox()) { Seems weird to test documentElementRenderer->isBox(). Maybe just a hasLayer() check? > Source/WebCore/rendering/RenderBox.cpp:118 > + if (backing && backing->graphicsLayer()) If there's a backing, it will always have a graphics layer. You can just test layer->isComposited() > Source/WebCore/rendering/RenderLayerBacking.cpp:1920 > + // Both update grometry and set content needs redisplay. "grometry"
Created attachment 244260 [details] Render: properly update body's background image Thx for the review. I addressed the remarks and the test now uses an animation with "-webkit-filter" in order to make sure "RenderLayerCompositor::requiresCompositingForAnimation" return true. About your first comment, I am also concerned about other places but I have no test case. Also not sure to understand if you mean that this patch should cover all places.
Created attachment 244335 [details] Render: properly update body's background image Forgot change logs.
Comment on attachment 244335 [details] Render: properly update body's background image Attachment 244335 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5301136626024448 New failing tests: animations/animation-background-image.html
Created attachment 244336 [details] Archive of layout-test-results from ews107 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 244335 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=244335&action=review Good but needs to be a ref test. > LayoutTests/ChangeLog:13 > + * animations/animation-background-image.html: Added. Please make this a ref test. > Source/WebCore/rendering/RenderBox.cpp:116 > + if (skip && bodyElementRenderer->isComposited() && documentElementRenderer->hasLayer()) { > + if (RenderLayerBacking* backing = downcast<RenderLayerModelObject>(documentElementRenderer)->layer()->backing()) I think this would read better as: if (skip && bodyElementRenderer->isComposited() && documentElementRenderer->isComposited()) {
Created attachment 244691 [details] Render: properly update body's background image Addressed remarks (ref test and composited condition)
Comment on attachment 244691 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=244691&action=review > Source/WebCore/loader/cache/CachedImage.cpp:430 > + // Ensure isLoaded() return true when notifying observers. > + setLoading(false); What is this change doing here?
Created attachment 244767 [details] Render: properly update body's background image Thx!, indeed setLoading(false); is not needed when loading png from local files. Though it is needed if url is http. Same for the updateGeometry change in RenderLayerBacking. I will submit them in other bugs/patches.
Comment on attachment 244767 [details] Render: properly update body's background image Attachment 244767 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5810555851898880 New failing tests: animations/animation-background-image.html
Created attachment 244770 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 244767 [details] Render: properly update body's background image Attachment 244767 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5144796729442304 New failing tests: animations/animation-background-image.html
Created attachment 244780 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 244805 [details] Render: properly update body's background image Go back to previous patch because even when loading the png from the disk, it may finish to load the png with a slight delay. In this case the cached image's observers are notified but checking isLoaded() return false. Which does not ask to repaint the background. To sum-up the 3 file changes are required to make the included layout test work.
Comment on attachment 244805 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=244805&action=review > Source/WebCore/loader/cache/CachedImage.cpp:430 > + // Ensure isLoaded() return true when notifying observers. > + setLoading(false); If you really need this change, it should be done in a separate patch, and I'm surprised that it doesn't have other side effects. Is it testable on its own?
(In reply to comment #16) > Is it testable on its own? I will try to make a such test and let you know.
I found a such test but I think about the "setLoading" diff, the problem is more an API pb, see patch attached to https://bugs.webkit.org/show_bug.cgi?id=140722
Created attachment 245709 [details] Render: properly update body's background image Dependency patch https://bugs.webkit.org/show_bug.cgi?id=140722 has now landed.
Comment on attachment 245709 [details] Render: properly update body's background image Attachment 245709 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5813513876406272 New failing tests: animations/animation-background-image.html
Created attachment 245715 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 245709 [details] Render: properly update body's background image Attachment 245709 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6122704277078016 New failing tests: animations/animation-background-image.html
Created attachment 245716 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 245721 [details] Render: properly update body's background image Initial patch contains 3 changes 1 in RenderBox (the main fix), 1 in RenderLayerBacking and 1 in CachedImage. Let's also keep the second change. (previous failure shows that we need both 1 and 2) (For log, the third change has been put in a separate patch already here: https://bugs.webkit.org/show_bug.cgi?id=140722, and now landed.)
Comment on attachment 245721 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=245721&action=review > Source/WebCore/rendering/RenderBox.cpp:119 > - return documentElementRenderer > + bool skip = documentElementRenderer > && !documentElementRenderer->hasBackground() > && (documentElementRenderer == bodyElementRenderer->parent()); > + > + // Also only skip painting background if document's layer can draw its content. > + if (skip && bodyElementRenderer->isComposited() && documentElementRenderer->isComposited()) > + skip &= downcast<RenderLayerModelObject>(documentElementRenderer)->layer()->backing()->graphicsLayer()->drawsContent(); > + > + return skip; This is getting confusing. I think it's time to switch to early returns. ASSERT(bodyElementRenderer->isBody()); // The <body> only paints its background if the root element has defined a background independent of the body, // or if the <body>'s parent is not the document element's renderer (e.g. inside SVG foreignObject). auto documentElementRenderer = bodyElementRenderer->document().documentElement()->renderer(); if (!documentElementRenderer) return false; if (documentElementRenderer->hasBackground()) return false; if (documentElementRenderer != bodyElementRenderer->parent()) return false; if (!bodyElementRenderer->isComposited() || !documentElementRenderer->isComposited())) return false; ASSERT(is<RenderLayerModelObject>(documentElementRenderer)); return downcast<RenderLayerModelObject>(documentElementRenderer)->layer()->backing()->graphicsLayer()->drawsContent();
Comment on attachment 245721 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=245721&action=review >> Source/WebCore/rendering/RenderBox.cpp:119 >> + return skip; > > This is getting confusing. I think it's time to switch to early returns. > > ASSERT(bodyElementRenderer->isBody()); > // The <body> only paints its background if the root element has defined a background independent of the body, > // or if the <body>'s parent is not the document element's renderer (e.g. inside SVG foreignObject). > auto documentElementRenderer = bodyElementRenderer->document().documentElement()->renderer(); > > if (!documentElementRenderer) > return false; > > if (documentElementRenderer->hasBackground()) > return false; > > if (documentElementRenderer != bodyElementRenderer->parent()) > return false; > > if (!bodyElementRenderer->isComposited() || !documentElementRenderer->isComposited())) > return false; > > ASSERT(is<RenderLayerModelObject>(documentElementRenderer)); > return downcast<RenderLayerModelObject>(documentElementRenderer)->layer()->backing()->graphicsLayer()->drawsContent(); Great idea! Thx Though I think the last lines should be: if (bodyElementRenderer->isComposited() && documentElementRenderer->isComposited()) return downcast<RenderLayerModelObject>(documentElementRenderer)->layer()->backing()->graphicsLayer()->drawsContent(); return true; Otherwise it will miss the case where bodyElementRenderer->isComposited() && documentElementRenderer->isComposited() is false but still return true. I'll update the patch very soon. Thx again for the "early returns" suggestion.
Created attachment 245863 [details] Render: properly update body's background image Addressed remark (i.e. switch to early returns)
> if (bodyElementRenderer->isComposited() && > documentElementRenderer->isComposited()) > return > downcast<RenderLayerModelObject>(documentElementRenderer)->layer()- > >backing()->graphicsLayer()->drawsContent(); > > return true; > > Otherwise it will miss the case where bodyElementRenderer->isComposited() && > documentElementRenderer->isComposited() is false but still return true. Indeed. "Also only skip painting background if document's layer can draw its content." comment mislead me as I thought by 'only skip...' you meant that if the layer can't draw itself, it needs to return false.
Comment on attachment 245863 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=245863&action=review > Source/WebCore/rendering/RenderBox.cpp:121 > + // Also only skip painting background if document's layer can draw its content. This comment is misleading and does not really add any value. It says what the next 2 lines does but it does not say why. Please remove it or preferably explain why we can skip background panting when both body and html are composited and HTML paints its own content. (or update the Changelog entry accordingly)
Created attachment 245931 [details] Render: properly update body's background image Addressed remarks (I updated Changelog and remove comment). There are many resaons for graphicsLayer->drawsContent() return false. But for the layout test that comes with the patch, it is due to RenderLayerBacking::isSimpleContainerCompositingLayer() returning true.
Created attachment 246026 [details] Render: properly update body's background image Typo fix: "Previsouly" -> "Previously"
Comment on attachment 246026 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=246026&action=review > LayoutTests/animations/animation-background-image.html:35 > +function createAnimation() { > + var anim = document.createElement("div"); > + anim.style.backgroundColor = "blue"; > + anim.style.width = "360px"; > + anim.style.height = "150px"; > + anim.style.position = "absolute"; > + anim.style.webkitAnimationName = "custommove"; > + anim.style.webkitAnimationDuration = "1s"; > + anim.style.webkitAnimationIterationCount = "1"; > + anim.style.webkitAnimationFillMode = "forwards"; > + anim.style.webkitAnimationPlayState = "running"; > + > + return anim; > +} > + > +function onLoad() { > + document.body.style.background = "url(resources/green-100.png)"; > + > + var div = document.getElementById("div"); > + div.style.zIndex = "-1"; > + > + var anim = createAnimation(); > + anim.addEventListener("webkitAnimationEnd", onAnimationEnd, false); > + > + div.appendChild(anim); You should be able to make a test that uses forces compositing using translateZ(). Using filter animations is too complex.
Created attachment 246104 [details] Render: properly update body's background image Change the layout test to use translateZ() to force compositing as suggested (instead of using filter animations).
Comment on attachment 246104 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=246104&action=review > LayoutTests/compositing/backgrounds/background-image-with-negative-zindex.html:27 > +function onPageShow() { > + if (window.testRunner) > + testRunner.notifyDone(); > +} Why is it in onPageShow() and not in onLoad()? Is it because you expect it to be executed sometime after onLoad() when the new div has already been laid out? > LayoutTests/compositing/backgrounds/background-image-with-negative-zindex.html:46 > +<body onload="onLoad()" onpageshow="onPageShow()" style="z-index: -1;"> z-index: -1 should be moved to <style>
Created attachment 246264 [details] Render: properly update body's background image Addressed remarks. (i.e, onPageShow() is a residual of previous layout test that was using animations. And move z-index=-1 to <style>)
Comment on attachment 246264 [details] Render: properly update body's background image View in context: https://bugs.webkit.org/attachment.cgi?id=246264&action=review > LayoutTests/compositing/backgrounds/background-image-with-negative-zindex-expected.html:17 > +<body style="background-image: url(../resources/apple.jpg);"> A little strange to have all the rest of the body style in the <style> and this one property here on the element. > Source/WebCore/rendering/RenderLayerBacking.cpp:1921 > if ((changeType == BackgroundImageChanged) && canCreateTiledImage(renderer().style())) > - updateGeometry(); > + // Both update geometry and set content needs redisplay. > + updateAfterLayout(NeedsFullRepaint | IsUpdateRoot); WebKit coding style requires braces around the body of the if statement in a case like this. The presence/absence of braces is based on the number of textual lines, not the number of statements. The comment here is not great. We want our comments to answer the question “why”, not say what the code does.
The updated patch is landed. https://trac.webkit.org/changeset/179871.
Re-opened since this is blocked by bug 142605
Marked the test as an image failure.
Created attachment 249745 [details] RenderBox: update layer once the image is entirely loaded. Since https://bugs.webkit.org/show_bug.cgi?id=140722 has been reverted the test background-image-with-negative-zindex.html became flacky.
Comment on attachment 249745 [details] RenderBox: update layer once the image is entirely loaded. I can't tell if this is correct, because WrappedImagePtr makes all this non strongly-typed.
Created attachment 249910 [details] RenderBox: update layer once the image is entirely loaded. Implement RenderBox::notifyFinished. Note that RenderImage also implements both "::imageChanged(WrappedImagePtr image, const IntRect*)" and "::notifyFinished(CachedResource* image)". As a reminder RenderObject inherits from CachedImageClient.
This was rolled out in <http://trac.webkit.org/changeset/188190>. Also removing TestExpectation - turns out that the test was an ImageOnlyFailure on Mac!
Comment on attachment 245863 [details] Render: properly update body's background image Cleared Zalan Bujtas's review+ from obsolete attachment 245863 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 249910 [details] RenderBox: update layer once the image is entirely loaded. This patch has been pending review since 2015 with no recent activity. Clearing from the review queue. Feel free to update and resubmit if the patch is still relevant.