Changeset 200047 in webkit


Ignore:
Timestamp:
Apr 25, 2016, 1:56:47 PM (9 years ago)
Author:
Simon Fraser
Message:

Toggling animation-play-state can re-start a finished animation
​https://bugs.webkit.org/show_bug.cgi?id=156731

Reviewed by Dean Jackson.

Source/WebCore:

After an animation completed, CompositeAnimation::updateKeyframeAnimations() cleared
all state that the animation had run on the element, so changing the value of some
animation property triggered the animation to run again. This is wrong, since animation-name
still applied to the element.

Fix by keeping state for keyframe animations in the Done state in the m_keyframeAnimations
map. This allows for the removal of the index property on KeyframeAnimation.

Tests: animations/change-completed-animation-transform.html

animations/change-completed-animation.html

  • page/animation/AnimationBase.cpp:

(WebCore::AnimationBase::timeToNextService):

  • page/animation/AnimationBase.h:

(WebCore::AnimationBase::isAnimatingProperty):

  • page/animation/CompositeAnimation.cpp: Add animations that should stick around to AnimationNameMap,

and swap with m_keyframeAnimations at the end.
(WebCore::CompositeAnimation::updateKeyframeAnimations):

  • page/animation/KeyframeAnimation.cpp:

(WebCore::KeyframeAnimation::KeyframeAnimation):
(WebCore::KeyframeAnimation::getAnimatedStyle):

  • page/animation/KeyframeAnimation.h:

LayoutTests:

  • animations/animation-direction-reverse-expected.txt:
  • animations/animation-direction-reverse.html: This is a progression. The test was detecting a

restarted animation.

  • animations/change-completed-animation-expected.txt: Added.
  • animations/change-completed-animation-transform-expected.html: Added.
  • animations/change-completed-animation-transform.html: Added. Ref test that ensures that the final

state for normal and accelerated animations is correct.

  • animations/change-completed-animation.html: Added. Tests that changing a property doesn't trigger

another animation, by detecting a second animationstart event.

