Bug 114915 – Animations and Transitions should not start when globally suspended

• Component: CSS
• Status: RESOLVED FIXED

[Dean Jackson]

If the AnimationController is suspended (e.g. via the private API on WebView) then we should not start any new animations or transitions. For animations, this just means the animation timer should not start until the animations are globally resumed. At that point any delay time starts counting. For transitions, they should simply act as if there is no transition duration - any change in style should happen immediately, and no events fired.

Related bug: https://bugs.webkit.org/show_bug.cgi?id=107609
(which is about such transitions in background tabs)

[Dean Jackson]

<rdar://problem/13088947>

[Dean Jackson]

Created attachment 198965 [details]
Patch

[WebKit Commit Bot]

Attachment 198965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'LayoutTests/animations/added-while-suspended-expected.txt', u'LayoutTests/animations/added-while-suspended.html', u'LayoutTests/transitions/started-while-suspended-expected.txt', u'LayoutTests/transitions/started-while-suspended.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/page/animation/AnimationBase.cpp', u'Source/WebCore/page/animation/AnimationBase.h', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationController.h', u'Source/WebCore/page/animation/AnimationControllerPrivate.h', u'Source/WebCore/page/animation/CompositeAnimation.cpp', u'Source/WebCore/page/animation/CompositeAnimation.h', u'Source/WebCore/page/animation/KeyframeAnimation.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebView.mm', u'Source/WebKit/mac/WebView/WebViewData.h', u'Source/WebKit/mac/WebView/WebViewData.mm', u'Source/autotools/symbols.filter']" exit_code: 1
Source/WebCore/page/animation/AnimationBase.h:81:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

