Bug 170043 - Animated SVG images are not paused in pages loaded in the background
Summary: Animated SVG images are not paused in pages loaded in the background
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 170155
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-23 19:58 PDT by Chris Dumez
Modified: 2017-05-16 13:20 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.08 KB, patch)
2017-03-29 10:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (939.14 KB, application/zip)
2017-03-29 11:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.70 MB, application/zip)
2017-03-29 12:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (deleted)
2017-03-29 12:28 PDT, Build Bot
no flags Details
Patch (11.84 KB, patch)
2017-03-29 12:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-03-23 19:58:42 PDT
SMIL animations inside SVG image do not get paused in background tabs.

Example:
https://pr.gg/svgExamples/smilAnimationImage.html (first animation)

The issue is that SVGImage has its own Page object (SVGImage::m_page) and this page contains the SMIL animation (not the main page). When the tab goes into the background, the main Page knows it is no longer visible and it pauses its SVG animations. However, the Page inside the SVGImage does not currently know it is no longer visible and its SVG animations keep going.

I saw this issue on Yahoo News and reduced it.
Comment 1 Radar WebKit Bug Importer 2017-03-23 19:59:13 PDT
<rdar://problem/31234412>
Comment 2 Chris Dumez 2017-03-23 20:13:18 PDT
I think this is the image causing trouble on Yahoo News:
https://s.yimg.com/os/yc/pt/icons/fuji-loader-v2-blue.0.svg

Seems to be used in a CSS background property.
Comment 3 Chris Dumez 2017-03-29 10:48:26 PDT
Created attachment 305758 [details]
Patch
Comment 4 Build Bot 2017-03-29 11:56:01 PDT
Comment on attachment 305758 [details]
Patch

Attachment 305758 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3434905

New failing tests:
svg/animations/animations-paused-in-background-page-iframe.html
svg/animations/animations-paused-in-background-page.html
Comment 5 Build Bot 2017-03-29 11:56:05 PDT
Created attachment 305765 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-03-29 12:03:57 PDT
Comment on attachment 305758 [details]
Patch

Attachment 305758 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3434904

New failing tests:
svg/animations/animations-paused-in-background-page-iframe.html
svg/animations/animations-paused-in-background-page.html
Comment 7 Build Bot 2017-03-29 12:04:01 PDT
Created attachment 305768 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-03-29 12:28:53 PDT
Comment on attachment 305758 [details]
Patch

Attachment 305758 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3434969

New failing tests:
svg/animations/animations-paused-in-background-page-iframe.html
svg/animations/animations-paused-in-background-page.html
Comment 9 Build Bot 2017-03-29 12:28:58 PDT
Created attachment 305770 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 10 Chris Dumez 2017-03-29 12:34:28 PDT
Created attachment 305771 [details]
Patch
Comment 11 Chris Dumez 2017-03-29 13:54:26 PDT
Comment on attachment 305771 [details]
Patch

Clearing flags on attachment: 305771

Committed r214561: <http://trac.webkit.org/changeset/214561>
Comment 12 Chris Dumez 2017-03-29 13:54:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Said Abou-Hallawa 2017-05-16 12:48:42 PDT
Comment on attachment 305771 [details]
Patch

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

> LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> +Setting page visibility to hidden
> +PASS internals.isImageAnimating(testImage) became false
> +Setting page visibility to visible
> +PASS internals.isImageAnimating(testImage) became false

These expected results do not seem to be correct. The image is not animating regardless whether the page is visible or not.

> LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> +    if (window.testRunner)
> +        testRunner.setPageVisibility('visible');
> +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);

How can this happen? The page visibility is changed to 'visible' but the image is still not animating?
Comment 14 Chris Dumez 2017-05-16 12:50:21 PDT
(In reply to Said Abou-Hallawa from comment #13)
> Comment on attachment 305771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305771&action=review
> 
> > LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> > +Setting page visibility to hidden
> > +PASS internals.isImageAnimating(testImage) became false
> > +Setting page visibility to visible
> > +PASS internals.isImageAnimating(testImage) became false
> 
> These expected results do not seem to be correct. The image is not animating
> regardless whether the page is visible or not.
> 
> > LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> > +    if (window.testRunner)
> > +        testRunner.setPageVisibility('visible');
> > +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);
> 
> How can this happen? The page visibility is changed to 'visible' but the
> image is still not animating?

