Web Animations: Correctly handle incomplete keyframes in CSS animations

Source/core/css/resolver/StyleResolver.cpp

Index: Source/core/css/resolver/StyleResolver.cpp
diff --git a/Source/core/css/resolver/StyleResolver.cpp b/Source/core/css/resolver/StyleResolver.cpp
index 7d34c1c50d779b90588ebb348dc5f746f498d382..2df92d7470ae92d1d49d22627ba2ff0627bc90ea 100644
--- a/Source/core/css/resolver/StyleResolver.cpp
+++ b/Source/core/css/resolver/StyleResolver.cpp
@@ -83,6 +83,74 @@
 
 using namespace std;
 
+namespace {
+
+using namespace WebCore;
+
+void prepareEndKeyframes(const RenderStyle* elementStyle, const PropertySet& allProperties, KeyframeAnimationEffect::KeyframeVector& keyframes)
+{
+    // Add 0% and 100% keyframes if absent.
+    RefPtr<Keyframe> startKeyframe = keyframes[0];
+    if (startKeyframe->offset()) {
+        startKeyframe = Keyframe::create();
+        startKeyframe->setOffset(0);
+        keyframes.prepend(startKeyframe);
+    }
+    RefPtr<Keyframe> endKeyframe = keyframes[keyframes.size() - 1];
+    if (endKeyframe->offset() != 1) {
+        endKeyframe = Keyframe::create();
+        endKeyframe->setOffset(1);
+        keyframes.append(endKeyframe);
+    }
+    ASSERT(keyframes.size() >= 2);
+    ASSERT(!keyframes.first()->offset());
+    ASSERT(keyframes.last()->offset() == 1);
+
+    // Snapshot current property values for 0% and 100% if missing.
+    const PropertySet& startKeyframeProperties = startKeyframe->properties();
+    const PropertySet& endKeyframeProperties = endKeyframe->properties();
+    bool missingStartValues = startKeyframeProperties.size() < allProperties.size();
+    bool missingEndValues = endKeyframeProperties.size() < allProperties.size();
+    if (!missingStartValues && !missingEndValues)
+        return;
+    for (PropertySet::const_iterator iter = allProperties.begin(); iter != allProperties.end(); ++iter) {
+        const CSSPropertyID property = *iter;
+        bool startNeedsValue = missingStartValues && !startKeyframeProperties.contains(property);
+        bool endNeedsValue = missingEndValues && !endKeyframeProperties.contains(property);
+        if (!startNeedsValue && !endNeedsValue)
+            continue;
+        RefPtr<AnimatableValue> snapshotValue = CSSAnimatableValueFactory::create(property, elementStyle);
+        if (startNeedsValue)
+            startKeyframe->setPropertyValue(property, snapshotValue.get());
+        if (endNeedsValue)
+            endKeyframe->setPropertyValue(property, snapshotValue.get());
+    }
+    ASSERT(startKeyframe->properties().size() == allProperties.size());
+    ASSERT(endKeyframe->properties().size() == allProperties.size());
+}
+
+PassRefPtr<TimingFunction> generateTimingFunction(const KeyframeAnimationEffect::KeyframeVector keyframes, const HashMap<double, RefPtr<TimingFunction> > perKeyframeTimingFunctions)
+{
+    // Generate the chained timing function. Note that timing functions apply
+    // from the keyframe in which they're specified to the next keyframe.
+    bool isTimingFunctionLinearThroughout = true;
+    RefPtr<ChainedTimingFunction> chainedTimingFunction = ChainedTimingFunction::create();
+    for (size_t i = 0; i < keyframes.size() - 1; ++i) {
+        double lowerBound = keyframes[i]->offset();
+        ASSERT(lowerBound >=0 && lowerBound < 1);
+        double upperBound = keyframes[i + 1]->offset();
+        ASSERT(upperBound > 0 && upperBound <= 1);
+        TimingFunction* timingFunction = perKeyframeTimingFunctions.get(lowerBound);
+        isTimingFunctionLinearThroughout &= timingFunction->type() == TimingFunction::LinearFunction;
+        chainedTimingFunction->appendSegment(upperBound, timingFunction);
+    }
+    if (isTimingFunctionLinearThroughout)
+        return LinearTimingFunction::create();
+    return chainedTimingFunction;
+}
+
+} // namespace
+
 namespace WebCore {
 
 using namespace HTMLNames;
@@ -833,7 +901,7 @@ void StyleResolver::keyframeStylesForAnimation(Element* e, const RenderStyle* el
     }
 }
 
