Bug 183558 – [Web Animations] Create, update and remove CSSAnimation and CSSTransition objects on the DocumentTimeline

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Animations

[Antoine Quint]

[Web Animations] Create, update and remove CSSAnimation and CSSTransition objects on the DocumentTimeline

[Antoine Quint]

Created attachment 335572 [details]
Patch

[Dean Jackson]

Comment on attachment 335572 [details]
Patch

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

> Source/WebCore/animation/AnimationTimeline.cpp:97
>          return Vector<RefPtr<WebAnimation>>();

Vector<RefPtr<WebAnimation>> { }

> Source/WebCore/animation/AnimationTimeline.cpp:148
> +        return HashMap<String, RefPtr<CSSAnimation>>();

use { } syntax

> Source/WebCore/animation/AnimationTimeline.cpp:159
> +            // We've found the name of this animation in our list of previous animations, this means we've already
> +            // created a CSSAnimation object for it and need to ensure that this CSSAnimation is backed by the current
> +            // animation object for this animation name.
> +            if (namesOfPreviousAnimations.contains(name))
> +                cssAnimationsByName.get(name)->setBackingAnimation(currentAnimation);

I think you should change AnimationList to export Ref<Animation>. Also I don't think setBackingAnimation needs to take a const, which would avoid the const_cast in the previous patch.

> Source/WebCore/animation/AnimationTimeline.cpp:161
> +            // Otherwise we are dealing with a new animation name and must create a CSSAnimation for it.
> +            else if (currentAnimation.isValidAnimation())

We usually put the comment inside the if, even if it means using { }

> Source/WebCore/animation/AnimationTimeline.cpp:208
> +        return HashMap<CSSPropertyID, RefPtr<CSSTransition>>();

{ }

(I even wonder if you need to define the type, since it is inferred.)

> Source/WebCore/animation/AnimationTimeline.h:89
> +    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToAnimationsMap;
> +    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap;
> +    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap;

I'm not sure if my advice to use Element* was good, but we'll see :)

