Bug 248020 – Add private WKWebView APIs to globally control animation play-state

• Component: Animations
• Status: RESOLVED FIXED

[Tyler Wilcock]

Add private iOS WKWebView API to pause all and play all animations on the page

[Tyler Wilcock]

Created attachment 463574 [details]
Patch

[Tyler Wilcock]

Created attachment 463576 [details]
Patch

[Tyler Wilcock]

Created attachment 463577 [details]
Patch

[Antoine Quint]

The title of this bug is not specific to animated images, but the patch is.

[Radar WebKit Bug Importer]

<rdar://problem/102638732>

[Tyler Wilcock]

Created attachment 463766 [details]
Patch

[Tyler Wilcock]

(In reply to Antoine Quint from comment #4)
> The title of this bug is not specific to animated images, but the patch is.
We talked about this in Slack, but posting here too for posterity.

The intention of these APIs is to pause and play all types of animations, not just animated images. So it will include SVG animations, Web Animations, model element animations, etc (the latter two are yet to be implemented). Here's a test exercising the core functionality for SVG and image animations: https://github.com/WebKit/WebKit/blob/main/LayoutTests/fast/images/pagewide-play-pause-offscreen-animations.html

[Darin Adler]

Comment on attachment 463766 [details]
Patch

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

> COMMIT_MESSAGE:1
> +Add private iOS WKWebView APIs: _pauseAllAnimations, _playAllAnimations, _isAnyAnimationAllowedToPlay
 

Why private?

[Tyler Wilcock]

(In reply to Darin Adler from comment #8)
> Comment on attachment 463766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=463766&action=review
> 
> > COMMIT_MESSAGE:1
> > +Add private iOS WKWebView APIs: _pauseAllAnimations, _playAllAnimations, _isAnyAnimationAllowedToPlay
>  
> 
> Why private?
As of now, there will only be one internal consumer. I think we could consider promoting it to a public API later on, but I figured making it private to start is a good first step.

[Darin Adler]

(In reply to Tyler Wilcock from comment #9)
> As of now, there will only be one internal consumer. I think we could
> consider promoting it to a public API later on, but I figured making it
> private to start is a good first step.

I don’t agree with this general principle. We should discuss our approach so we don’t build up an even longer list of private API.

[Said Abou-Hallawa]

Comment on attachment 463766 [details]
Patch

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

> COMMIT_MESSAGE:28
> +tests in AnimationControl.mm to pass consistently.

Can you explain why this change is needed to make the tests pass consistently?

> Source/WebCore/platform/graphics/BitmapImage.cpp:446
> +        m_animationIsRunning = !!m_frameTimer;

This is a weird place to set this flag.

> Source/WebCore/platform/graphics/BitmapImage.h:241
> +    bool m_animationIsRunning { false };

What is the meaning of this flag? And what is the difference between its value and the value of canAnimate()?

I think you are changing its meaning. It is currently true when we are displaying a frame, waiting for its interval to elapse to draw the next frame. With your change it I think it will mean whether the image is animating or still since it is not cleared in BitmapImage::clearTimer().

Also it is weird to have two flags m_animationIsRunning and m_animationFinished although each of them means the opposite to the other. How can we guarantee they are not both set to true? Actually having both of them initialized with false is confusing. If m_animationIsRunning is false then m_animationFinished should be true.

[Said Abou-Hallawa]

Comment on attachment 463766 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.h:241
>> +    bool m_animationIsRunning { false };
> 
> What is the meaning of this flag? And what is the difference between its value and the value of canAnimate()?
> 
> I think you are changing its meaning. It is currently true when we are displaying a frame, waiting for its interval to elapse to draw the next frame. With your change it I think it will mean whether the image is animating or still since it is not cleared in BitmapImage::clearTimer().
> 
> Also it is weird to have two flags m_animationIsRunning and m_animationFinished although each of them means the opposite to the other. How can we guarantee they are not both set to true? Actually having both of them initialized with false is confusing. If m_animationIsRunning is false then m_animationFinished should be true.

I meant to say you are changing the meaning of BitmapImage::isAnimating() since it is now returning m_animationIsRunning.

[James Craig]

It may  be wise to keep the new API private until CSS has a resolution on Issue #7440? https://github.com/w3c/csswg-drafts/issues/7440

[Tyler Wilcock]

Created attachment 464018 [details]
Patch

[Tyler Wilcock]

(In reply to Said Abou-Hallawa from comment #11)
> Comment on attachment 463766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=463766&action=review
> 
> > COMMIT_MESSAGE:28
> > +tests in AnimationControl.mm to pass consistently.
> 
> Can you explain why this change is needed to make the tests pass
> consistently?
> 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:446
> > +        m_animationIsRunning = !!m_frameTimer;
> 
> This is a weird place to set this flag.
> 
> > Source/WebCore/platform/graphics/BitmapImage.h:241
> > +    bool m_animationIsRunning { false };
> 
> What is the meaning of this flag? And what is the difference between its
> value and the value of canAnimate()?
> 
> I think you are changing its meaning. It is currently true when we are
> displaying a frame, waiting for its interval to elapse to draw the next
> frame. With your change it I think it will mean whether the image is
> animating or still since it is not cleared in BitmapImage::clearTimer().
> 
> Also it is weird to have two flags m_animationIsRunning and
> m_animationFinished although each of them means the opposite to the other.
> How can we guarantee they are not both set to true? Actually having both of
> them initialized with false is confusing. If m_animationIsRunning is false
> then m_animationFinished should be true.
Thanks for the insight! I was able to write the test such that the changes to BitmapImage::isAnimating are unnecessary.

[Tyler Wilcock]

Created attachment 464021 [details]
Patch

[chris fleizach]

(In reply to Tyler Wilcock from comment #16)
> Created attachment 464021 [details]
> Patch

Looks like feedback has been addressed and public API has been pended for good reasons. I'm inclined to approve this.

[Tyler Wilcock]

Created attachment 464393 [details]
Patch

[Tyler Wilcock]

Created attachment 464409 [details]
Patch

[EWS]

Committed 258671@main (c6458455e6da): <https://commits.webkit.org/258671@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 464409 [details].