(In reply to comment #3)
> Attachment 198965 [details] did not pass style-queue:
> 
> Source/WebCore/page/animation/AnimationBase.h:81:  One space before end of line comments  [whitespace/comments] [5]
> Total errors found: 1 in 23 files

Following existing style.

[Build Bot]

Comment on attachment 198965 [details]
Patch

Attachment 198965 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/181035

[Simon Fraser (smfr)]

Comment on attachment 198965 [details]
Patch

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

> Source/WebCore/page/animation/AnimationBase.cpp:375
> +

Extra blank line.

> Source/WebCore/page/animation/AnimationController.cpp:269
> +    if (isSuspended())
> +        return;

I wonder if we should make suspend/resume nestable, with a counter??

> Source/WebCore/page/animation/AnimationController.h:75
> +    void animationsForDocumentMayStart(Document*);

This name is ambiguous: it can be read as a getter, and it's not clear how it interacts with suspend/resume. I guess it's startAnimationsIfNotSuspended()?

> Source/WebCore/page/animation/CompositeAnimation.h:95
> -    bool m_suspended;
> +    bool m_isSuspended;

I'd suggest not renaming the member var.

> Source/WebKit/mac/WebView/WebView.mm:-2774
>  - (BOOL)cssAnimationsSuspended
>  {
> -    return _private->cssAnimationsSuspended;
> +    Frame* frame = core([self mainFrame]);
> +    if (frame)
> +        return frame->animation()->isSuspended();
> +
> +    return false;
>  }
>  
>  - (void)setCSSAnimationsSuspended:(BOOL)suspended
>  {
> -    if (suspended == _private->cssAnimationsSuspended)
> +    Frame* frame = core([self mainFrame]);
> +    if (suspended == frame->animation()->isSuspended())
>          return;
>          
> -    _private->cssAnimationsSuspended = suspended;
> -    
> -    Frame* frame = core([self mainFrame]);

After these changes does the suspended state survive across pages loads, which may change the Frame? I think we need to preserve that behavior.

[Dean Jackson]

Comment on attachment 198965 [details]
Patch

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

>> Source/WebCore/page/animation/AnimationBase.cpp:375
>> +
> 
> Extra blank line.

Fixed.

>> Source/WebCore/page/animation/AnimationController.cpp:269
>> +        return;
> 
> I wonder if we should make suspend/resume nestable, with a counter??

I don't want to change our API behaviour in this patch. This is a private API anyway (at the moment).

>> Source/WebCore/page/animation/AnimationController.h:75
>> +    void animationsForDocumentMayStart(Document*);
> 
> This name is ambiguous: it can be read as a getter, and it's not clear how it interacts with suspend/resume. I guess it's startAnimationsIfNotSuspended()?

Yes, fixed.

>> Source/WebCore/page/animation/CompositeAnimation.h:95
>> +    bool m_isSuspended;
> 
> I'd suggest not renaming the member var.

OK. Reverted.

>> Source/WebKit/mac/WebView/WebView.mm:-2774
>> -    Frame* frame = core([self mainFrame]);
> 
> After these changes does the suspended state survive across pages loads, which may change the Frame? I think we need to preserve that behavior.

That behaviour didn't exist. Previously, we'd just return the member variable in WebViewData, which did not necessarily reflect reality. As you say, the frame could change, or the document could restart animations, but we'd never know. At least now the API does reflect the actual value.

You're right that it might change from underneath us. However, I don't want to fix that in this patch. The few clients we have should be checking in the load delegates.

[Dean Jackson]

Created attachment 198978 [details]
Patch

[Dean Jackson]

New patch with Simon's suggestions (and ignores some of them :)

[WebKit Commit Bot]

Attachment 198978 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'LayoutTests/animations/added-while-suspended-expected.txt', u'LayoutTests/animations/added-while-suspended.html', u'LayoutTests/transitions/started-while-suspended-expected.txt', u'LayoutTests/transitions/started-while-suspended.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/page/animation/AnimationBase.cpp', u'Source/WebCore/page/animation/AnimationBase.h', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationController.h', u'Source/WebCore/page/animation/AnimationControllerPrivate.h', u'Source/WebCore/page/animation/CompositeAnimation.cpp', u'Source/WebCore/page/animation/CompositeAnimation.h', u'Source/WebCore/page/animation/KeyframeAnimation.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/ChangeLog', u'Source/WebKit/WebKit.vcxproj/WebKitExportGenerator/WebKitExports.def.in', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebView.mm', u'Source/WebKit/mac/WebView/WebViewData.h', u'Source/WebKit/mac/WebView/WebViewData.mm', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in', u'Source/autotools/symbols.filter']" exit_code: 1
Source/WebCore/page/animation/AnimationBase.h:81:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

(In reply to comment #7)
> > After these changes does the suspended state survive across pages loads, which may change the Frame? I think we need to preserve that behavior.
> 
> That behaviour didn't exist. Previously, we'd just return the member variable in WebViewData, which did not necessarily reflect reality. As you say, the frame could change, or the document could restart animations, but we'd never know. At least now the API does reflect the actual value.
> 
> You're right that it might change from underneath us. However, I don't want to fix that in this patch. The few clients we have should be checking in the load delegates.

But previously someone could do:

[webview setCSSAnimationsSuspended:YES]
...start page load

Are you saying that the animations in the loaded document would not be suspended in this case, i.e. that WebCore didn't ask the WebView for the suspended state?

With your patch (and possibly before), the situation is:

[webview setCSSAnimationsSuspended:YES]
...load a page...
animations are not suspended.

so it feels like the SPI is broken. State set on the WebView should stick. I know this is a behavior change, but seems like the right thing to fix.

[Dean Jackson]

(In reply to comment #11)
> (In reply to comment #7)
> > > After these changes does the suspended state survive across pages loads, which may change the Frame? I think we need to preserve that behavior.
> > 
> > That behaviour didn't exist. Previously, we'd just return the member variable in WebViewData, which did not necessarily reflect reality. As you say, the frame could change, or the document could restart animations, but we'd never know. At least now the API does reflect the actual value.
> > 
> > You're right that it might change from underneath us. However, I don't want to fix that in this patch. The few clients we have should be checking in the load delegates.
> 
> But previously someone could do:
> 
> [webview setCSSAnimationsSuspended:YES]
> ...start page load
> 
> Are you saying that the animations in the loaded document would not be suspended in this case, i.e. that WebCore didn't ask the WebView for the suspended state?

That is correct. WebView kept its own flag. If you changed the flag it would call into WebCore. However, WebCore was doing whatever it wanted under the hood, and never worrying what the WebView had requested.


> With your patch (and possibly before), the situation is:
> 
> [webview setCSSAnimationsSuspended:YES]
> ...load a page...
> animations are not suspended.
> 
> so it feels like the SPI is broken. State set on the WebView should stick. I know this is a behavior change, but seems like the right thing to fix.

OK.

[Build Bot]

Comment on attachment 198978 [details]
Patch

Attachment 198978 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/110857

New failing tests:
transitions/transition-end-event-rendering.html
transitions/transition-end-event-left.html
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
animations/transition-and-animation-1.html
transitions/transition-end-event-multiple-02.html
transitions/transition-end-event-attributes.html
fast/css/image-rendering.html
transitions/transition-duration-cleared-in-transitionend-crash.html
transitions/transition-end-event-multiple-03.html
transitions/svg-transitions.html
transitions/transition-end-event-multiple-01.html
animations/unanimated-style.html
transitions/transform-op-list-match.html
transitions/transition-drt-api-delay.html
transitions/transition-end-event-container.html
transitions/transform-op-list-no-match.html
animations/timing-functions.html
transitions/transition-end-event-all-properties.html
transitions/text-indent-transition.html
transitions/transition-drt-api.html
transitions/transition-end-event-destroy-renderer.html
transitions/transition-end-event-nested.html
platform/mac/fast/speechsynthesis/speech-synthesis-pause-resume.html
animations/transition-and-animation-2.html
platform/mac/editing/deleting/deletionUI-single-instance.html
animations/transition-and-animation-3.html
transitions/transition-end-event-multiple-04.html
fast/loader/javascript-url-in-object.html
transitions/transition-end-event-set-none.html
animations/width-using-ems.html

[Build Bot]

Created attachment 199220 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Dean Jackson]

(In reply to comment #12)

> > so it feels like the SPI is broken. State set on the WebView should stick. I know this is a behavior change, but seems like the right thing to fix.
> 
> OK.

It turns out that this actually is the case, so this patch is ready.

[Dean Jackson]

Committed r149576: <http://trac.webkit.org/changeset/149576>