Bug 222545 - REGRESSION(r272004): transform transition with delay doesn't behave correctly
Summary: REGRESSION(r272004): transform transition with delay doesn't behave correctly
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: 2021-03-01 05:04 PST by Antoine Quint
Modified: 2021-03-01 11:52 PST (History)
5 users (show)

See Also:


Attachments
Test (952 bytes, text/html)
2021-03-01 05:04 PST, Antoine Quint
no flags Details
Test (657 bytes, text/html)
2021-03-01 07:51 PST, Antoine Quint
no flags Details
Patch (9.71 KB, patch)
2021-03-01 09:28 PST, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2021-03-01 05:04:33 PST
Created attachment 421811 [details]
Test

I am filing this bug based on a report made in the Apple Developer Forums at https://developer.apple.com/forums/thread/674449. The post has a test which is attached to this bug. I have managed to regress this to r272004.
Comment 1 Radar WebKit Bug Importer 2021-03-01 05:06:55 PST
<rdar://problem/74865413>
Comment 2 Antoine Quint 2021-03-01 07:51:06 PST
Created attachment 421827 [details]
Test
Comment 3 Antoine Quint 2021-03-01 07:52:06 PST
The new test shows the issue is that the scale that is applied during the initial transition is applied as an additional translate while the second transition is in its delay phase, which is made extremely long.
Comment 4 Antoine Quint 2021-03-01 07:58:25 PST
The CADebug output shows we have two animations during the second transition's delay phase:

    (keyframe-animation
      (beginTime 108545.905575)
      (duration 2.000000)
      (fillMode both)
      (repeatCount 1.000000)
      (additive true)
      (removedOnCompletion false)
      (keyPath transform)
      (calculationMode linear)
      (values (array
        (vector 3 0 0 0 0 3 0 0 0 0 1 0 0 0 0 1)
        (vector 0.6000000238 0 0 0 0 0.6000000238 0 0 0 0 1 0 -1232.400024 0 0 1)))
      (keyTimes (vector 0 1))
      (timingFunctions (vector 0.25 0.1000000015 0.25 1)))
    (basic-animation
      (beginTime 8545.905575)
      (duration 100000.000000)
      (additive true)
      (keyPath transform)
      (from (vector 3 0 0 0 0 3 0 0 0 0 1 0 0 0 0 1))
      (to (vector 3 0 0 0 0 3 0 0 0 0 1 0 0 0 0 1)))))))))))))))))))))))))))))

CA additive animations are applied in reverse order, so the keyframe-animation is the second transition, and since it has fillMode = "both" it applies on top of the basic-animation.

In this case we shouldn't have the basic-animation at all since the transitions has explicit keyframes.
Comment 5 Antoine Quint 2021-03-01 08:05:43 PST
I think the fix here is to not generate a non-interpolating "base" animation if the first interpolating animation in the stack for this transform-related property fills backwards.
Comment 6 Antoine Quint 2021-03-01 09:28:43 PST
Created attachment 421834 [details]
Patch
Comment 7 Antoine Quint 2021-03-01 10:49:01 PST
Committed r273656 (234696@main): <https://commits.webkit.org/234696@main>
Comment 8 Simon Fraser (smfr) 2021-03-01 11:45:57 PST
Comment on attachment 421834 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3134
> +            // If we have an animation with an explicit begin time that does not fill backwards and starts with a delay,
> +            // we must create a non-interpolating animation to set the current value for this transform-related property
> +            // until that animation begins.
> +            if (earliestAnimation) {
> +                auto fillMode = earliestAnimation->m_animation->fillMode();
> +                if (fillMode != PlatformCAAnimation::Backwards && fillMode != PlatformCAAnimation::Both) {
> +                    Seconds earliestBeginTime = *earliestAnimation->computedBeginTime() + animationGroupBeginTime;
> +                    if (earliestBeginTime > currentTime) {
> +                        if (auto* baseValueTransformAnimation = makeBaseValueTransformAnimation(property, TransformationMatrixSource::AskClient, earliestBeginTime)) {
> +                            prepareAnimationForAddition(*baseValueTransformAnimation);
> +                            caAnimations.append(baseValueTransformAnimation->m_animation);
> +                        }
> +                    }

If this is identical to the block above you should share it.
Comment 9 Antoine Quint 2021-03-01 11:52:51 PST
(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 421834 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421834&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3134
> > +            // If we have an animation with an explicit begin time that does not fill backwards and starts with a delay,
> > +            // we must create a non-interpolating animation to set the current value for this transform-related property
> > +            // until that animation begins.
> > +            if (earliestAnimation) {
> > +                auto fillMode = earliestAnimation->m_animation->fillMode();
> > +                if (fillMode != PlatformCAAnimation::Backwards && fillMode != PlatformCAAnimation::Both) {
> > +                    Seconds earliestBeginTime = *earliestAnimation->computedBeginTime() + animationGroupBeginTime;
> > +                    if (earliestBeginTime > currentTime) {
> > +                        if (auto* baseValueTransformAnimation = makeBaseValueTransformAnimation(property, TransformationMatrixSource::AskClient, earliestBeginTime)) {
> > +                            prepareAnimationForAddition(*baseValueTransformAnimation);
> > +                            caAnimations.append(baseValueTransformAnimation->m_animation);
> > +                        }
> > +                    }
> 
> If this is identical to the block above you should share it.

That whole IFDEF is going away Real Soon Now, so I didn't really bother too much.