Bug 140183 – Failed to display background image when body is composited

• Status: REOPENED
• Component: Layout and Rendering

[Julien Isorce]

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.

[Julien Isorce]

Created attachment 244160 [details]
Render: properly update body's background image

Patch with unit test

[Simon Fraser (smfr)]

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"

[Julien Isorce]

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.

[Julien Isorce]

Created attachment 244335 [details]
Render: properly update body's background image

Forgot change logs.

[Build Bot]

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

[Build Bot]

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

[Simon Fraser (smfr)]

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()) {

[Julien Isorce]

Created attachment 244691 [details]
Render: properly update body's background image

Addressed remarks (ref test and composited condition)

[Simon Fraser (smfr)]

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?

[Julien Isorce]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Julien Isorce]

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.

[Simon Fraser (smfr)]

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?

[Julien Isorce]

(In reply to comment #16)
> Is it testable on its own?

I will try to make a such test and let you know.

[Julien Isorce]

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

[Julien Isorce]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Julien Isorce]

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.)

[zalan]

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();

[Julien Isorce]

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.

[Julien Isorce]

Created attachment 245863 [details]
Render: properly update body's background image

Addressed remark (i.e. switch to early returns)

[zalan]

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

[zalan]

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)

[Julien Isorce]

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.

[Julien Isorce]

Created attachment 246026 [details]
Render: properly update body's background image

Typo fix: "Previsouly" -> "Previously"

[Simon Fraser (smfr)]

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.

[Julien Isorce]

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).

[zalan]

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>

[Julien Isorce]

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

[Darin Adler]

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.

[Anton Obzhirov]

The updated patch is landed.
https://trac.webkit.org/changeset/179871.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142605

[Simon Fraser (smfr)]

Marked the test as an image failure.

[Julien Isorce]

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.

[Simon Fraser (smfr)]

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.

[Julien Isorce]

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.

[Alexey Proskuryakov]

This was rolled out in <http://trac.webkit.org/changeset/188190>.

Also removing TestExpectation - turns out that the test was an ImageOnlyFailure on Mac!

[Csaba Osztrogonác]

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.

[Brady Eidson]

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.