Bug 236019 – Overlap computations are broken with rotate, translate, scale properties

• Status: NEW
• Component: Compositing

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Testcase needs to be dropped into LayoutTests/compositing/layer-creation

[Radar WebKit Bug Importer]

<rdar://problem/88383253>

[Antoine Quint]

Created attachment 450665 [details]
Patch

[Simon Fraser (smfr)]

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.

[Darin Adler]

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.