[Web Animations] Support "transition: all" for CSS Transitions as Web Animations
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2018 11:42:31 +0000 (11:42 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2018 11:42:31 +0000 (11:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183917

Reviewed by Dean Jackson.

LayoutTests/imported/mozilla:

Record progressions thanks to supporting "transition: all".

* css-transitions/test_animation-finished-expected.txt:
* css-transitions/test_csstransition-transitionproperty-expected.txt:
* css-transitions/test_effect-target-expected.txt:
* css-transitions/test_element-get-animations-expected.txt:
* css-transitions/test_pseudoElement-get-animations-expected.txt:

Source/WebCore:

We now support "transition: all" CSS Transitions by iterating over all known CSS properties should the mode
of the backing animation be AnimateAll. Any property that we find to have a different value in the previous
and current style will have a backing CSSTransition object created for it. To support this, we now explicitly
provide a CSSPropertyID when creating a CSSTransition since we can no longer infer the transition property
from the backing animation, as Animation objects with mode AnimateAll report CSSPropertyInvalid as their
property.

* animation/AnimationTimeline.cpp:
(WebCore::shouldBackingAnimationBeConsideredForCSSTransition): New method that checks whether a given backing
Animation object is suitable for consideration as a CSSTransition, where the mode must not be either AnimateNone
or AnimateUnknownProperty, and should the mode be AnimateSingleProperty, the property must not be CSSPropertyInvalid.
(WebCore::AnimationTimeline::updateCSSTransitionsForElement): We now assemble the list of previously animated
properties by looking at the m_elementToCSSTransitionByCSSPropertyID map and getting its keys. Then we compile
all backing Animation objects found in the old style that match the conditions enforced by the new method
shouldBackingAnimationBeConsideredForCSSTransition(). Then as we iterate over backing Animation objects found
in the new style, we iterate over all known CSS properties if the mode is AnimateAll, indicating that we're dealing
with a "transition: all" style. If we're dealing with a single property, we only process that single property.
* animation/CSSTransition.cpp:
(WebCore::CSSTransition::create): Expect a new CSSPropertyID parameter when creating a new CSSTransition since
we can no longer infer it from the backing Animation object.
(WebCore::CSSTransition::CSSTransition): Expect a new CSSPropertyID parameter when creating a new CSSTransition
since we can no longer infer it from the backing Animation object.
(WebCore::CSSTransition::matchesBackingAnimationAndStyles const): We can no longer use the == overloaded operator
for backing Animation objects to determine whether their respective properties match since this would compare the
"property" member of both Animation objects and when going from a "transition: all" style to one targeting a single
property, we would falsely identify mis-matching Animation objects. Instead, we pass a false flag to animationsMatch()
which indicates that we don't care about matching the transition property itself.
* animation/CSSTransition.h: Expose a new property() accessor which returns the CSSPropertyID passed at construction.
* animation/KeyframeEffectReadOnly.cpp:
(WebCore::KeyframeEffectReadOnly::computeCSSTransitionBlendingKeyframes): Use the new property() accessor on
CSSTransition to get at the transition property.
* platform/animation/Animation.cpp:
(WebCore::Animation::animationsMatch const): Replace the boolean parameter, which was not in use in WebCore, to indicate
whether we should match the property-related fields. We need this in CSSTransition::matchesBackingAnimationAndStyles().
* platform/animation/Animation.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@229888 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
LayoutTests/imported/mozilla/ChangeLog
LayoutTests/imported/mozilla/css-transitions/test_animation-finished-expected.txt
LayoutTests/imported/mozilla/css-transitions/test_csstransition-transitionproperty-expected.txt
LayoutTests/imported/mozilla/css-transitions/test_effect-target-expected.txt
LayoutTests/imported/mozilla/css-transitions/test_element-get-animations-expected.txt
LayoutTests/imported/mozilla/css-transitions/test_pseudoElement-get-animations-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationTimeline.cpp
Source/WebCore/animation/CSSTransition.cpp
Source/WebCore/animation/CSSTransition.h
Source/WebCore/animation/KeyframeEffectReadOnly.cpp
Source/WebCore/platform/animation/Animation.cpp
Source/WebCore/platform/animation/Animation.h

index 9fdecc1..00d96f6 100644 (file)
@@ -1,3 +1,18 @@
+2018-03-22  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Support "transition: all" for CSS Transitions as Web Animations
+        https://bugs.webkit.org/show_bug.cgi?id=183917
+
+        Reviewed by Dean Jackson.
+
+        Record progressions thanks to supporting "transition: all".
+
+        * css-transitions/test_animation-finished-expected.txt:
+        * css-transitions/test_csstransition-transitionproperty-expected.txt:
+        * css-transitions/test_effect-target-expected.txt:
+        * css-transitions/test_element-get-animations-expected.txt:
+        * css-transitions/test_pseudoElement-get-animations-expected.txt:
+
 2018-03-21  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] Make imported/mozilla/css-animations/test_event-dispatch.html pass reliably
index 96f4e9d..1053a33 100644 (file)
@@ -1,4 +1,4 @@
 
 PASS Test restarting a finished transition 
-FAIL Test restarting a reversed finished transition assert_equals: Replaying a finished reversed transition should reset its currentTime to the end of the effect expected 2000000 but got 0
+PASS Test restarting a reversed finished transition 
 
index cfdf551..d90ce73 100644 (file)
@@ -1,5 +1,5 @@
 
 PASS Returned CSS transitions have the correct Animation.target 
-FAIL effect.target should return the same CSSPseudoElement object each time assert_equals: Got transitions running on ::after pseudo element expected 2 but got 0
-FAIL effect.target from the script-generated animation should return the same CSSPseudoElement object as that from the CSS generated transition undefined is not an object (evaluating 'document.getAnimations()[0].effect')
+PASS effect.target should return the same CSSPseudoElement object each time 
+FAIL effect.target from the script-generated animation should return the same CSSPseudoElement object as that from the CSS generated transition assert_equals: Got animations running on ::after pseudo element expected 2 but got 4
 
index 9cf6b41..7cbf626 100644 (file)
@@ -1,3 +1,9 @@
-#PID UNRESPONSIVE - com.apple.WebKit.WebContent.Development (pid 13530)
-FAIL: Timed out waiting for notifyDone to be called
+
+PASS getAnimations for CSS Transitions 
+PASS getAnimations returns CSSTransition objects for CSS Transitions 
+PASS getAnimations for CSS Transitions that have finished 
+FAIL getAnimations for transition on non-animatable property assert_equals: getAnimations returns an empty sequence for a transition of a non-animatable property expected 0 but got 1
+PASS getAnimations for transition on unsupported property 
+FAIL getAnimations sorts simultaneous transitions by name assert_equals: expected "border-bottom-width" but got "border-left-width"
+PASS getAnimations sorts transitions by when they were generated 
 
index 467b0b3..5fb11eb 100644 (file)
@@ -1,3 +1,3 @@
 
-FAIL getAnimations sorts simultaneous transitions by name assert_equals: Got expected number of animations on document expected 3 but got 0
+FAIL getAnimations sorts simultaneous transitions by name assert_equals: Got pseudo-element target expected "[object CSSPseudoElement]" but got "[object Element]"
 
index 726fa12..52a038e 100644 (file)
@@ -1,3 +1,46 @@
+2018-03-22  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Support "transition: all" for CSS Transitions as Web Animations
+        https://bugs.webkit.org/show_bug.cgi?id=183917
+
+        Reviewed by Dean Jackson.
+
+        We now support "transition: all" CSS Transitions by iterating over all known CSS properties should the mode
+        of the backing animation be AnimateAll. Any property that we find to have a different value in the previous
+        and current style will have a backing CSSTransition object created for it. To support this, we now explicitly
+        provide a CSSPropertyID when creating a CSSTransition since we can no longer infer the transition property
+        from the backing animation, as Animation objects with mode AnimateAll report CSSPropertyInvalid as their
+        property.
+
+        * animation/AnimationTimeline.cpp:
+        (WebCore::shouldBackingAnimationBeConsideredForCSSTransition): New method that checks whether a given backing
+        Animation object is suitable for consideration as a CSSTransition, where the mode must not be either AnimateNone
+        or AnimateUnknownProperty, and should the mode be AnimateSingleProperty, the property must not be CSSPropertyInvalid.
+        (WebCore::AnimationTimeline::updateCSSTransitionsForElement): We now assemble the list of previously animated
+        properties by looking at the m_elementToCSSTransitionByCSSPropertyID map and getting its keys. Then we compile
+        all backing Animation objects found in the old style that match the conditions enforced by the new method
+        shouldBackingAnimationBeConsideredForCSSTransition(). Then as we iterate over backing Animation objects found
+        in the new style, we iterate over all known CSS properties if the mode is AnimateAll, indicating that we're dealing
+        with a "transition: all" style. If we're dealing with a single property, we only process that single property.
+        * animation/CSSTransition.cpp:
+        (WebCore::CSSTransition::create): Expect a new CSSPropertyID parameter when creating a new CSSTransition since
+        we can no longer infer it from the backing Animation object.
+        (WebCore::CSSTransition::CSSTransition): Expect a new CSSPropertyID parameter when creating a new CSSTransition
+        since we can no longer infer it from the backing Animation object.
+        (WebCore::CSSTransition::matchesBackingAnimationAndStyles const): We can no longer use the == overloaded operator
+        for backing Animation objects to determine whether their respective properties match since this would compare the
+        "property" member of both Animation objects and when going from a "transition: all" style to one targeting a single
+        property, we would falsely identify mis-matching Animation objects. Instead, we pass a false flag to animationsMatch()
+        which indicates that we don't care about matching the transition property itself.
+        * animation/CSSTransition.h: Expose a new property() accessor which returns the CSSPropertyID passed at construction.
+        * animation/KeyframeEffectReadOnly.cpp:
+        (WebCore::KeyframeEffectReadOnly::computeCSSTransitionBlendingKeyframes): Use the new property() accessor on
+        CSSTransition to get at the transition property.
+        * platform/animation/Animation.cpp:
+        (WebCore::Animation::animationsMatch const): Replace the boolean parameter, which was not in use in WebCore, to indicate
+        whether we should match the property-related fields. We need this in CSSTransition::matchesBackingAnimationAndStyles().
+        * platform/animation/Animation.h:
+
 2018-03-22  Tim Horton  <timothy_horton@apple.com>
 
         Adopt WK_ALTERNATE_FRAMEWORKS_DIR in WebCore
index 844e4be..3ef4bc7 100644 (file)
@@ -191,6 +191,16 @@ void AnimationTimeline::updateCSSAnimationsForElement(Element& element, const Re
         m_elementToCSSAnimationByName.remove(&element);
 }
 
+static bool shouldBackingAnimationBeConsideredForCSSTransition(const Animation& backingAnimation)
+{
+    auto mode = backingAnimation.animationMode();
+    if (mode == Animation::AnimateNone || mode == Animation::AnimateUnknownProperty)
+        return false;
+    if (mode == Animation::AnimateSingleProperty && backingAnimation.property() == CSSPropertyInvalid)
+        return false;
+    return true;
+}
+
 void AnimationTimeline::updateCSSTransitionsForElement(Element& element, const RenderStyle& newStyle, const RenderStyle* oldStyle)
 {
     if (element.document().pageCacheState() != Document::NotInPageCache)
@@ -208,49 +218,65 @@ void AnimationTimeline::updateCSSTransitionsForElement(Element& element, const R
         return;
     }
 
-    // FIXME: We do not handle "all" transitions yet.
+    // Create or get the CSSTransitions by CSS property name map for this element.
+    auto& cssTransitionsByProperty = m_elementToCSSTransitionByCSSPropertyID.ensure(&element, [] {
+        return HashMap<CSSPropertyID, RefPtr<CSSTransition>> { };
+    }).iterator->value;
 
     // First, compile the list of backing animations and properties that were applied to this element up to this point.
-    HashSet<CSSPropertyID> previousProperties;
+    auto previousProperties = copyToVector(cssTransitionsByProperty.keys());
     HashSet<const Animation*> previousBackingAnimations;
     if (oldStyle && oldStyle->hasTransitions()) {
         auto* previousTransitions = oldStyle->transitions();
         for (size_t i = 0; i < previousTransitions->size(); ++i) {
-            auto& animation = previousTransitions->animation(i);
-            auto previousTransitionProperty = animation.property();
-            if (previousTransitionProperty != CSSPropertyInvalid) {
-                previousProperties.add(previousTransitionProperty);
-                previousBackingAnimations.add(&animation);
-            }
+            auto& backingAnimation = previousTransitions->animation(i);
+            if (shouldBackingAnimationBeConsideredForCSSTransition(backingAnimation))
+                previousBackingAnimations.add(&backingAnimation);
         }
     }
 
-    // Create or get the CSSTransitions by CSS property name map for this element.
-    auto& cssTransitionsByProperty = m_elementToCSSTransitionByCSSPropertyID.ensure(&element, [] {
-        return HashMap<CSSPropertyID, RefPtr<CSSTransition>> { };
-    }).iterator->value;
-
     if (auto* currentTransitions = newStyle.transitions()) {
         for (size_t i = 0; i < currentTransitions->size(); ++i) {
             auto& backingAnimation = currentTransitions->animation(i);
-            auto property = backingAnimation.property();
-            if (property == CSSPropertyInvalid)
+            if (!shouldBackingAnimationBeConsideredForCSSTransition(backingAnimation))
                 continue;
-            previousProperties.remove(property);
-            // We've found a backing animation that we didn't know about for a valid property.
-            if (!previousBackingAnimations.contains(&backingAnimation)) {
-                // If we already had a CSSTransition for this property, check whether its timing properties match the current backing
-                // animation's properties and whether its blending keyframes match the old and new styles. If they do, move on to the
-                // next transition, otherwise delete the previous CSSTransition object, and create a new one.
-                if (cssTransitionsByProperty.contains(property)) {
-                    if (cssTransitionsByProperty.get(property)->matchesBackingAnimationAndStyles(backingAnimation, oldStyle, newStyle))
+            auto property = backingAnimation.property();
+            bool transitionsAllProperties = backingAnimation.animationMode() == Animation::AnimateAll;
+            auto numberOfProperties = CSSPropertyAnimation::getNumProperties();
+            // In the "transition-property: all" case, where the animation's mode is set to AnimateAll,
+            // the property will be set to CSSPropertyInvalid and we need to iterate over all known
+            // CSS properties and see if they have mis-matching values in the old and new styles, which
+            // means they should have a CSSTransition created for them.
+            // We implement a single loop which handles the "all" case and the specified property case
+            // by using the pre-set property above in the specified property case and breaking out of
+            // the loop after the first complete iteration.
+            for (int propertyIndex = 0; propertyIndex < numberOfProperties; ++propertyIndex) {
+                if (transitionsAllProperties) {
+                    bool isShorthand;
+                    property = CSSPropertyAnimation::getPropertyAtIndex(propertyIndex, isShorthand);
+                    if (isShorthand)
                         continue;
-                    removeDeclarativeAnimation(cssTransitionsByProperty.take(property));
+                } else if (propertyIndex) {
+                    // We only go once through this loop if we are transitioning a single property.
+                    break;
+                }
+
+                bool hadProperty = previousProperties.removeFirst(property);
+                // We've found a backing animation that we didn't know about for a valid property.
+                if (!previousBackingAnimations.contains(&backingAnimation)) {
+                    // If we already had a CSSTransition for this property, check whether its timing properties match the current backing
+                    // animation's properties and whether its blending keyframes match the old and new styles. If they do, move on to the
+                    // next transition, otherwise delete the previous CSSTransition object, and create a new one.
+                    if (hadProperty) {
+                        if (cssTransitionsByProperty.get(property)->matchesBackingAnimationAndStyles(backingAnimation, oldStyle, newStyle))
+                            continue;
+                        removeDeclarativeAnimation(cssTransitionsByProperty.take(property));
+                    }
+                    // Now we can create a new CSSTransition with the new backing animation provided it has a valid
+                    // duration and the from and to values are distinct.
+                    if (backingAnimation.duration() > 0 && oldStyle && !CSSPropertyAnimation::propertiesEqual(property, oldStyle, &newStyle))
+                        cssTransitionsByProperty.set(property, CSSTransition::create(element, property, backingAnimation, oldStyle, newStyle));
                 }
-                // Now we can create a new CSSTransition with the new backing animation provided it has a valid
-                // duration and the from and to values are distinct.
-                if (backingAnimation.duration() > 0 && oldStyle && !CSSPropertyAnimation::propertiesEqual(property, oldStyle, &newStyle))
-                    cssTransitionsByProperty.set(property, CSSTransition::create(element, backingAnimation, oldStyle, newStyle));
             }
         }
     }
index 7fd1b73..4d1d79b 100644 (file)
 
 namespace WebCore {
 
-Ref<CSSTransition> CSSTransition::create(Element& target, const Animation& backingAnimation, const RenderStyle* oldStyle, const RenderStyle& newStyle)
+Ref<CSSTransition> CSSTransition::create(Element& target, CSSPropertyID property, const Animation& backingAnimation, const RenderStyle* oldStyle, const RenderStyle& newStyle)
 {
-    auto result = adoptRef(*new CSSTransition(target, backingAnimation));
-    result->m_transitionProperty = backingAnimation.property();
+    auto result = adoptRef(*new CSSTransition(target, property, backingAnimation));
     result->initialize(target);
     downcast<KeyframeEffectReadOnly>(result->effect())->computeCSSTransitionBlendingKeyframes(oldStyle, newStyle);
     return result;
 }
 
-CSSTransition::CSSTransition(Element& element, const Animation& backingAnimation)
+CSSTransition::CSSTransition(Element& element, CSSPropertyID property, const Animation& backingAnimation)
     : DeclarativeAnimation(element, backingAnimation)
+    , m_property(property)
 {
 }
 
 bool CSSTransition::matchesBackingAnimationAndStyles(const Animation& newBackingAnimation, const RenderStyle* oldStyle, const RenderStyle& newStyle) const
 {
-    bool backingAnimationsMatch = backingAnimation() == newBackingAnimation;
+    // See if the animations match, excluding the property since we can move from an "all" transition to an explicit property transition.
+    bool backingAnimationsMatch = backingAnimation().animationsMatch(newBackingAnimation, false);
     if (!oldStyle || !effect())
         return backingAnimationsMatch;
     return backingAnimationsMatch && !downcast<KeyframeEffectReadOnly>(effect())->stylesWouldYieldNewCSSTransitionsBlendingKeyframes(*oldStyle, newStyle);
index e8a053a..7d3eeee 100644 (file)
@@ -37,19 +37,20 @@ class RenderStyle;
 
 class CSSTransition final : public DeclarativeAnimation {
 public:
-    static Ref<CSSTransition> create(Element&, const Animation&, const RenderStyle* oldStyle, const RenderStyle& newStyle);
+    static Ref<CSSTransition> create(Element&, CSSPropertyID, const Animation&, const RenderStyle* oldStyle, const RenderStyle& newStyle);
     ~CSSTransition() = default;
 
     bool isCSSTransition() const override { return true; }
-    String transitionProperty() const { return getPropertyNameString(m_transitionProperty); }
+    String transitionProperty() const { return getPropertyNameString(m_property); }
+    CSSPropertyID property() const { return m_property; }
 
     bool matchesBackingAnimationAndStyles(const Animation&, const RenderStyle* oldStyle, const RenderStyle& newStyle) const;
     bool canBeListed() const final;
 
 private:
-    CSSTransition(Element&, const Animation&);
+    CSSTransition(Element&, CSSPropertyID, const Animation&);
 
-    CSSPropertyID m_transitionProperty;
+    CSSPropertyID m_property;
 
 };
 
index 2a7a881..4ab4c54 100644 (file)
@@ -721,21 +721,21 @@ void KeyframeEffectReadOnly::computeCSSTransitionBlendingKeyframes(const RenderS
     if (!oldStyle || m_blendingKeyframes.size())
         return;
 
-    auto& backingAnimation = downcast<CSSTransition>(animation())->backingAnimation();
+    auto property = downcast<CSSTransition>(animation())->property();
 
     auto toStyle = RenderStyle::clonePtr(newStyle);
     if (m_target)
         Style::loadPendingResources(*toStyle, m_target->document(), m_target.get());
 
     KeyframeList keyframeList("keyframe-effect-" + createCanonicalUUIDString());
-    keyframeList.addProperty(backingAnimation.property());
+    keyframeList.addProperty(property);
 
     KeyframeValue fromKeyframeValue(0, RenderStyle::clonePtr(*oldStyle));
-    fromKeyframeValue.addProperty(backingAnimation.property());
+    fromKeyframeValue.addProperty(property);
     keyframeList.insert(WTFMove(fromKeyframeValue));
 
     KeyframeValue toKeyframeValue(1, WTFMove(toStyle));
-    toKeyframeValue.addProperty(backingAnimation.property());
+    toKeyframeValue.addProperty(property);
     keyframeList.insert(WTFMove(toKeyframeValue));
 
     m_blendingKeyframes = WTFMove(keyframeList);
index cf6cceb..3594c12 100644 (file)
@@ -127,12 +127,12 @@ Animation& Animation::operator=(const Animation& o)
 
 Animation::~Animation() = default;
 
-bool Animation::animationsMatch(const Animation& other, bool matchPlayStates) const
+bool Animation::animationsMatch(const Animation& other, bool matchProperties) const
 {
     bool result = m_name == other.m_name
         && m_nameStyleScopeOrdinal == other.m_nameStyleScopeOrdinal
-        && m_property == other.m_property
-        && m_mode == other.m_mode
+        && m_playState == other.m_playState
+        && m_playStateSet == other.m_playStateSet
         && m_iterationCount == other.m_iterationCount
         && m_delay == other.m_delay
         && m_duration == other.m_duration
@@ -148,7 +148,6 @@ bool Animation::animationsMatch(const Animation& other, bool matchPlayStates) co
         && m_fillModeSet == other.m_fillModeSet
         && m_iterationCountSet == other.m_iterationCountSet
         && m_nameSet == other.m_nameSet
-        && m_propertySet == other.m_propertySet
         && m_timingFunctionSet == other.m_timingFunctionSet
 #if ENABLE(CSS_ANIMATIONS_LEVEL_2)
         && m_triggerSet == other.m_triggerSet
@@ -158,7 +157,7 @@ bool Animation::animationsMatch(const Animation& other, bool matchPlayStates) co
     if (!result)
         return false;
 
-    return !matchPlayStates || (m_playState == other.m_playState && m_playStateSet == other.m_playStateSet);
+    return !matchProperties || (m_mode == other.m_mode && m_property == other.m_property && m_propertySet == other.m_propertySet);
 }
 
 const String& Animation::initialName()
index 669cc24..f7b0eca 100644 (file)
@@ -162,7 +162,7 @@ public:
     Animation& operator=(const Animation& o);
 
     // return true if all members of this class match (excluding m_next)
-    bool animationsMatch(const Animation&, bool matchPlayStates = true) const;
+    bool animationsMatch(const Animation&, bool matchProperties = true) const;
 
     // return true every Animation in the chain (defined by m_next) match 
     bool operator==(const Animation& o) const { return animationsMatch(o); }