Changeset 201818 in webkit

Timestamp:

Jun 8, 2016 11:46:43 AM (8 years ago)

Author:

dino@apple.com

Message:

Multiple selectors break keyframes animation
​https://bugs.webkit.org/show_bug.cgi?id=158199
<rdar://problem/26652591>

Reviewed by Simon Fraser.

Source/WebCore:

If we came across a duplicate key entry in a keyframe, we
were replacing the existing entry, instead of merging.

Test: animations/duplicate-keys.html

• css/CSSKeyframeRule.h:

(WebCore::StyleKeyframe::setKey): Add a way to set the key of a rule
as a number, rather than going through a string and the CSS parser.

• css/StyleResolver.cpp:

(WebCore::StyleResolver::keyframeStylesForAnimation): Check if the rule
has duplicates, and if it does, merge all the common entries.

• rendering/style/KeyframeList.cpp:

(WebCore::KeyframeList::insert): Now that we've removed duplicates at
the processing time, we should never come across a duplicate while
building this list.

LayoutTests:

• animations/duplicate-keys-expected.html: Added.
• animations/duplicate-keys.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/animations/duplicate-keys-expected.html (added)
• LayoutTests/animations/duplicate-keys.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/css/CSSKeyframeRule.h (modified) (1 diff)
• Source/WebCore/css/StyleResolver.cpp (modified) (5 diffs)
• Source/WebCore/rendering/style/KeyframeList.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-06-08  Dean Jackson  <dino@apple.com>
+
+        Multiple selectors break keyframes animation
+        https://bugs.webkit.org/show_bug.cgi?id=158199
+        <rdar://problem/26652591>
+
+        Reviewed by Simon Fraser.
+
+        * animations/duplicate-keys-expected.html: Added.
+        * animations/duplicate-keys.html: Added.
+
 2016-06-08  Per Arne Vollan  <pvollan@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-06-08  Dean Jackson  <dino@apple.com>
+
+        Multiple selectors break keyframes animation
+        https://bugs.webkit.org/show_bug.cgi?id=158199
+        <rdar://problem/26652591>
+
+        Reviewed by Simon Fraser.
+
+        If we came across a duplicate key entry in a keyframe, we
+        were replacing the existing entry, instead of merging.
+
+        Test: animations/duplicate-keys.html
+
+        * css/CSSKeyframeRule.h:
+        (WebCore::StyleKeyframe::setKey): Add a way to set the key of a rule
+        as a number, rather than going through a string and the CSS parser.
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::keyframeStylesForAnimation): Check if the rule
+        has duplicates, and if it does, merge all the common entries.
+        * rendering/style/KeyframeList.cpp:
+        (WebCore::KeyframeList::insert): Now that we've removed duplicates at
+        the processing time, we should never come across a duplicate while
+        building this list.
+
 2016-06-08  Ryan Haddad  <ryanhaddad@apple.com>
 

trunk/Source/WebCore/css/CSSKeyframeRule.h

@@ -48 +48 @@
     String keyText() const;
     void setKeyText(const String& text) { m_keys = CSSParser::parseKeyframeSelector(text); }
+    void setKey(double key)
+    {
+        ASSERT(m_keys.isEmpty());
+        m_keys.clear();
+        m_keys.append(key);
+    }
 
     const Vector<double>& keys() const { return m_keys; };

trunk/Source/WebCore/css/StyleResolver.cpp

@@ -514 +514 @@
     list.clear();
 
-    // Get the keyframesRule for this name
+    // Get the keyframesRule for this name.
     if (list.animationName().isEmpty())
         return;
@@ -526 +526 @@
     const StyleRuleKeyframes* keyframesRule = it->value.get();
 
