Bug 232366 – Add discrete animation support between PathOperations

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Kiet Ho]

Add discrete animation support between two PathOperations

[Kiet Ho]

Created attachment 442567 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 442567 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Ideally there would be a description of the change here.

[Simon Fraser (smfr)]

Comment on attachment 442567 [details]
Patch

r+ but please put some words in the changelog.

[Myles C. Maxfield]

Comment on attachment 442567 [details]
Patch

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

r=me assuming all CSS animations hitting this codepath expect this behavior.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:274
>  static inline RefPtr<PathOperation> blendFunc(PathOperation* from, PathOperation* to, const CSSPropertyBlendingContext& context)

Is this changing the behavior of all path animations, not just motion-path? (For example, will clip-path be affected by this change?)

If so, are we confident that all CSS animations which will hit this codepath have the 50% swap behavior? The old behavior seemed pretty deliberate in its implementation of a 100% swap behavior.

[Myles C. Maxfield]

Comment on attachment 442567 [details]
Patch

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

> Source/WebCore/animation/CSSPropertyAnimation.cpp:1027
> +    bool canInterpolate(const RenderStyle& from, const RenderStyle& to, CompositeOperation) const override

I don't see this called anywhere in this patch. Where is it called from?

[Kiet Ho]

Comment on attachment 442567 [details]
Patch

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

>> Source/WebCore/ChangeLog:7
>> +
> 
> Ideally there would be a description of the change here.

Will add, I thought this patch is small enough that just the bug title is enough.

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:274
>>  static inline RefPtr<PathOperation> blendFunc(PathOperation* from, PathOperation* to, const CSSPropertyBlendingContext& context)
> 
> Is this changing the behavior of all path animations, not just motion-path? (For example, will clip-path be affected by this change?)
> 
> If so, are we confident that all CSS animations which will hit this codepath have the 50% swap behavior? The old behavior seemed pretty deliberate in its implementation of a 100% swap behavior.

This will change the behavior of all path animations, including clip-path. This patch fixes a bunch of WPT clip-path interpolation test, so I'm confident this is correct. I talked to Antoine about this, and he said that "This code predates any notion of discrete animations, so yeah that function probably needs to be updated to account for this."

>> Source/WebCore/animation/CSSPropertyAnimation.cpp:1027
>> +    bool canInterpolate(const RenderStyle& from, const RenderStyle& to, CompositeOperation) const override
> 
> I don't see this called anywhere in this patch. Where is it called from?

This overrides AnimationPropertyWrapperBase::canInterpolate, which is called by CSSPropertyAnimation::canPropertyBeInterpolated, which is called by updateCSSTransitionsForStyleableAndProperty. CSS Transition uses canInterpolate to figure out if it's possible to have a transition or not. I quote Antoine below:

> this is where it is specified that when blending of two values is discrete no CSS Transition can be created, in Section 3 Starting of Transitions:
> > When comparing the before-change style and after-change style for a given property, the property values are transitionable if they have an animation type that is neither not animatable nor discrete.
> We implement this in updateCSSTransitionsForStyleableAndProperty() in Styleable.cpp with a call to CSSPropertyAnimation::canPropertyBeInterpolated()

[Myles C. Maxfield]

Cool.

[Kiet Ho]

Created attachment 442633 [details]
Patch

[EWS]

Committed r284961 (243613@main): <https://commits.webkit.org/243613@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442633 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84729545>