Location:
trunk
Files:
4 added
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r200043 r200047  
     12016-04-25  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Toggling animation-play-state can re-start a finished animation
     4        https://bugs.webkit.org/show_bug.cgi?id=156731
     5
     6        Reviewed by Dean Jackson.
     7
     8        * animations/animation-direction-reverse-expected.txt:
     9        * animations/animation-direction-reverse.html: This is a progression. The test was detecting a
     10        restarted animation.
     11        * animations/change-completed-animation-expected.txt: Added.
     12        * animations/change-completed-animation-transform-expected.html: Added.
     13        * animations/change-completed-animation-transform.html: Added. Ref test that ensures that the final
     14        state for normal and accelerated animations is correct.
     15        * animations/change-completed-animation.html: Added. Tests that changing a property doesn't trigger
     16        another animation, by detecting a second animationstart event.
     17
    1182016-04-25  Simon Fraser  <simon.fraser@apple.com>
    219

  • trunk/LayoutTests/animations/animation-direction-reverse-expected.txt

    r107162 r200047  
    11PASS - "webkitTransform" property for "box" element at 0.5s saw something close to: 1,0,0,1,150,0
    22PASS - "webkitTransform" property for "box" element at 1s saw something close to: 1,0,0,1,100,0
    3 PASS - "webkitTransform" property for "box" element at 2.5s saw something close to: 1,0,0,1,200,0
     3PASS - "webkitTransform" property for "box" element at 2.5s saw something close to: none
    44

  • trunk/LayoutTests/animations/animation-direction-reverse.html

    r107162 r200047  
    3535      ["move1", 0.5, "box", "webkitTransform", [1,0,0,1, 150,0], 20],
    3636      ["move1", 1.0, "box", "webkitTransform", [1,0,0,1,100,0], 20],
    37       ["move1", 2.5, "box", "webkitTransform", [1,0,0,1, 200,0], 20],
     37      ["move1", 2.5, "box", "webkitTransform", 'none', 20],
    3838    ];
    3939

  • trunk/Source/WebCore/ChangeLog

    r200046 r200047  
     12016-04-25  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Toggling animation-play-state can re-start a finished animation
     4        https://bugs.webkit.org/show_bug.cgi?id=156731
     5
     6        Reviewed by Dean Jackson.
     7
     8        After an animation completed, CompositeAnimation::updateKeyframeAnimations() cleared
     9        all state that the animation had run on the element, so changing the value of some
     10        animation property triggered the animation to run again. This is wrong, since animation-name
     11        still applied to the element.
     12
     13        Fix by keeping state for keyframe animations in the Done state in the m_keyframeAnimations
     14        map. This allows for the removal of the index property on KeyframeAnimation.
     15
     16        Tests: animations/change-completed-animation-transform.html
     17               animations/change-completed-animation.html
     18
     19        * page/animation/AnimationBase.cpp:
     20        (WebCore::AnimationBase::timeToNextService):
     21        * page/animation/AnimationBase.h:
     22        (WebCore::AnimationBase::isAnimatingProperty):
     23        * page/animation/CompositeAnimation.cpp: Add animations that should stick around to AnimationNameMap,
     24        and swap with m_keyframeAnimations at the end.
     25        (WebCore::CompositeAnimation::updateKeyframeAnimations):
     26        * page/animation/KeyframeAnimation.cpp:
     27        (WebCore::KeyframeAnimation::KeyframeAnimation):
     28        (WebCore::KeyframeAnimation::getAnimatedStyle):
     29        * page/animation/KeyframeAnimation.h:
     30
    1312016-04-25  Alberto Garcia  <berto@igalia.com>
    232

  • trunk/Source/WebCore/page/animation/AnimationBase.cpp

    r200042 r200047  
    561561    // Returns the time at which next service is required. -1 means no service is required. 0 means
    562562    // service is required now, and > 0 means service is required that many seconds in the future.
    563     if (paused() || isNew() || m_animationState == AnimationState::FillingForwards)
     563    if (paused() || isNew() || postActive() || fillingForwards())
    564564        return -1;
    565565   

  • trunk/Source/WebCore/page/animation/AnimationBase.h

    r199964 r200047  
    182182            return true;
    183183
    184         if ((runningState & Running) && !inPausedState() && (m_animationState >= AnimationState::StartWaitStyleAvailable && m_animationState <= AnimationState::Done))
     184        if ((runningState & Running) && !inPausedState() && (m_animationState >= AnimationState::StartWaitStyleAvailable && m_animationState < AnimationState::Done))
    185185            return true;
    186186

  • trunk/Source/WebCore/page/animation/CompositeAnimation.cpp

    r200041 r200047  
    204204    m_keyframeAnimations.checkConsistency();
    205205   
    206     if (currentStyle && currentStyle->hasAnimations() && targetStyle->hasAnimations() && *(currentStyle->animations()) == *(targetStyle->animations())) {
    207         // The current and target animations are the same so we just need to toss any
    208         // animation which is finished (postActive).
    209         for (auto& animation : m_keyframeAnimations.values()) {
    210             if (animation->postActive())
    211                 animation->setIndex(-1);
    212         }
    213     } else {
    214         // Mark all existing animations as no longer active.
    215         for (auto& animation : m_keyframeAnimations.values())
    216             animation->setIndex(-1);
     206    if (currentStyle && currentStyle->hasAnimations() && targetStyle->hasAnimations() && *(currentStyle->animations()) == *(targetStyle->animations()))
     207        return;
    217208
    218209#if ENABLE(CSS_ANIMATIONS_LEVEL_2)
    219         m_hasScrollTriggeredAnimation = false;
     210    m_hasScrollTriggeredAnimation = false;
    220211#endif
    221212
    222         // Toss the animation order map.
    223         m_keyframeAnimationOrderMap.clear();
    224 
    225         static NeverDestroyed<const AtomicString> none("none", AtomicString::ConstructFromLiteral);
    226        
    227         // Now mark any still active animations as active and add any new animations.
    228         if (targetStyle->animations()) {
    229             int numAnims = targetStyle->animations()->size();
    230             for (int i = 0; i < numAnims; ++i) {
    231                 Animation& animation = targetStyle->animations()->animation(i);
    232                 AtomicString animationName(animation.name());
    233 
    234                 if (!animation.isValidAnimation())
     213    AnimationNameMap newAnimations;
     214
     215    // Toss the animation order map.
     216    m_keyframeAnimationOrderMap.clear();
     217
     218    static NeverDestroyed<const AtomicString> none("none", AtomicString::ConstructFromLiteral);
     219   
     220    // Now mark any still active animations as active and add any new animations.
     221    if (targetStyle->animations()) {
     222        int numAnims = targetStyle->animations()->size();
     223        for (int i = 0; i < numAnims; ++i) {
     224            Animation& animation = targetStyle->animations()->animation(i);
     225            AtomicString animationName(animation.name());
     226
     227            if (!animation.isValidAnimation())
     228                continue;
     229           
     230            // See if there is a current animation for this name.
     231            RefPtr<KeyframeAnimation> keyframeAnim = m_keyframeAnimations.get(animationName.impl());
     232            if (keyframeAnim) {
     233                newAnimations.add(keyframeAnim->name().impl(), keyframeAnim);
     234
     235                if (keyframeAnim->postActive())
    235236                    continue;
    236                
    237                 // See if there is a current animation for this name.
    238                 RefPtr<KeyframeAnimation> keyframeAnim = m_keyframeAnimations.get(animationName.impl());
    239                
    240                 if (keyframeAnim) {
    241                     // If this animation is postActive, skip it so it gets removed at the end of this function.
    242                     if (keyframeAnim->postActive())
    243                         continue;
    244237
    245238#if ENABLE(CSS_ANIMATIONS_LEVEL_2)
    246                     if (animation.trigger()->isScrollAnimationTrigger())
    247                         m_hasScrollTriggeredAnimation = true;
     239                if (animation.trigger()->isScrollAnimationTrigger())
     240                    m_hasScrollTriggeredAnimation = true;
    248241#endif
    249242
    250                     // This one is still active.
    251 
    252                     // Animations match, but play states may differ. Update if needed.
    253                     keyframeAnim->updatePlayState(animation.playState());
    254                                
    255                     // Set the saved animation to this new one, just in case the play state has changed.
    256                     keyframeAnim->setAnimation(animation);
    257                     keyframeAnim->setIndex(i);
    258                 } else if ((animation.duration() || animation.delay()) && animation.iterationCount() && animationName != none) {
    259                     keyframeAnim = KeyframeAnimation::create(animation, renderer, i, this, targetStyle);
    260                     LOG(Animations, "Creating KeyframeAnimation %p on renderer %p with keyframes %s, duration %.2f, delay %.2f, iterations %.2f", keyframeAnim.get(), renderer, animation.name().utf8().data(), animation.duration(), animation.delay(), animation.iterationCount());
    261                     if (m_suspended) {
    262                         keyframeAnim->updatePlayState(AnimPlayStatePaused);
    263                         LOG(Animations, "  (created in suspended/paused state)");
    264                     }
     243                // Animations match, but play states may differ. Update if needed.
     244                keyframeAnim->updatePlayState(animation.playState());
     245
     246                // Set the saved animation to this new one, just in case the play state has changed.
     247                keyframeAnim->setAnimation(animation);
     248            } else if ((animation.duration() || animation.delay()) && animation.iterationCount() && animationName != none) {
     249                keyframeAnim = KeyframeAnimation::create(animation, renderer, this, targetStyle);
     250                LOG(Animations, "Creating KeyframeAnimation %p on renderer %p with keyframes %s, duration %.2f, delay %.2f, iterations %.2f", keyframeAnim.get(), renderer, animation.name().utf8().data(), animation.duration(), animation.delay(), animation.iterationCount());
     251
     252                if (m_suspended) {
     253                    keyframeAnim->updatePlayState(AnimPlayStatePaused);
     254                    LOG(Animations, "  (created in suspended/paused state)");
     255                }
    265256#if !LOG_DISABLED
    266                     for (auto propertyID : keyframeAnim->keyframes().properties())
    267                         LOG(Animations, "  property %s", getPropertyName(propertyID));
     257                for (auto propertyID : keyframeAnim->keyframes().properties())
     258                    LOG(Animations, "  property %s", getPropertyName(propertyID));
    268259#endif
    269260
    270261#if ENABLE(CSS_ANIMATIONS_LEVEL_2)
    271                     if (animation.trigger()->isScrollAnimationTrigger())
    272                         m_hasScrollTriggeredAnimation = true;
     262                if (animation.trigger()->isScrollAnimationTrigger())
     263                    m_hasScrollTriggeredAnimation = true;
    273264#endif
    274265
    275                     m_keyframeAnimations.set(keyframeAnim->name().impl(), keyframeAnim);
    276                 }
    277                
    278                 // Add this to the animation order map.
    279                 if (keyframeAnim)
    280                     m_keyframeAnimationOrderMap.append(keyframeAnim->name().impl());
     266                newAnimations.set(keyframeAnim->name().impl(), keyframeAnim);
    281267            }
     268           
     269            // Add this to the animation order map.
     270            if (keyframeAnim)
     271                m_keyframeAnimationOrderMap.append(keyframeAnim->name().impl());
    282272        }
    283273    }
    284274   
    285275    // Make a list of animations to be removed.
    286     Vector<AtomicStringImpl*> animsToBeRemoved;
    287276    for (auto& animation : m_keyframeAnimations.values()) {
    288         if (animation->index() < 0) {
    289             animsToBeRemoved.append(animation->name().impl());
     277        if (!newAnimations.contains(animation->name().impl())) {
    290278            animationController().animationWillBeRemoved(animation.get());
    291279            animation->clear();
    … …  
    294282    }
    295283   
    296     // Now remove the animations from the list.
    297     for (auto* nameForRemoval : animsToBeRemoved)
    298         m_keyframeAnimations.remove(nameForRemoval);
     284    std::swap(newAnimations, m_keyframeAnimations);
    299285}
    300286

  • trunk/Source/WebCore/page/animation/KeyframeAnimation.cpp

    r200042 r200047  
    4242namespace WebCore {
    4343
    44 KeyframeAnimation::KeyframeAnimation(Animation& animation, RenderElement* renderer, int index, CompositeAnimation* compositeAnimation, RenderStyle* unanimatedStyle)
     44KeyframeAnimation::KeyframeAnimation(Animation& animation, RenderElement* renderer, CompositeAnimation* compositeAnimation, RenderStyle* unanimatedStyle)
    4545    : AnimationBase(animation, renderer, compositeAnimation)
    4646    , m_keyframes(animation.name())
    4747    , m_unanimatedStyle(RenderStyle::clonePtr(*unanimatedStyle))
    48     , m_index(index)
    4948{
    5049    // Get the keyframe RenderStyles
    … …  
    191190void KeyframeAnimation::getAnimatedStyle(std::unique_ptr<RenderStyle>& animatedStyle)
    192191{
    193     // If we're in the delay phase and we're not backwards filling, tell the caller
    194     // to use the current style.
    195     if (waitingToStart() && m_animation->delay() > 0 && !m_animation->fillsBackwards())
     192    // If we're done, or in the delay phase and we're not backwards filling, tell the caller to use the current style.
     193    if (postActive() || (waitingToStart() && m_animation->delay() > 0 && !m_animation->fillsBackwards()))
    196194        return;
    197195

  • trunk/Source/WebCore/page/animation/KeyframeAnimation.h

    r199964 r200047  
    4141class KeyframeAnimation final : public AnimationBase {
    4242public:
    43     static Ref<KeyframeAnimation> create(Animation& animation, RenderElement* renderer, int index, CompositeAnimation* compositeAnimation, RenderStyle* unanimatedStyle)
     43    static Ref<KeyframeAnimation> create(Animation& animation, RenderElement* renderer, CompositeAnimation* compositeAnimation, RenderStyle* unanimatedStyle)
    4444    {
    45         return adoptRef(*new KeyframeAnimation(animation, renderer, index, compositeAnimation, unanimatedStyle));
     45        return adoptRef(*new KeyframeAnimation(animation, renderer, compositeAnimation, unanimatedStyle));
    4646    }
    4747
    … …  
    5454
    5555    const AtomicString& name() const { return m_keyframes.animationName(); }
    56     int index() const { return m_index; }
    57     void setIndex(int i) { m_index = i; }
    5856
    5957    bool hasAnimationForProperty(CSSPropertyID) const;
    … …  
    9189
    9290private:
    93     KeyframeAnimation(Animation&, RenderElement*, int index, CompositeAnimation*, RenderStyle* unanimatedStyle);
     91    KeyframeAnimation(Animation&, RenderElement*, CompositeAnimation*, RenderStyle* unanimatedStyle);
    9492    virtual ~KeyframeAnimation();
    9593   
    … …  
    10098    std::unique_ptr<RenderStyle> m_unanimatedStyle; // The style just before we started animation
    10199
    102     int m_index; // The order in which this animation appears in the animation-name style.
    103100    bool m_startEventDispatched { false };
    104101};
Note: See TracChangeset for help on using the changeset viewer.