-    // Construct and populate the style for each keyframe
-    for (auto& keyframe : keyframesRule->keyframes()) {
-        // Apply the declaration to the style. This is a simplified version of the logic in styleForElement
+    auto* keyframes = &keyframesRule->keyframes();
+    Vector<Ref<StyleKeyframe>> newKeyframesIfNecessary;
+
+    bool hasDuplicateKeys;
+    HashSet<double> keyframeKeys;
+    for (auto& keyframe : *keyframes) {
+        for (auto key : keyframe->keys()) {
+            if (!keyframeKeys.add(key)) {
+                hasDuplicateKeys = true;
+                break;
+            }
+        }
+        if (hasDuplicateKeys)
+            break;
+    }
+
+    // FIXME: If HashMaps could have Ref<> as value types, we wouldn't need
+    // to copy the HashMap into a Vector.
+    if (hasDuplicateKeys) {
+        // Merge duplicate key times.
+        HashMap<double, RefPtr<StyleKeyframe>> keyframesMap;
+
+        for (auto& originalKeyframe : keyframesRule->keyframes()) {
+            for (auto key : originalKeyframe->keys()) {
+                if (auto keyframe = keyframesMap.get(key))
+                    keyframe->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
+                else {
+                    auto styleKeyframe = StyleKeyframe::create(MutableStyleProperties::create());
+                    styleKeyframe.ptr()->setKey(key);
+                    styleKeyframe.ptr()->mutableProperties().mergeAndOverrideOnConflict(originalKeyframe->properties());
+                    keyframesMap.set(key, styleKeyframe.ptr());
+                }
+            }
+        }
+
+        for (auto& keyframe : keyframesMap.values())
+            newKeyframesIfNecessary.append(*keyframe.get());
+
+        keyframes = &newKeyframesIfNecessary;
+    }
+
+    // Construct and populate the style for each keyframe.
+    for (auto& keyframe : *keyframes) {
+        // Apply the declaration to the style. This is a simplified version of the logic in styleForElement.
         m_state = State(element, nullptr);
 
         // Add this keyframe style to all the indicated key times
-        for (auto& key : keyframe->keys()) {
+        for (auto key : keyframe->keys()) {
             KeyframeValue keyframeValue(0, nullptr);
             keyframeValue.setStyle(styleForKeyframe(elementStyle, keyframe.ptr(), keyframeValue));
@@ -540 +581 @@
     }
 
-    // If the 0% keyframe is missing, create it (but only if there is at least one other keyframe)
+    // If the 0% keyframe is missing, create it (but only if there is at least one other keyframe).
     int initialListSize = list.size();
     if (initialListSize > 0 && list[0].key()) {
@@ -546 +587 @@
         if (!zeroPercentKeyframe) {
             zeroPercentKeyframe = &StyleKeyframe::create(MutableStyleProperties::create()).leakRef();
-            zeroPercentKeyframe->setKeyText("0%");
+            zeroPercentKeyframe->setKey(0);
         }
         KeyframeValue keyframeValue(0, nullptr);
@@ -553 +594 @@
     }
 
-    // If the 100% keyframe is missing, create it (but only if there is at least one other keyframe)
+    // If the 100% keyframe is missing, create it (but only if there is at least one other keyframe).
     if (initialListSize > 0 && (list[list.size() - 1].key() != 1)) {
         static StyleKeyframe* hundredPercentKeyframe;
         if (!hundredPercentKeyframe) {
             hundredPercentKeyframe = &StyleKeyframe::create(MutableStyleProperties::create()).leakRef();
-            hundredPercentKeyframe->setKeyText("100%");
+            hundredPercentKeyframe->setKey(1);
         }
         KeyframeValue keyframeValue(1, nullptr);

trunk/Source/WebCore/rendering/style/KeyframeList.cpp

@@ -79 +79 @@
 
     bool inserted = false;
-    bool replaced = false;
     size_t i = 0;
     for (; i < m_keyframes.size(); ++i) {
         if (m_keyframes[i].key() == keyframe.key()) {
-            m_keyframes[i] = WTFMove(keyframe);
-            replaced = true;
+            ASSERT_NOT_REACHED();
             break;
         }
@@ -96 +94 @@
     }
     
-    if (!replaced && !inserted)
+    if (!inserted)
         m_keyframes.append(WTFMove(keyframe));
-
-    if (replaced) {
-        // We have to rebuild the properties list from scratch.
-        m_properties.clear();
-        for (auto& keyframeValue : m_keyframes) {
-            for (auto& property : keyframeValue.properties())
-                m_properties.add(property);
-        }
-        return;
-    }
 
     for (auto& property : m_keyframes[i].properties())