Bug 139435

Summary:

http://omfgdogs.info/ only animates when you resize the window

Product:

WebKit

Reporter:

Ian Henderson <ian>

Component:

Layout and Rendering

Assignee:

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:

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

Ian Henderson

Reported 2014-12-09 01:55:37 PST

Navigate to http://omfgdogs.info/. The animated dog background is not repainted unless you resize the window.

Attachments
Patch (7.35 KB, patch) 2014-12-09 12:05 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (11.98 KB, patch) 2014-12-09 13:22 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (15.22 KB, patch) 2014-12-09 15:58 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (19.42 KB, patch) 2014-12-09 17:12 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (21.64 KB, patch) 2014-12-10 16:45 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (13.41 KB, patch) 2014-12-10 20:41 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (14.08 KB, patch) 2014-12-10 21:58 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2014-12-09 08:41:42 PST

GIF throttling regression?

Radar WebKit Bug Importer

Comment 2 2014-12-09 08:42:07 PST

<rdar://problem/19190493>

Chris Dumez

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

Chris Dumez

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

Simon Fraser (smfr)

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

Chris Dumez

Comment 6 2014-12-09 12:05:58 PST

Created attachment 242953 [details] Patch

Antti Koivisto

Comment 7 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?

Chris Dumez

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

Simon Fraser (smfr)

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

Chris Dumez

Comment 10 2014-12-09 13:22:53 PST

Created attachment 242960 [details] Patch

Chris Dumez

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

Simon Fraser (smfr)

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

Chris Dumez

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

Chris Dumez

Comment 14 2014-12-09 15:58:45 PST

Created attachment 242973 [details] Patch

Chris Dumez

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

Simon Fraser (smfr)

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

Chris Dumez

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

Chris Dumez

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

Chris Dumez

Comment 19 2014-12-09 17:12:01 PST

Created attachment 242980 [details] Patch

Chris Dumez

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

Chris Dumez

Comment 21 2014-12-10 16:45:42 PST

Created attachment 243078 [details] Patch

Chris Dumez

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

Chris Dumez

Comment 23 2014-12-10 20:41:17 PST

Created attachment 243095 [details] Patch

Chris Dumez

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

Simon Fraser (smfr)

Comment 25 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?

Chris Dumez

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

Chris Dumez

Comment 27 2014-12-10 21:58:32 PST

Created attachment 243102 [details] Patch

Chris Dumez

Comment 28 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>

Chris Dumez

Comment 29 2014-12-10 23:40:47 PST

All reviewed patches have been landed. Closing bug.

Alexey Proskuryakov

Comment 30 2014-12-12 15:04:27 PST

The test added here is flaky, filed bug 139599.

David Kilzer (:ddkilzer)

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

Chris Dumez

Comment 32 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?

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