Bug 114915

Summary: Animations and Transitions should not start when globally suspended
Product: WebKit Reporter: Dean Jackson <dino>
Component: CSSAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, buildbot, commit-queue, dstockwell, esprehn+autocc, rniwa, simon.fraser, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
sam: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion none

Description Dean Jackson 2013-04-21 08:06:57 PDT
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)
Comment 1 Dean Jackson 2013-04-21 08:07:12 PDT
<rdar://problem/13088947>
Comment 2 Dean Jackson 2013-04-21 08:42:21 PDT
Created attachment 198965 [details]
Patch
Comment 3 WebKit Commit Bot 2013-04-21 08:44:53 PDT
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.
Comment 4 Dean Jackson 2013-04-21 08:46:34 PDT
(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.
Comment 5 Build Bot 2013-04-21 09:08:14 PDT
Comment on attachment 198965 [details]
Patch

Attachment 198965 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/181035
Comment 6 Simon Fraser (smfr) 2013-04-21 10:54:21 PDT
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.
Comment 7 Dean Jackson 2013-04-21 16:53:04 PDT
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.
Comment 8 Dean Jackson 2013-04-21 17:09:33 PDT
Created attachment 198978 [details]
Patch
Comment 9 Dean Jackson 2013-04-21 17:10:21 PDT
New patch with Simon's suggestions (and ignores some of them :)
Comment 10 WebKit Commit Bot 2013-04-21 17:11:15 PDT
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.
Comment 11 Simon Fraser (smfr) 2013-04-22 09:29:09 PDT
(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.
Comment 12 Dean Jackson 2013-04-22 14:00:42 PDT
(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.
Comment 13 Build Bot 2013-04-23 05:27:23 PDT
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
Comment 14 Build Bot 2013-04-23 05:27:25 PDT
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
Comment 15 Dean Jackson 2013-05-04 19:25:37 PDT
(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.
Comment 16 Dean Jackson 2013-05-04 21:42:52 PDT
Committed r149576: <http://trac.webkit.org/changeset/149576>