Bug 232366

Created attachment 442567 [details] Patch

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.

Comment on attachment 442567 [details] Patch r+ but please put some words in the changelog.

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.

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?

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()

Cool.

Created attachment 442633 [details] Patch

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].

<rdar://problem/84729545>