- Revision
- 201818
- Author
- d...@apple.com
- Date
- 2016-06-08 11:46:43 -0700 (Wed, 08 Jun 2016)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (201817 => 201818)
--- trunk/LayoutTests/ChangeLog 2016-06-08 18:45:47 UTC (rev 201817)
+++ trunk/LayoutTests/ChangeLog 2016-06-08 18:46:43 UTC (rev 201818)
@@ -1,3 +1,14 @@
+2016-06-08 Dean Jackson <d...@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 <pvol...@apple.com>
[Win] Shadow DOM tests are failing.
Added: trunk/LayoutTests/animations/duplicate-keys-expected.html (0 => 201818)
--- trunk/LayoutTests/animations/duplicate-keys-expected.html (rev 0)
+++ trunk/LayoutTests/animations/duplicate-keys-expected.html 2016-06-08 18:46:43 UTC (rev 201818)
@@ -0,0 +1,38 @@
+<style>
+.box {
+ height: 100px;
+ width: 0px;
+ border-width: 0;
+ border-left-width: 200px;
+ border-right-width: 200px;
+ border-style: solid;
+ border-color: blue;
+ animation: foo 200ms forwards;
+}
+@keyframes foo {
+ 0% {
+ border-left-color: orange;
+ border-right-color: green;
+ }
+ 50% {
+ border-left-color: red;
+ }
+ to {
+ border-left-color: orange;
+ border-right-color: green;
+ }
+}
+
+</style>
+<script>
+if (window.testRunner) {
+ window.addEventListener("load", function () {
+ var box = document.querySelector(".box");
+ internals.pauseAnimationAtTimeOnElement("foo", 0.1, box);
+ }, false);
+}
+</script>
+<p>Left side should go orange -> red -> orange, right side should stay green at the end.</p>
+<div class="box">
+</div>
+
Property changes on: trunk/LayoutTests/animations/duplicate-keys-expected.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Added: trunk/LayoutTests/animations/duplicate-keys.html (0 => 201818)
--- trunk/LayoutTests/animations/duplicate-keys.html (rev 0)
+++ trunk/LayoutTests/animations/duplicate-keys.html 2016-06-08 18:46:43 UTC (rev 201818)
@@ -0,0 +1,42 @@
+<style>
+.box {
+ height: 100px;
+ width: 0px;
+ border-width: 0;
+ border-left-width: 200px;
+ border-right-width: 200px;
+ border-style: solid;
+ border-color: blue;
+ animation: foo 200ms forwards;
+}
+@keyframes foo {
+ 0% {
+ border-left-color: orange;
+ border-right-color: green;
+ }
+ 50% {
+ border-left-color: red;
+ }
+ 50%, to {
+ border-right-color: green;
+ }
+ to {
+ border-left-color: orange;
+ }
+}
+
+</style>
+<script>
+if (window.testRunner) {
+ testRunner.waitUntilDone();
+ window.addEventListener("load", function () {
+ var box = document.querySelector(".box");
+ internals.pauseAnimationAtTimeOnElement("foo", 0.1, box);
+ testRunner.notifyDone();
+ }, false);
+}
+</script>
+<p>Left side should go orange -> red -> orange, right side should stay green at the end.</p>
+<div class="box">
+</div>
+
Property changes on: trunk/LayoutTests/animations/duplicate-keys.html
___________________________________________________________________
Added: svn:mime-type
Added: svn:keywords
Added: svn:eol-style
Modified: trunk/Source/WebCore/ChangeLog (201817 => 201818)
--- trunk/Source/WebCore/ChangeLog 2016-06-08 18:45:47 UTC (rev 201817)
+++ trunk/Source/WebCore/ChangeLog 2016-06-08 18:46:43 UTC (rev 201818)
@@ -1,3 +1,27 @@
+2016-06-08 Dean Jackson <d...@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 <ryanhad...@apple.com>
Rebaseline bindings tests after r201808
Modified: trunk/Source/WebCore/css/CSSKeyframeRule.h (201817 => 201818)
--- trunk/Source/WebCore/css/CSSKeyframeRule.h 2016-06-08 18:45:47 UTC (rev 201817)
+++ trunk/Source/WebCore/css/CSSKeyframeRule.h 2016-06-08 18:46:43 UTC (rev 201818)
@@ -47,6 +47,12 @@
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; };
Modified: trunk/Source/WebCore/css/StyleResolver.cpp (201817 => 201818)
--- trunk/Source/WebCore/css/StyleResolver.cpp 2016-06-08 18:45:47 UTC (rev 201817)
+++ trunk/Source/WebCore/css/StyleResolver.cpp 2016-06-08 18:46:43 UTC (rev 201818)
@@ -513,7 +513,7 @@
{
list.clear();
- // Get the keyframesRule for this name
+ // Get the keyframesRule for this name.
if (list.animationName().isEmpty())
return;
@@ -525,13 +525,54 @@
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));
keyframeValue.setKey(key);
@@ -539,25 +580,25 @@
}
}
- // 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()) {
static StyleKeyframe* zeroPercentKeyframe;
if (!zeroPercentKeyframe) {
zeroPercentKeyframe = &StyleKeyframe::create(MutableStyleProperties::create()).leakRef();
- zeroPercentKeyframe->setKeyText("0%");
+ zeroPercentKeyframe->setKey(0);
}
KeyframeValue keyframeValue(0, nullptr);
keyframeValue.setStyle(styleForKeyframe(elementStyle, zeroPercentKeyframe, keyframeValue));
list.insert(WTFMove(keyframeValue));
}
- // 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);
keyframeValue.setStyle(styleForKeyframe(elementStyle, hundredPercentKeyframe, keyframeValue));
Modified: trunk/Source/WebCore/rendering/style/KeyframeList.cpp (201817 => 201818)
--- trunk/Source/WebCore/rendering/style/KeyframeList.cpp 2016-06-08 18:45:47 UTC (rev 201817)
+++ trunk/Source/WebCore/rendering/style/KeyframeList.cpp 2016-06-08 18:46:43 UTC (rev 201818)
@@ -78,12 +78,10 @@
return;
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;
}
@@ -95,19 +93,9 @@
}
}
- 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())
m_properties.add(property);
}
