Bug 207760 – [Web Animations] Style changes due to Web Animations should not trigger CSS Transitions

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

[Web Animations] Use running animations unaniamted style when considering new CSS Transitions

[Radar WebKit Bug Importer]

<rdar://problem/59458111>

[Antoine Quint]

Created attachment 390767 [details]
Patch

[Antoine Quint]

Note that the patch for review is _not_ expected to build since it depends on the one for bug 207629 and bug 207364. We can retry it once it lands, but the "WIP Patch" for bug 207364 contained all of the code and showed positive bot results.

[Simon Fraser (smfr)]

Comment on attachment 390767 [details]
Patch

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

> LayoutTests/ChangeLog:3
> +        [Web Animations] Use running animations unanimated style when considering new CSS Transitions

Should this be "Use running animation's unanimated style when considering new CSS Transitions"? It's a bit hard to parse.

I think it's more like "Style changes due to Web Animations should not trigger transitions".

> Source/WebCore/animation/KeyframeEffect.cpp:786
> +bool KeyframeEffect::animatesProperty(CSSPropertyID property) const
> +{
> +    if (!m_blendingKeyframes.isEmpty())
> +        return m_blendingKeyframes.properties().contains(property);
> +
> +    for (auto& keyframe : m_parsedKeyframes) {
> +        for (auto keyframeProperty : keyframe.unparsedStyle.keys()) {
> +            if (keyframeProperty == property)
> +                return true;
> +        }
> +    }
> +    return false;
> +}

I wonder if this will become a perf bottleneck. People do make keyframe animations with hundreds of keyframes.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 390767 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390767&action=review
> 
> > LayoutTests/ChangeLog:3
> > +        [Web Animations] Use running animations unanimated style when considering new CSS Transitions
> 
> Should this be "Use running animation's unanimated style when considering
> new CSS Transitions"? It's a bit hard to parse.
> 
> I think it's more like "Style changes due to Web Animations should not
> trigger transitions".

Much better.

> > Source/WebCore/animation/KeyframeEffect.cpp:786
> > +bool KeyframeEffect::animatesProperty(CSSPropertyID property) const
> > +{
> > +    if (!m_blendingKeyframes.isEmpty())
> > +        return m_blendingKeyframes.properties().contains(property);
> > +
> > +    for (auto& keyframe : m_parsedKeyframes) {
> > +        for (auto keyframeProperty : keyframe.unparsedStyle.keys()) {
> > +            if (keyframeProperty == property)
> > +                return true;
> > +        }
> > +    }
> > +    return false;
> > +}
> 
> I wonder if this will become a perf bottleneck. People do make keyframe
> animations with hundreds of keyframes.

Some more considerations:

    1. We'll stop iterating as soon as we find the opacity in one keyframe, hopefully sooner than later, good chance we'll find it right on the first keyframe
    2. This only happens for cases where both a transition and a running animation for the same property are specified
    3. On top of the previous point, this only happens if the running animation has _not_ yet been applied, at any further point the blending keyframes will be resolved and we take the faster path
    4. Finally this only applies to JS-created Web Animations, since CSS Animations have their unanimated style set right away since they're created during style resolution

I don't think all of those points are likely to happen all at once outside of specific testing situations, but I could be wrong. If that happens, we can probably find a way to improve on this.

[Antoine Quint]

Committed r256627: <https://trac.webkit.org/changeset/256627>

[Antoine Quint]

*** Bug 203081 has been marked as a duplicate of this bug. ***