-void StyleResolver::resolveKeyframes(const Element* element, const RenderStyle* style, const AtomicString& name, TimingFunction* defaultTimingFunction, KeyframeAnimationEffect::KeyframeVector& keyframes, RefPtr<TimingFunction>& timingFunction)
+void StyleResolver::resolveKeyframes(const Element* element, const RenderStyle* style, const AtomicString& name, TimingFunction* defaultTimingFunction, Vector<std::pair<KeyframeAnimationEffect::KeyframeVector, RefPtr<TimingFunction> > >& keyframesAndTimingFunctions)
 {
     ASSERT(RuntimeEnabledFeatures::webAnimationsCSSEnabled());
     const StyleRuleKeyframes* keyframesRule = matchScopedKeyframesRule(element, name.impl());
@@ -841,7 +909,8 @@ void StyleResolver::resolveKeyframes(const Element* element, const RenderStyle*
         return;
 
     // Construct and populate the style for each keyframe
-    HashMap<double, RefPtr<TimingFunction> > timingFunctions;
+    KeyframeAnimationEffect::KeyframeVector keyframes;
+    HashMap<double, RefPtr<TimingFunction> > perKeyframeTimingFunctions;
     const Vector<RefPtr<StyleKeyframe> >& styleKeyframes = keyframesRule->keyframes();
     for (size_t i = 0; i < styleKeyframes.size(); ++i) {
         const StyleKeyframe* styleKeyframe = styleKeyframes[i].get();
@@ -856,7 +925,6 @@ void StyleResolver::resolveKeyframes(const Element* element, const RenderStyle*
             CSSPropertyID property = properties->propertyAt(j).id();
             if (property == CSSPropertyWebkitAnimationTimingFunction || property == CSSPropertyAnimationTimingFunction) {
                 // FIXME: This sometimes gets the wrong timing function. See crbug.com/288540.
-
                 timingFunction = KeyframeValue::timingFunction(keyframeStyle.get(), name);
             } else if (CSSAnimations::isAnimatableProperty(property)) {
                 keyframe->setPropertyValue(property, CSSAnimatableValueFactory::create(property, keyframeStyle.get()).get());
@@ -864,16 +932,19 @@ void StyleResolver::resolveKeyframes(const Element* element, const RenderStyle*
         }
         keyframes.append(keyframe);
         // The last keyframe specified at a given offset is used.
-        timingFunctions.set(offsets[0], timingFunction);
+        perKeyframeTimingFunctions.set(offsets[0], timingFunction);
         for (size_t j = 1; j < offsets.size(); ++j) {
             keyframes.append(keyframe->cloneWithOffset(offsets[j]));
-            timingFunctions.set(offsets[j], timingFunction);
+            perKeyframeTimingFunctions.set(offsets[j], timingFunction);
         }
     }
 
     if (keyframes.isEmpty())
         return;
 
+    if (!perKeyframeTimingFunctions.contains(0))
+        perKeyframeTimingFunctions.set(0, defaultTimingFunction);
+
     // Remove duplicate keyframes. In CSS the last keyframe at a given offset takes priority.
     std::stable_sort(keyframes.begin(), keyframes.end(), Keyframe::compareOffsets);
     size_t targetIndex = 0;
@@ -885,76 +956,73 @@ void StyleResolver::resolveKeyframes(const Element* element, const RenderStyle*
     }
     keyframes.shrink(targetIndex + 1);
 
-    // Add 0% and 100% keyframes if absent.
-    RefPtr<Keyframe> startKeyframe = keyframes[0];
-    if (startKeyframe->offset()) {
-        startKeyframe = Keyframe::create();
-        startKeyframe->setOffset(0);
-        keyframes.prepend(startKeyframe);
-    }
-    RefPtr<Keyframe> endKeyframe = keyframes[keyframes.size() - 1];
-    if (endKeyframe->offset() != 1) {
-        endKeyframe = Keyframe::create();
-        endKeyframe->setOffset(1);
-        keyframes.append(endKeyframe);
+    // Determine how many keyframes specify each property. Note that we
+    // consider keyframes at offsets of 0 and 1 to always specify all
+    // properties, as values for these properties will be filled in by

[dstockwell, 2013/09/30 07:30:57]

Can't we fill in the end keyframes before counting?

[Steve Block, 2013/09/30 12:17:54]

We could, but in the case that we have to split out any properties, we'd have to
find those newly added property values in the end keyframes and move them to the
new keyframe groups. That approach would eliminate a little code duplication,
but is less efficient and seems an odd way to do it.

Note that in the common case where all non-end keyframes specify all properties,
there's no significant difference between these two approaches.

[dstockwell, 2013/09/30 21:56:44]

It would eliminate lines 973 -> 987, right? I found that section quite
confusing.

I'm not sure being trivially more efficient is worth it for the non-common case,
but it's your call.

[Steve Block, 2013/10/01 04:39:53]

I've updated the patch to handle end keyframes first. We can revert to the
previous approach if we find this approach to be too slow or confusing.

+    // prepareEndKeyframes(). Note that to do this efficiently, we have to do
+    // so after duplicate keyframes have been removed above.
+    typedef HashCountedSet<CSSPropertyID> PropertyCountedSet;
+    PropertyCountedSet propertyCounts;
+    size_t numKeyframes = keyframes.size();
+    for (size_t i = 0; i < numKeyframes; ++i) {
+        const PropertySet& properties = keyframes[i]->properties();
+        for (PropertySet::const_iterator iter = properties.begin(); iter != properties.end(); ++iter)
+            propertyCounts.add(*iter);
     }
-    ASSERT(keyframes.size() >= 2);
-    ASSERT(!keyframes.first()->offset());
-    ASSERT(keyframes.last()->offset() == 1);
 
-    // Generate the chained timing function. Note that timing functions apply
-    // from the keyframe in which they're specified to the next keyframe.
-    // FIXME: Handle keyframe sets where some keyframes don't specify all
-    // properties. In this case, timing functions apply between the keyframes
-    // which specify a particular property, so we'll need a separate chained
-    // timing function (and therefore animation) for each property. See
-    // LayoutTests/animations/missing-keyframe-properties-timing-function.html
-    if (!timingFunctions.contains(0))
-        timingFunctions.set(0, defaultTimingFunction);
-    bool isTimingFunctionLinearThroughout = true;
-    RefPtr<ChainedTimingFunction> chainedTimingFunction = ChainedTimingFunction::create();
-    for (size_t i = 0; i < keyframes.size() - 1; ++i) {
-        double lowerBound = keyframes[i]->offset();
-        ASSERT(lowerBound >=0 && lowerBound < 1);
-        double upperBound = keyframes[i + 1]->offset();
-        ASSERT(upperBound > 0 && upperBound <= 1);
-        TimingFunction* timingFunction = timingFunctions.get(lowerBound);
-        ASSERT(timingFunction);
-        isTimingFunctionLinearThroughout &= timingFunction->type() == TimingFunction::LinearFunction;
-        chainedTimingFunction->appendSegment(upperBound, timingFunction);
+    bool firstKeyframeIsAtOffsetZero = !keyframes.first()->offset();
+    bool lastKeyframeIsAtOffsetOne = keyframes.last()->offset() == 1;
+    if (firstKeyframeIsAtOffsetZero || lastKeyframeIsAtOffsetOne) {
+        const PropertySet& firstKeyframeProperties = keyframes.first()->properties();
+        const PropertySet& lastKeyframeProperties = keyframes.last()->properties();
+        if (firstKeyframeProperties.size() < propertyCounts.size() || lastKeyframeProperties.size() < propertyCounts.size()) {
+            for (HashCountedSet<CSSPropertyID>::const_iterator iter = propertyCounts.begin(); iter != propertyCounts.end(); ++iter) {
+                CSSPropertyID property = iter->key;
+                if (firstKeyframeIsAtOffsetZero && !keyframes.first()->properties().contains(property))
+                    propertyCounts.add(property);
+                if (lastKeyframeIsAtOffsetOne && !keyframes.last()->properties().contains(property))
+                    propertyCounts.add(property);
+            }
+        }
     }
-    if (isTimingFunctionLinearThroughout)
-        timingFunction = LinearTimingFunction::create();
-    else
-        timingFunction = chainedTimingFunction;
 
-    // Snapshot current property values for 0% and 100% if missing.
-    PropertySet allProperties;
-    for (size_t i = 0; i < keyframes.size(); i++) {
-        const PropertySet& keyframeProperties = keyframes[i]->properties();
-        for (PropertySet::const_iterator iter = keyframeProperties.begin(); iter != keyframeProperties.end(); ++iter)
-            allProperties.add(*iter);
-    }
-    const PropertySet& startKeyframeProperties = startKeyframe->properties();
-    const PropertySet& endKeyframeProperties = endKeyframe->properties();
-    bool missingStartValues = startKeyframeProperties.size() < allProperties.size();
-    bool missingEndValues = endKeyframeProperties.size() < allProperties.size();
-    if (!missingStartValues && !missingEndValues)
-        return;
-    for (PropertySet::const_iterator iter = allProperties.begin(); iter != allProperties.end(); ++iter) {
-        const CSSPropertyID property = *iter;
-        bool startNeedsValue = missingStartValues && !startKeyframeProperties.contains(property);
-        bool endNeedsValue = missingEndValues && !endKeyframeProperties.contains(property);
-        if (!startNeedsValue && !endNeedsValue)
+    // Split keyframes into groups, where each group contains only keyframes
+    // which specify all properties used in that group. Each group is animated
+    // in a separate animation, to allow per-keyframe timing functions to be
+    // applied correctly.
+    PropertySet propertiesSpecifiedInAllKeyframes;
+    for (PropertyCountedSet::const_iterator iter = propertyCounts.begin(); iter != propertyCounts.end(); ++iter) {
+        const CSSPropertyID property = iter->key;
+        const size_t count = iter->value;
+        ASSERT(count <= numKeyframes);
+        if (count == numKeyframes) {
+            propertiesSpecifiedInAllKeyframes.add(property);
             continue;
-        RefPtr<AnimatableValue> snapshotValue = CSSAnimatableValueFactory::create(property, style);
-        if (startNeedsValue)
-            startKeyframe->setPropertyValue(property, snapshotValue.get());
-        if (endNeedsValue)
-            endKeyframe->setPropertyValue(property, snapshotValue.get());
+        }
+        KeyframeAnimationEffect::KeyframeVector splitOutKeyframes;
+        for (size_t i = 0; i < numKeyframes; i++) {
+            Keyframe* keyframe = keyframes[i].get();
+            if (!keyframe->properties().contains(property))
+                continue;
+            RefPtr<Keyframe> clonedKeyframe = Keyframe::create();
+            clonedKeyframe->setOffset(keyframe->offset());
+            clonedKeyframe->setComposite(keyframe->composite());
+            clonedKeyframe->setPropertyValue(property, keyframe->propertyValue(property));
+            splitOutKeyframes.append(clonedKeyframe);
+            keyframe->clearPropertyValue(property);
+        }
+        PropertySet properties;
+        properties.add(property);
+        prepareEndKeyframes(style, properties, splitOutKeyframes);
+        keyframesAndTimingFunctions.append(std::make_pair(splitOutKeyframes, generateTimingFunction(splitOutKeyframes, perKeyframeTimingFunctions)));
     }
-    ASSERT(startKeyframe->properties().size() == allProperties.size());
-    ASSERT(endKeyframe->properties().size() == allProperties.size());
+#ifndef NDEBUG
+    size_t numPropertiesSpecifiedInAllKeyframes = propertiesSpecifiedInAllKeyframes.size();
+    for (size_t i = 0; i < keyframes.size(); ++i)
+        ASSERT(!keyframes[i]->offset() || keyframes[i]->offset() == 1 || keyframes[i]->properties().size() == numPropertiesSpecifiedInAllKeyframes);
+#endif
+    prepareEndKeyframes(style, propertiesSpecifiedInAllKeyframes, keyframes);
+    keyframesAndTimingFunctions.append(std::make_pair(keyframes, generateTimingFunction(keyframes, perKeyframeTimingFunctions)));

[dstockwell, 2013/09/30 07:30:57]

We probably don't want to do this if numPropertiesSpecifiedInAllKeyframes is 0
and keyframesAndTimingFunctions is not empty.

[Steve Block, 2013/09/30 12:17:54]

Good point. Done.

 }
 
 PassRefPtr<RenderStyle> StyleResolver::pseudoStyleForElement(Element* e, const PseudoStyleRequest& pseudoStyleRequest, RenderStyle* parentStyle)