Created attachment 450652 [details] Testcase See attached testcase. Gray dots should always overlap the blue. We must fail to run KeyframeEffect::computeExtentOfTransformAnimation() code for rotate, translate, scale properties.
Testcase needs to be dropped into LayoutTests/compositing/layer-creation
<rdar://problem/88383253>
Created attachment 450665 [details] Patch
Comment on attachment 450665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450665&action=review > Source/WebCore/animation/DocumentTimeline.cpp:356 > + result = result || effect->computeExtentOfTransformAnimation(bounds); You need to be careful: the || operator can short-circuit. > Source/WebCore/animation/KeyframeEffect.cpp:1954 > + if (onlyAnimatesTransformProperty && transformFunctionListsMatch()) > canCompute = computeTransformedExtentViaTransformList(rendererBox, *style, keyframeBounds); > else > canCompute = computeTransformedExtentViaMatrix(rendererBox, *style, keyframeBounds); Does the right thing happen if an element is running, say, both a scale and transform operation? The bounds need to be computed by applying their effects in the correct order (taking transform-origin etc into account). I think we need some more tests to verify that this works correctly. > LayoutTests/compositing/layer-creation/translate-scale-individual-transform-properties-animation-overlap.html:14 > + -webkit-transform-origin: 10px bottom; > + transition: -webkit-transform 10s; not sure why we have these and the keyframe animation. > LayoutTests/compositing/layer-creation/translate-scale-individual-transform-properties-animation-overlap.html:44 > + if (window.testRunner) { > + testRunner.dumpAsText(); > + testRunner.waitUntilDone(); The cool kids are using a new syntax.
Comment on attachment 450665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450665&action=review Seems like this needs a a bit more improvement to address Simon’s feedback on tests to make sure we cover more complex cases correctly. > Source/WebCore/animation/DocumentTimeline.cpp:351 > + auto& animatedProperties = effect->animatedProperties(); In this narrow context could use a one word name, properties. > Source/WebCore/animation/DocumentTimeline.cpp:352 > + if (animatedProperties.contains(CSSPropertyTransform) All these separate calls to contains are awkward. Consider refactoring that make it easier to see this is correct. Maybe just add a contains that takes a Span and pass them all with { }. Could also have this list of property names be a names constexpr array we can use over and over again. Or a function that returns a span over that kind of array. >> Source/WebCore/animation/DocumentTimeline.cpp:356 >> + result = result || effect->computeExtentOfTransformAnimation(bounds); > > You need to be careful: the || operator can short-circuit. As Simon says, seems highly likely this is wrong. If we need to call computeExtentOfTransformAnimation on all the effects then it needs to be outside the || context. Would be good to add a test case complex enough to show this. > Source/WebCore/animation/DocumentTimeline.cpp:380 > + if (keyframeEffect->isCurrentlyAffectingProperty(CSSPropertyTransform, KeyframeEffect::Accelerated::Yes) Since we see the same set of properties here the same considerations as above apply. Find a way to have the code easier to read and clearer we didn’t it’s forget one property. > Source/WebCore/animation/KeyframeEffect.cpp:1921 > + ASSERT(m_blendingKeyframes.containsProperty(CSSPropertyTransform) And the pattern recurs again here.