[Web Animations] Use running animations unaniamted style when considering new CSS Transitions
<rdar://problem/59458111>
Created attachment 390767 [details] Patch
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.
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.
(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.
Committed r256627: <https://trac.webkit.org/changeset/256627>
*** Bug 203081 has been marked as a duplicate of this bug. ***