This was already fixed in http://trac.webkit.org/r214626.
Comment 15 Said Abou-Hallawa 2017-05-16 13:11:25 PDT
(In reply to Chris Dumez from comment #14)
> (In reply to Said Abou-Hallawa from comment #13)
> > Comment on attachment 305771 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=305771&action=review
> > 
> > > LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> > > +Setting page visibility to hidden
> > > +PASS internals.isImageAnimating(testImage) became false
> > > +Setting page visibility to visible
> > > +PASS internals.isImageAnimating(testImage) became false
> > 
> > These expected results do not seem to be correct. The image is not animating
> > regardless whether the page is visible or not.
> > 
> > > LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> > > +    if (window.testRunner)
> > > +        testRunner.setPageVisibility('visible');
> > > +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);
> > 
> > How can this happen? The page visibility is changed to 'visible' but the
> > image is still not animating?
> 
> This was already fixed in http://trac.webkit.org/r214626.

How this test was passing sometimes? In http://trac.webkit.org/r214626, you just switched the expectation from false to true. Will this new change make the test flaky but because of the opposite reason?
Comment 16 Said Abou-Hallawa 2017-05-16 13:19:29 PDT
(In reply to Said Abou-Hallawa from comment #15)
> (In reply to Chris Dumez from comment #14)
> > (In reply to Said Abou-Hallawa from comment #13)
> > > Comment on attachment 305771 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=305771&action=review
> > > 
> > > > LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> > > > +Setting page visibility to hidden
> > > > +PASS internals.isImageAnimating(testImage) became false
> > > > +Setting page visibility to visible
> > > > +PASS internals.isImageAnimating(testImage) became false
> > > 
> > > These expected results do not seem to be correct. The image is not animating
> > > regardless whether the page is visible or not.
> > > 
> > > > LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> > > > +    if (window.testRunner)
> > > > +        testRunner.setPageVisibility('visible');
> > > > +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);
> > > 
> > > How can this happen? The page visibility is changed to 'visible' but the
> > > image is still not animating?
> > 
> > This was already fixed in http://trac.webkit.org/r214626.
> 
> How this test was passing sometimes? In http://trac.webkit.org/r214626, you
> just switched the expectation from false to true. Will this new change make
> the test flaky but because of the opposite reason?

Never mind I just realized http://trac.webkit.org/r214626 is not the fix of the flatness of https://bugs.webkit.org/show_bug.cgi?id=172183 and it was landed 7 weeks ago.
Comment 17 Chris Dumez 2017-05-16 13:20:34 PDT
(In reply to Said Abou-Hallawa from comment #15)
> (In reply to Chris Dumez from comment #14)
> > (In reply to Said Abou-Hallawa from comment #13)
> > > Comment on attachment 305771 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=305771&action=review
> > > 
> > > > LayoutTests/svg/animations/animations-paused-in-background-page-expected.txt:10
> > > > +Setting page visibility to hidden
> > > > +PASS internals.isImageAnimating(testImage) became false
> > > > +Setting page visibility to visible
> > > > +PASS internals.isImageAnimating(testImage) became false
> > > 
> > > These expected results do not seem to be correct. The image is not animating
> > > regardless whether the page is visible or not.
> > > 
> > > > LayoutTests/svg/animations/animations-paused-in-background-page.html:14
> > > > +    if (window.testRunner)
> > > > +        testRunner.setPageVisibility('visible');
> > > > +    shouldBecomeEqual("internals.isImageAnimating(testImage)", "false", finishJSTest);
> > > 
> > > How can this happen? The page visibility is changed to 'visible' but the
> > > image is still not animating?
> > 
> > This was already fixed in http://trac.webkit.org/r214626.
> 
> How this test was passing sometimes? In http://trac.webkit.org/r214626, you
> just switched the expectation from false to true. Will this new change make
> the test flaky but because of the opposite reason?

There is a small delay between the point the visibility is set and the image actually starts / stops animating.

Previously, the test was sometimes passing because:
1. The page is initially hidden
2. We ask for the page to become visible
3. We poll until the image stops animating

Step 3 will sometimes find that the image is not animating because the page visibility has not been taken into consideration yet.

Since the page is initially hidden and then becomes visible, then the image's state will go from Non-animating to Animating.

If in step 3, we check that the image starts animating (Which is what we really wanted to test and what I did in the follow-up patch), then it will NOT be flaky in the other direction.

The image goes from Non-animating to Animating, and once it is animating, it keeps animating.