Bug 216308 - REGRESSION (r260360): Ionic modal dialog doesn't animate correctly when dragged and released
Summary: REGRESSION (r260360): Ionic modal dialog doesn't animate correctly when dragg...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-09 05:41 PDT by Antoine Quint
Modified: 2020-09-10 10:48 PDT (History)
6 users (show)

See Also:


Attachments
Ionic modal component demo (1.86 KB, text/html)
2020-09-10 02:06 PDT, Antoine Quint
no flags Details
Reduction (497 bytes, text/html)
2020-09-10 07:37 PDT, Antoine Quint
no flags Details
Patch (6.54 KB, patch)
2020-09-10 08:16 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2020-09-09 05:41:22 PDT
This is a followup to bug 215853. Steps to reproduce:

1. Go to https://ionicframework.com/docs
2. In the iOS device on the right, scroll down to "Modal" and select that row
3. Click on "Open Modal"
4. Drag down a bit on the modal's title bar
5. Release it

Prior to r260360 this would animate back nicely but now it no longer does that.
Comment 1 Radar WebKit Bug Importer 2020-09-09 05:42:03 PDT
<rdar://problem/68567444>
Comment 2 Antoine Quint 2020-09-09 06:06:14 PDT
Note that with r260360 the animations are generally broken, due to the easing functions not being applied correctly. This was addressed by r263466 but did not revert the behavior to what was expected and was correct pre-r260360.
Comment 3 Antoine Quint 2020-09-10 02:06:47 PDT
Created attachment 408423 [details]
Ionic modal component demo

Attached a demo of the modal component in isolation to help debug.
Comment 4 Antoine Quint 2020-09-10 03:38:29 PDT
The method called on the Ionic Animation object as the gesture ends is progressEnd(0, 0.45330467732456015, 500). I assume we attempt a 500ms animation started from a 0.45 point and we're not starting it from the provided progress but from the beginning which is what it looks like.
Comment 5 Antoine Quint 2020-09-10 03:41:10 PDT
Actually, the 0 probably means to play to the start value, so play the animation in reverse.
Comment 6 Antoine Quint 2020-09-10 07:37:41 PDT
Created attachment 408442 [details]
Reduction

Attached what I think is a reduction of the problem. In this code we run an animation for 1s with a linear easing and then update the timing at mid-way to change the easing to a bezier-curve and change the "direction" property. I think we fail to account for the "direction" when we set the current time of the accelerated animation.
Comment 7 Antoine Quint 2020-09-10 08:08:36 PDT
Yes, the reduction is correct, I have a fix that addresses it and the Ionic demo.
Comment 8 Antoine Quint 2020-09-10 08:16:50 PDT
Created attachment 408446 [details]
Patch
Comment 9 Simon Fraser (smfr) 2020-09-10 10:06:39 PDT
Comment on attachment 408446 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1032
> +    if (anim->playbackRate() != 1 || !anim->directionIsForwards())
>          return false;

Why don't we just adjust the timings given to CA so we can still accelerate these?
Comment 10 EWS 2020-09-10 10:31:44 PDT
Committed r266834: <https://trac.webkit.org/changeset/266834>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408446 [details].
Comment 11 Antoine Quint 2020-09-10 10:48:49 PDT
(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 408446 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408446&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1032
> > +    if (anim->playbackRate() != 1 || !anim->directionIsForwards())
> >          return false;
> 
> Why don't we just adjust the timings given to CA so we can still accelerate
> these?

We could. However, my plan is to address both the case of non-1 playback rates and directions in one fell swoop with CAAnimationGroup. See bug 211839 for details.