Bug 139435

Summary: http://omfgdogs.info/ only animates when you resize the window
Product: WebKit Reporter: Ian Henderson <ian>
Component: Layout and RenderingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, cdumez, cmarcelo, commit-queue, esprehn+autocc, glenn, kangil.han, kling, koivisto, kondapallykalyan, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: OS X 10.9   
See Also: https://bugs.webkit.org/show_bug.cgi?id=139599
Bug Depends on:    
Bug Blocks: 139528    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ian Henderson 2014-12-09 01:55:37 PST
Navigate to http://omfgdogs.info/.  The animated dog background is not repainted unless you resize the window.
Comment 1 Simon Fraser (smfr) 2014-12-09 08:41:42 PST
GIF throttling regression?
Comment 2 Radar WebKit Bug Importer 2014-12-09 08:42:07 PST
<rdar://problem/19190493>
Comment 3 Chris Dumez 2014-12-09 10:25:41 PST
Yes, it looks like the isInsideViewport() detection is broken for this case:
shouldRepaintForImageAnimation(0x119fc5398, viewport: [0, 0, 1638, 1299])
-> rects don't intersec, element rect: [8, 8, 1638, 0]
-> outsideViewport()

It is claiming the image is outside the viewport because its absoluteClippedOverflowRect has 0 height for some reason.
Comment 4 Chris Dumez 2014-12-09 10:29:36 PST
According to Web Inspector, the body Element (which has the animated gif as background) has a computed height of 0px.
Comment 5 Simon Fraser (smfr) 2014-12-09 10:36:38 PST
(In reply to comment #4)
> According to Web Inspector, the body Element (which has the animated gif as
> background) has a computed height of 0px.

Right, that's root background propagation. The throttling logic needs to take this into account.
Comment 6 Chris Dumez 2014-12-09 12:05:58 PST
Created attachment 242953 [details]
Patch
Comment 7 Antti Koivisto 2014-12-09 12:17:38 PST
Comment on attachment 242953 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1342
> +    // Don't bother computing rects for the root or body renderer and
> +    // consider it is always inside the viewport.
> +    if (isBody() || isRoot())
> +        return true;

Shouldn't we still throttle subframe bodies that are scrolled out of view?
Comment 8 Chris Dumez 2014-12-09 12:30:21 PST
Comment on attachment 242953 [details]
Patch

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

>> Source/WebCore/rendering/RenderElement.cpp:1342
>> +        return true;
> 
> Shouldn't we still throttle subframe bodies that are scrolled out of view?

Good point, I'll add a layout test for this case and adapt the code accordingly.
Comment 9 Simon Fraser (smfr) 2014-12-09 12:39:25 PST
Comment on attachment 242953 [details]
Patch

I think shouldRepaintForImageAnimation() really needs to figure out if the animated image is being applied as the root background.
Comment 10 Chris Dumez 2014-12-09 13:22:53 PST
Created attachment 242960 [details]
Patch
Comment 11 Chris Dumez 2014-12-09 14:49:28 PST
Ok, the case mentioned by Antti is now handled properly (covered by a new layout test). It now uses the background (extended) rect so hopefully, it addresses Simon's concerns as well.
Comment 12 Simon Fraser (smfr) 2014-12-09 14:55:55 PST
Comment on attachment 242960 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1349
> +    if (isRoot() || isBody())
> +        repaintRect = snappedIntRect(view->backgroundRect(view));
> +    else
> +        repaintRect = snappedIntRect(absoluteClippedOverflowRect());

This doesn't handle the case where <html> and <body> have different background styles. Nor does it handle the case of a non-repeating animated GIF background-image positioned outside the bounds/viewport (but neither did the existing code).
Comment 13 Chris Dumez 2014-12-09 15:04:32 PST
Comment on attachment 242960 [details]
Patch

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

>> Source/WebCore/rendering/RenderElement.cpp:1349
>> +        repaintRect = snappedIntRect(absoluteClippedOverflowRect());
> 
> This doesn't handle the case where <html> and <body> have different background styles. Nor does it handle the case of a non-repeating animated GIF background-image positioned outside the bounds/viewport (but neither did the existing code).

"This doesn't handle the case where <html> and <body> have different background styles"

Sorry, you lost me. Could you maybe give me an example where this check would be insufficient? Or hint what kind of change would be needed to handle such case? Note that since this function is used for optimization purposes, returning a false-positive for isInsideViewport() wouldn't be that bad. What we should absolutely avoid is to return false-negative.

Note that this logic is very close to what is done in RenderObject::repaintSlowRepaintObject() already.
Comment 14 Chris Dumez 2014-12-09 15:58:45 PST
Created attachment 242973 [details]
Patch
Comment 15 Chris Dumez 2014-12-09 16:02:23 PST
Comment on attachment 242973 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1346
> +    if (isRoot() || (isBody() && !document().documentElement()->renderer()->hasBackground()))

I added the !document().documentElement()->renderer()->hasBackground() check so that we are no longer regressing the case where both the body and the html element have a background, and the body is outside the viewport (with an animated gif background). My previous iteration would wrongly use the background rect in this case and thus we would keep animating the body's background. This is covered by the following new layout test: fast/dom/repeating-timer-animating-body-element-outside-viewport-throttling.html.
Comment 16 Simon Fraser (smfr) 2014-12-09 16:10:43 PST
Comment on attachment 242973 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1337
> +    if (!isRooted(&view))

isRooted is a tree walk, and renderers who get this will almost always be rooted. Is this check worth it?

>> Source/WebCore/rendering/RenderElement.cpp:1346
>> +    if (isRoot() || (isBody() && !document().documentElement()->renderer()->hasBackground()))
> 
> I added the !document().documentElement()->renderer()->hasBackground() check so that we are no longer regressing the case where both the body and the html element have a background, and the body is outside the viewport (with an animated gif background). My previous iteration would wrongly use the background rect in this case and thus we would keep animating the body's background. This is covered by the following new layout test: fast/dom/repeating-timer-animating-body-element-outside-viewport-throttling.html.

I really want this to call rendererForRootBackground() somehow. The background propagation code is spread out too far already, and this adds yet another case that is impossible to find by searching.
Comment 17 Chris Dumez 2014-12-09 16:36:46 PST
Comment on attachment 242973 [details]
Patch

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

>> Source/WebCore/rendering/RenderElement.cpp:1337
>> +    if (!isRooted(&view))
> 
> isRooted is a tree walk, and renderers who get this will almost always be rooted. Is this check worth it?

Probably not. I'll go back to using view().

>>> Source/WebCore/rendering/RenderElement.cpp:1346
>>> +    if (isRoot() || (isBody() && !document().documentElement()->renderer()->hasBackground()))
>> 
>> I added the !document().documentElement()->renderer()->hasBackground() check so that we are no longer regressing the case where both the body and the html element have a background, and the body is outside the viewport (with an animated gif background). My previous iteration would wrongly use the background rect in this case and thus we would keep animating the body's background. This is covered by the following new layout test: fast/dom/repeating-timer-animating-body-element-outside-viewport-throttling.html.
> 
> I really want this to call rendererForRootBackground() somehow. The background propagation code is spread out too far already, and this adds yet another case that is impossible to find by searching.

Hmm, rendererForRootBackground() can only be called if isRoot() and will give me the body's renderer if html has no background. However, if the root had no background (image), why would this method be called? (since it is called to determine if we should animate the background image of the html element).
Comment 18 Chris Dumez 2014-12-09 16:49:43 PST
Comment on attachment 242973 [details]
Patch

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

>>>> Source/WebCore/rendering/RenderElement.cpp:1346
>>>> +    if (isRoot() || (isBody() && !document().documentElement()->renderer()->hasBackground()))
>>> 
>>> I added the !document().documentElement()->renderer()->hasBackground() check so that we are no longer regressing the case where both the body and the html element have a background, and the body is outside the viewport (with an animated gif background). My previous iteration would wrongly use the background rect in this case and thus we would keep animating the body's background. This is covered by the following new layout test: fast/dom/repeating-timer-animating-body-element-outside-viewport-throttling.html.
>> 
>> I really want this to call rendererForRootBackground() somehow. The background propagation code is spread out too far already, and this adds yet another case that is impossible to find by searching.
> 
> Hmm, rendererForRootBackground() can only be called if isRoot() and will give me the body's renderer if html has no background. However, if the root had no background (image), why would this method be called? (since it is called to determine if we should animate the background image of the html element).

Using the view's background rect seems like the right thing to do for the root, and for the body if the root has not background (This one is what this bug is about), right?
By the way, this method may be better called "isRepaintRectInsideViewport()" now.
Comment 19 Chris Dumez 2014-12-09 17:12:01 PST
Created attachment 242980 [details]
Patch
Comment 20 Chris Dumez 2014-12-10 10:04:33 PST
Comment on attachment 242980 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1335
>      auto& frameView = view().frameView();

Now calling view()

> Source/WebCore/rendering/RenderElement.cpp:1341
> +    if (isRoot() || (isBody() && !document().documentElement()->renderer()->hasBackground()))

I haven't changed this part, as per my earlier comment. Please let me know if I misunderstood and rendererForRootBackground() should indeed be used.
Comment 21 Chris Dumez 2014-12-10 16:45:42 PST
Created attachment 243078 [details]
Patch
Comment 22 Chris Dumez 2014-12-10 16:46:42 PST
Comment on attachment 243078 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1344
> +        shouldUseBackgroundRect = &rootObject->rendererForRootBackground() == this;

Now using rendererForRootBackground() as suggested by Simon.
Comment 23 Chris Dumez 2014-12-10 20:41:17 PST
Created attachment 243095 [details]
Patch
Comment 24 Chris Dumez 2014-12-10 21:39:48 PST
Comment on attachment 243095 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1370
> +    LayoutRect imageRect = shouldUseBackgroundRect ? renderer.view().backgroundRect(&renderer.view()) : renderer.absoluteClippedOverflowRect();

This should use the boundingBox instead of the overflowRect. I am fixing this in https://bugs.webkit.org/show_bug.cgi?id=139528 as it is not strictly related (I am happy to merge the patches if requested though).
Comment 25 Simon Fraser (smfr) 2014-12-10 21:43:45 PST
Comment on attachment 243095 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1364
> +    bool shouldUseBackgroundRect = renderer.isRoot();

I'd call backgroundIsPaintedByRoot

> Source/WebCore/rendering/RenderElement.cpp:1366
> +        RenderElement* rootObject = renderer.document().documentElement() ? renderer.document().documentElement()->renderer() : nullptr;

Add a // FIXME: should share body background propagation code.

> Source/WebCore/rendering/RenderElement.cpp:1370
> +    LayoutRect imageRect = shouldUseBackgroundRect ? renderer.view().backgroundRect(&renderer.view()) : renderer.absoluteClippedOverflowRect();

imageRect could be "backgroundPaintingRect"

> Source/WebCore/rendering/RenderElement.cpp:1373
>  

I would like a FIXME lower down where we get the imageDidChange callback and put the renderer into the hash set, saying that we're losing information about which part of the style the image is associated with.

> Source/WebCore/testing/Internals.h:77
> +    bool hasPausedImageAnimations(Element*, ExceptionCode&);

Nice!

> LayoutTests/fast/images/animated-gif-body-delegated-background-image.html:13
> +  isBackgroundAnimated = !internals.hasPausedImageAnimations(document.body);
> +  bodyHeight = window.getComputedStyle(document.body)["height"];

Do these variables have to be in global scope?
Comment 26 Chris Dumez 2014-12-10 21:49:25 PST
Comment on attachment 243095 [details]
Patch

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

>>> Source/WebCore/rendering/RenderElement.cpp:1370
>>> +    LayoutRect imageRect = shouldUseBackgroundRect ? renderer.view().backgroundRect(&renderer.view()) : renderer.absoluteClippedOverflowRect();
>> 
>> This should use the boundingBox instead of the overflowRect. I am fixing this in https://bugs.webkit.org/show_bug.cgi?id=139528 as it is not strictly related (I am happy to merge the patches if requested though).
> 
> imageRect could be "backgroundPaintingRect"

In the case of a regular <img> (not a background-image), wouldn't backgroundPaintingRect be a bit confusing? I guess it could still be considered a "background" painting. Just confirming.

>> LayoutTests/fast/images/animated-gif-body-delegated-background-image.html:13
>> +  bodyHeight = window.getComputedStyle(document.body)["height"];
> 
> Do these variables have to be in global scope?

Yes, it is because they are used in shouldBe() functions.
Comment 27 Chris Dumez 2014-12-10 21:58:32 PST
Created attachment 243102 [details]
Patch
Comment 28 Chris Dumez 2014-12-10 23:40:35 PST
Comment on attachment 243102 [details]
Patch

Clearing flags on attachment: 243102

Committed r177135: <http://trac.webkit.org/changeset/177135>
Comment 29 Chris Dumez 2014-12-10 23:40:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Alexey Proskuryakov 2014-12-12 15:04:27 PST
The test added here is flaky, filed bug 139599.
Comment 31 David Kilzer (:ddkilzer) 2015-01-26 09:58:07 PST
Comment on attachment 243102 [details]
Patch

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

> Source/WebCore/rendering/RenderElement.cpp:1368
> +        RenderElement* rootObject = renderer.document().documentElement() ? renderer.document().documentElement()->renderer() : nullptr;
> +        backgroundIsPaintedByRoot = &rootObject->rendererForRootBackground() == &renderer;

Why is there no NULL check here?  rootObject could be set to nullptr, then we dereference it immediately on the next line.
Comment 32 Chris Dumez 2015-01-26 10:04:41 PST
Comment on attachment 243102 [details]
Patch

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

>> Source/WebCore/rendering/RenderElement.cpp:1368
>> +        backgroundIsPaintedByRoot = &rootObject->rendererForRootBackground() == &renderer;
> 
> Why is there no NULL check here?  rootObject could be set to nullptr, then we dereference it immediately on the next line.

Yes, it looks bad. I'll take a look and fix. Did this cause crashes we know about?