Bug 183291 – NSAnimation is not working in the WebContent process when WindowServer access is blocked.

• Component: WebKit2
• Status: NEW

[Per Arne Vollan]

The animation callback 'setCurrentProgress' is never called.

[Radar WebKit Bug Importer]

<rdar://problem/38070914>

[Per Arne Vollan]

Created attachment 334900 [details]
Patch

[Per Arne Vollan]

Created attachment 334901 [details]
Patch

[Anders Carlsson]

Using a timer here gets rid of the animation curve. I don't think that's what you intended to do.

[Tim Horton]

Comment on attachment 334900 [details]
Patch

What Anders said.

[Per Arne Vollan]

Created attachment 335010 [details]
Patch

[Per Arne Vollan]

(In reply to Tim Horton from comment #5)
> Comment on attachment 334900 [details]
> Patch
> 
> What Anders said.

I have created a patch where the animation curve is based on a sin formula.

Thanks for reviewing, all!

[Dean Jackson]

Comment on attachment 335010 [details]
Patch

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

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:382
> +        // Use a formula based on sin to create an animation curve where the opacity rate of change is low
> +        // in the beginning and at the end of the animation, but higher in the middle of the animation.
> +        // This should be similar to the 'NSAnimationEaseInOut' animation curve.
> +        auto t = elapsed / _duration;
> +        progress = 0.5 * sin(M_PI * t - M_PI / 2) + 0.5;

Is it? We have code in WebKit for CA's ease-in-out curve. You could use that instead.

[Per Arne Vollan]

(In reply to Dean Jackson from comment #8)
> Comment on attachment 335010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335010&action=review
> 
> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:382
> > +        // Use a formula based on sin to create an animation curve where the opacity rate of change is low
> > +        // in the beginning and at the end of the animation, but higher in the middle of the animation.
> > +        // This should be similar to the 'NSAnimationEaseInOut' animation curve.
> > +        auto t = elapsed / _duration;
> > +        progress = 0.5 * sin(M_PI * t - M_PI / 2) + 0.5;
> 
> Is it? We have code in WebKit for CA's ease-in-out curve. You could use that
> instead.

Thanks! That is a good point. I was planning on implementing the scrollbar animation using CA layer animations as suggested earlier. The track and thumb of the scrollbar needs to be animated individually, but this might not be trivial with the CA layer approach, since the thumb and track then would need to be rendered in separate layers, I believe.

[Per Arne Vollan]

Created attachment 335160 [details]
Patch

[Per Arne Vollan]

Created attachment 335161 [details]
Patch

[EWS Watchlist]

Comment on attachment 335161 [details]
Patch

Attachment 335161 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6835370

New failing tests:
http/wpt/resource-timing/rt-initiatorType-media.html

[EWS Watchlist]

Created attachment 335172 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dean Jackson]

Comment on attachment 335160 [details]
Patch

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

I'm giving this an r+ but I think it would be better if we got the animation data from AppKit rather than simulate it ourselves. That way, if/when AppKit decides to tweak the animation function, we remain in sync.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:382
> +        auto t = elapsed / _duration;

We should probably have a guarantee that _duration is never 0.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:418
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101400
>      [self stopAnimation];
> +#else

It's a bit confusing that this calls stopAnimation on older OS versions, but you implement a separate stopAnimation call on newer versions.

[Per Arne Vollan]

(In reply to Dean Jackson from comment #14)
> Comment on attachment 335160 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335160&action=review
> 
> I'm giving this an r+ but I think it would be better if we got the animation
> data from AppKit rather than simulate it ourselves. That way, if/when AppKit
> decides to tweak the animation function, we remain in sync.
> 

Yes, I completely agree. I think there also is an agreement on that it would be better to use CA layer animation to do this, but since we need to individually animate the track and thumb of the scrollbar, this is more work. But, we should look further into this.

> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:382
> > +        auto t = elapsed / _duration;
> 
> We should probably have a guarantee that _duration is never 0.
> 

Good catch! I will fix that.

> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:418
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101400
> >      [self stopAnimation];
> > +#else
> 
> It's a bit confusing that this calls stopAnimation on older OS versions, but
> you implement a separate stopAnimation call on newer versions.

Yes, definitely, I will fix this before landing. Thanks for reviewing!

[Per Arne Vollan]

Created attachment 335316 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 335316 [details]
Patch

Clearing flags on attachment: 335316

Committed r229419: <https://trac.webkit.org/changeset/229419>