Bug 183291 - NSAnimation is not working in the WebContent process when WindowServer access is blocked.
Summary: NSAnimation is not working in the WebContent process when WindowServer access...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-02 09:27 PST by Per Arne Vollan
Modified: 2019-09-15 11:16 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.17 KB, patch)
2018-03-02 09:32 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2018-03-02 09:34 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.86 KB, patch)
2018-03-05 11:42 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.74 KB, patch)
2018-03-06 17:04 PST, Per Arne Vollan
dino: review+
Details | Formatted Diff | Diff
Patch (5.74 KB, patch)
2018-03-06 17:06 PST, Per Arne Vollan
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (11.98 MB, application/zip)
2018-03-06 22:24 PST, EWS Watchlist
no flags Details
Patch (5.56 KB, patch)
2018-03-08 10:31 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-03-02 09:27:29 PST
The animation callback 'setCurrentProgress' is never called.
Comment 1 Radar WebKit Bug Importer 2018-03-02 09:28:09 PST
<rdar://problem/38070914>
Comment 2 Per Arne Vollan 2018-03-02 09:32:43 PST
Created attachment 334900 [details]
Patch
Comment 3 Per Arne Vollan 2018-03-02 09:34:37 PST
Created attachment 334901 [details]
Patch
Comment 4 Anders Carlsson 2018-03-02 13:38:38 PST
Using a timer here gets rid of the animation curve. I don't think that's what you intended to do.
Comment 5 Tim Horton 2018-03-02 13:43:51 PST
Comment on attachment 334900 [details]
Patch

What Anders said.
Comment 6 Per Arne Vollan 2018-03-05 11:42:23 PST
Created attachment 335010 [details]
Patch
Comment 7 Per Arne Vollan 2018-03-05 11:43:33 PST
(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!
Comment 8 Dean Jackson 2018-03-06 13:15:56 PST
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.
Comment 9 Per Arne Vollan 2018-03-06 13:38:58 PST
(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.
Comment 10 Per Arne Vollan 2018-03-06 17:04:45 PST
Created attachment 335160 [details]
Patch
Comment 11 Per Arne Vollan 2018-03-06 17:06:05 PST
Created attachment 335161 [details]
Patch
Comment 12 EWS Watchlist 2018-03-06 22:24:18 PST
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
Comment 13 EWS Watchlist 2018-03-06 22:24:29 PST
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
Comment 14 Dean Jackson 2018-03-08 09:40:42 PST
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.
Comment 15 Per Arne Vollan 2018-03-08 10:10:11 PST
(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!
Comment 16 Per Arne Vollan 2018-03-08 10:31:10 PST
Created attachment 335316 [details]
Patch
Comment 17 WebKit Commit Bot 2018-03-08 11:12:49 PST
Comment on attachment 335316 [details]
Patch

Clearing flags on attachment: 335316

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