Bug 216308

Summary:

REGRESSION (r260360): Ionic modal dialog doesn't animate correctly when dragged and released

Product:

WebKit

Reporter:

Antoine Quint <graouts>

Component:

Animations

Assignee:

Antoine Quint <graouts>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dino, ews-watchlist, graouts, ldebeasi, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=215853

Attachments:

Description	Flags
Ionic modal component demo	none
Reduction	none
Patch	none

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.