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.
Created attachment 345670 [details] Reduced test case
Created attachment 345674 [details] Reduced test case
Reduced test case shows a consistent failure.
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.
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.
Using a CSS Animation or Web Animation does not reproduce the issue either, so it's specific to CSS Transitions.
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.
Actually doing what the spec says and setting "will-change" to have all the animated properties in KeyframeEffectReadOnly::apply() does the trick.
Created attachment 345786 [details] Patch
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.
(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.
Committed r234250: <https://trac.webkit.org/changeset/234250>
<rdar://problem/42616207>
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.