Bug 187942 – [Web Animations] REGRESSION: transition added immediately after element creation doesn't work

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Tim Horton]

Created attachment 345640 [details]
repro

Steps to Reproduce:

1. With "Web Animations and CSS Integration" turned on:
2. Drag the black box.

Expected: A growing animation.
Actual: Most of the time, the box does not appear until the animation would have completed.

If the animation does run correctly, refresh the page and try again.

With "Web Animations and CSS Integration" turned off, this problem never reproduces.

Reproduces in STP 61.

[Antoine Quint]

Created attachment 345670 [details]
Reduced test case

[Antoine Quint]

Created attachment 345674 [details]
Reduced test case

[Antoine Quint]

Reduced test case shows a consistent failure.

[Antoine Quint]

The computed keyframes after creating the transitions shows that we think we're animating the "transform" property from "none" to "none", ignoring the "scale(0)" initial value.

Animating "opacity" instead from an initial 0 to the default 1 computes the expected keyframes but does not run the animation.

Finally, animating "margin-left" from an initial "100px" to the default "0" computes the expected keyframes _and_ runs the animation.

[Antoine Quint]

getKeyframes() reporting that we're animating from "none" to "none" is a bug of its own, we actually have a scale(0) operation for the from keyframe. So I think this bug is really about an initial transition of a hardware-accelerated property, so looking at the opacity case should get to the bottom of the issue.

[Antoine Quint]

Using a CSS Animation or Web Animation does not reproduce the issue either, so it's specific to CSS Transitions.

[Antoine Quint]

The animation doesn't start because the renderer says it's not composited when entering KeyframeEffectReadOnly::updateAcceleratedAnimationState(). There must be something that the codepath used to generate blending keyframes for CSS Animations and Web Animations does that the one used for CSS Transitions doesn't do.

[Antoine Quint]

Actually doing what the spec says and setting "will-change" to have all the animated properties in KeyframeEffectReadOnly::apply() does the trick.

[Antoine Quint]

Created attachment 345786 [details]
Patch

[Dean Jackson]

Comment on attachment 345786 [details]
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:433
> +        }

Don't you want an else continue here? You continue from the inner loop but would just loop through the remainder of the animations. unless you need to resolve the style for all of them, but i don't think so.

> LayoutTests/webanimations/accelerated-transition-by-removing-property.html:22
> +setTimeout(() => {
> +    target.style.removeProperty("transform");
> +    target.style.transition = "transform 10s linear";
> +    setTimeout(() => {

You should add a test that has multiple animations, only one of which is hardware composited.

[Antoine Quint]

(In reply to Dean Jackson from comment #10)
> Comment on attachment 345786 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345786&action=review
> 
> > Source/WebCore/animation/DocumentTimeline.cpp:433
> > +        }
> 
> Don't you want an else continue here? You continue from the inner loop but
> would just loop through the remainder of the animations. unless you need to
> resolve the style for all of them, but i don't think so.

We do want to resolve the styles for all of them, but we can stop looking for accelerated animations once we've found that at least one isn't.

> > LayoutTests/webanimations/accelerated-transition-by-removing-property.html:22
> > +setTimeout(() => {
> > +    target.style.removeProperty("transform");
> > +    target.style.transition = "transform 10s linear";
> > +    setTimeout(() => {
> 
> You should add a test that has multiple animations, only one of which is
> hardware composited.

Good point, will add such a test which will only test there are no layers.

[Antoine Quint]

Committed r234250: <https://trac.webkit.org/changeset/234250>

[Radar WebKit Bug Importer]

<rdar://problem/42616207>

[Simon Fraser (smfr)]

Comment on attachment 345786 [details]
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:421
> +            if (auto* effect = animation->effect()) {
> +                if (is<KeyframeEffectReadOnly>(effect)) {

is<> is null-safe so you can write this as if (is<KeyframeEffectReadOnly>(animation->effect()))

> Source/WebCore/animation/DocumentTimeline.cpp:422
> +                    auto* keyframeEffect = downcast<KeyframeEffectReadOnly>(effect);

If you've already null-checked it's better to get a reference:
auto& keyframeEffect = downcast<KeyframeEffectReadOnly>(*effect);

> Source/WebCore/animation/DocumentTimeline.cpp:425
> +                            hasNonAcceleratedAnimations = true;

So much nesting.

> Source/WebCore/animation/DocumentTimeline.cpp:429
> +                        if (!hasPendingAcceleratedAnimations)
> +                            hasPendingAcceleratedAnimations = keyframeEffect->hasPendingAcceleratedAction();

hasPendingAcceleratedAnimations |= keyframeEffect->hasPendingAcceleratedAction()

> Source/WebCore/animation/DocumentTimeline.cpp:438
> +    return !hasNonAcceleratedAnimations && hasPendingAcceleratedAnimations;

The !hasNonAcceleratedAnimations is a double negative and hurts my head.