> Source/WebCore/animation/DocumentTimeline.cpp:166
> +    if (m_document && (!elementToAnimationsMap().isEmpty() || !elementToCSSAnimationsMap().isEmpty() || !elementToCSSTransitionsMap().isEmpty())) {

Make a hasAnimations() helper on AnimationTimeline

> Source/WebCore/style/StyleTreeResolver.cpp:333
> +    shouldRecompositeLayer = element.styleResolutionShouldRecompositeLayer() || shouldRecompositeLayer;

use |= ?

> LayoutTests/webanimations/css-animations.html:26
> +    test(() => {

I like _ instead of ()

> LayoutTests/webanimations/css-animations.html:44
> +    const animation = target.getAnimations()[0];

So does getAnimations force a style recalc?

> LayoutTests/webanimations/css-animations.html:46
> +    assert_true(animation instanceof CSSAnimation, "The animation is a CSSAnimation.");
> +    assert_true(animation instanceof Animation, "The animation is an Animation.");

Should be the other way around

> LayoutTests/webanimations/css-animations.html:66
> +    assert_equals(computedTiming.currentIteration, 1, "The animations's computed timing currentIteration property matches the properties set by CSS");

I assume this means the 2nd iteration?

> LayoutTests/webanimations/css-animations.html:71
> +    assert_equals(computedTiming.iterationStart, 0, "The animations's computed timing iterationStart property matches the properties set by CSS");

Not sure what this means

> LayoutTests/webanimations/css-animations.html:74
> +    assert_equals(computedTiming.progress, 0.5, "The animations's computed timing progress property matches the properties set by CSS");

Huh? Is this progress within the iteration?

> LayoutTests/webanimations/css-animations.html:81
> +    assert_equals(keyframes[0].offset, 0, "The animation's effect's first keyframe has a 0 offset.");

Lucky we don't have a CSS property called offset.

> LayoutTests/webanimations/css-animations.html:98
> +targetTest(target => {
> +    target.style.animationDelay = "-1s";
> +    target.style.animationDuration = "2s";
> +    target.style.animationName = "animation";
> +
> +    assert_equals(target.getAnimations()[0].effect.timing.delay, -1000, "The animation's delay matches the initial animation-delay property.");
> +
> +    target.style.animationDelay = 0;
> +    assert_equals(target.getAnimations()[0].effect.timing.delay, 0, "The animation's delay matches the updated animation-delay property.");
> +}, "Web Animations should reflect the animation-delay property.");

Didn't you just test this?

> LayoutTests/webanimations/css-animations.html:144
> +    assert_equals(target.getAnimations()[0].effect.timing.iterations, 1, "The animation's duration matches the updated animation-iteration-count property.");

Add one like this too:

 189    assert_equals(target.getAnimations()[0], initialAnimation, "The animation object remained the same instance throughout the test.");

> LayoutTests/webanimations/css-animations.html:172
> +    assert_not_equals(updatedAnimation, initialAnimation, "Changing the animation-name property generates a different CSSAnimation object.");

You should test this right after getting updatedAnimation

[Antoine Quint]

(In reply to Dean Jackson from comment #2)
> Comment on attachment 335572 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335572&action=review
> 
> > Source/WebCore/animation/AnimationTimeline.cpp:97
> >          return Vector<RefPtr<WebAnimation>>();
> 
> Vector<RefPtr<WebAnimation>> { }

Will fix.

> > Source/WebCore/animation/AnimationTimeline.cpp:148
> > +        return HashMap<String, RefPtr<CSSAnimation>>();
> 
> use { } syntax

Will fix.

> > Source/WebCore/animation/AnimationTimeline.cpp:159
> > +            // We've found the name of this animation in our list of previous animations, this means we've already
> > +            // created a CSSAnimation object for it and need to ensure that this CSSAnimation is backed by the current
> > +            // animation object for this animation name.
> > +            if (namesOfPreviousAnimations.contains(name))
> > +                cssAnimationsByName.get(name)->setBackingAnimation(currentAnimation);
> 
> I think you should change AnimationList to export Ref<Animation>. Also I
> don't think setBackingAnimation needs to take a const, which would avoid the
> const_cast in the previous patch.

Right. I'd like to do this as a separate task. AnimationList is most likely due for a good amount of updates.

> > Source/WebCore/animation/AnimationTimeline.cpp:161
> > +            // Otherwise we are dealing with a new animation name and must create a CSSAnimation for it.
> > +            else if (currentAnimation.isValidAnimation())
> 
> We usually put the comment inside the if, even if it means using { }

Will fix.

> > Source/WebCore/animation/AnimationTimeline.cpp:208
> > +        return HashMap<CSSPropertyID, RefPtr<CSSTransition>>();
> 
> { }
> 
> (I even wonder if you need to define the type, since it is inferred.)

I tried, but we do (error: cannot deduce lambda return type from initializer list).

> > Source/WebCore/animation/AnimationTimeline.h:89
> > +    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToAnimationsMap;
> > +    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSAnimationsMap;
> > +    HashMap<Element*, Vector<RefPtr<WebAnimation>>> m_elementToCSSTransitionsMap;
> 
> I'm not sure if my advice to use Element* was good, but we'll see :)

:)

> > Source/WebCore/animation/DocumentTimeline.cpp:166
> > +    if (m_document && (!elementToAnimationsMap().isEmpty() || !elementToCSSAnimationsMap().isEmpty() || !elementToCSSTransitionsMap().isEmpty())) {
> 
> Make a hasAnimations() helper on AnimationTimeline

Since these are specific to elements, I'm adding this protected method:

    bool hasElementAnimations() const { return !m_elementToAnimationsMap.isEmpty() || !m_elementToCSSAnimationsMap.isEmpty() || !m_elementToCSSTransitionsMap.isEmpty(); }

> > Source/WebCore/style/StyleTreeResolver.cpp:333
> > +    shouldRecompositeLayer = element.styleResolutionShouldRecompositeLayer() || shouldRecompositeLayer;
> 
> use |= ?

Will fix.

> > LayoutTests/webanimations/css-animations.html:26
> > +    test(() => {
> 
> I like _ instead of ()

Noted, but I don't :)

> > LayoutTests/webanimations/css-animations.html:44
> > +    const animation = target.getAnimations()[0];
> 
> So does getAnimations force a style recalc?

Yes, it calls updateStyleIfNeeded() on the document to ensure all pending CSS updates are accounted for before returning the current list of qualifying animations.

> > LayoutTests/webanimations/css-animations.html:46
> > +    assert_true(animation instanceof CSSAnimation, "The animation is a CSSAnimation.");
> > +    assert_true(animation instanceof Animation, "The animation is an Animation.");
> 
> Should be the other way around

Will fix.

> > LayoutTests/webanimations/css-animations.html:66
> > +    assert_equals(computedTiming.currentIteration, 1, "The animations's computed timing currentIteration property matches the properties set by CSS");
> 
> I assume this means the 2nd iteration?

That's right, it's 0-based.

> > LayoutTests/webanimations/css-animations.html:74
> > +    assert_equals(computedTiming.progress, 0.5, "The animations's computed timing progress property matches the properties set by CSS");
> 
> Huh? Is this progress within the iteration?

Correct.

> > LayoutTests/webanimations/css-animations.html:81
> > +    assert_equals(keyframes[0].offset, 0, "The animation's effect's first keyframe has a 0 offset.");
> 
> Lucky we don't have a CSS property called offset.

:)

The spec says that the "offset" CSS property is referred to as cssOffset when in JS objects, so at least it prepared for that.

> > LayoutTests/webanimations/css-animations.html:98
> > +targetTest(target => {
> > +    target.style.animationDelay = "-1s";
> > +    target.style.animationDuration = "2s";
> > +    target.style.animationName = "animation";
> > +
> > +    assert_equals(target.getAnimations()[0].effect.timing.delay, -1000, "The animation's delay matches the initial animation-delay property.");
> > +
> > +    target.style.animationDelay = 0;
> > +    assert_equals(target.getAnimations()[0].effect.timing.delay, 0, "The animation's delay matches the updated animation-delay property.");
> > +}, "Web Animations should reflect the animation-delay property.");
> 
> Didn't you just test this?

We just tested the initial value. I suppose there's some redundancy here.

> > LayoutTests/webanimations/css-animations.html:144
> > +    assert_equals(target.getAnimations()[0].effect.timing.iterations, 1, "The animation's duration matches the updated animation-iteration-count property.");
> 
> Add one like this too:
> 
>  189    assert_equals(target.getAnimations()[0], initialAnimation, "The
> animation object remained the same instance throughout the test.");

Will fix.

> > LayoutTests/webanimations/css-animations.html:172
> > +    assert_not_equals(updatedAnimation, initialAnimation, "Changing the animation-name property generates a different CSSAnimation object.");
> 
> You should test this right after getting updatedAnimation

OK.

Thanks for taking the time to review all this code!

[Antoine Quint]

*** This bug has been marked as a duplicate of bug 183504 ***