Introduce SMIL overrideStyle, to make SVG stop mutating CSS styles directly
authorzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 4 Mar 2012 19:07:52 +0000 (19:07 +0000)
committerzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 4 Mar 2012 19:07:52 +0000 (19:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=79790

Reviewed by Dirk Schulze.

Source/WebCore:

Restore SMIL animation behavior back to pre-r109342.
1) Always animate presentation attributes as CSS properties, regardless of the
attributeTypes value. Matches Opera/FF, and makes the attribute optional again
as specified in both SMIL & SVG.

Extend existing svg/animations/attributesTypes.html to verify this.

2) Switch setInstancesUpdatesBlocked calls to the right locations again, to
avoid chromium assertions (and/or extra work being done). Fixing that reveals another
problem: in the instance updating code path, we always called setTargetAttributeAnimatedCSSValue
even for XML animations.

* svg/SVGAnimationElement.cpp:
(WebCore::SVGAnimationElement::shouldApplyAnimation): Restore old logic.
(WebCore::SVGAnimationElement::setTargetAttributeAnimatedValue): Fix typo, move setInstancesUpdatesBlocked calls.

LayoutTests:

Extended attributesTypes.html test case to check that presentation attributes are
always animated as CSS properties, regardless of their attributeTypes value.

* platform/chromium/test_expectations.txt: Remove animate-elem-32-t.svg suppression.
* svg/animations/attributeTypes-expected.txt: Updated expectations.
* svg/animations/resources/attributeTypes.svg: Fix comment.
* svg/animations/script-tests/attributeTypes.js: Extend test.
(sample1):
(sample2):
(sample3):

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

LayoutTests/ChangeLog
LayoutTests/platform/chromium/test_expectations.txt
LayoutTests/svg/animations/attributeTypes-expected.txt
LayoutTests/svg/animations/resources/attributeTypes.svg
LayoutTests/svg/animations/script-tests/attributeTypes.js
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGAnimationElement.cpp

index 52de0e8..b5986e9 100644 (file)
@@ -1,3 +1,21 @@
+2012-03-04  Nikolas Zimmermann  <nzimmermann@rim.com>
+
+        Introduce SMIL overrideStyle, to make SVG stop mutating CSS styles directly
+        https://bugs.webkit.org/show_bug.cgi?id=79790
+
+        Reviewed by Dirk Schulze.
+
+        Extended attributesTypes.html test case to check that presentation attributes are
+        always animated as CSS properties, regardless of their attributeTypes value.
+
+        * platform/chromium/test_expectations.txt: Remove animate-elem-32-t.svg suppression.
+        * svg/animations/attributeTypes-expected.txt: Updated expectations.
+        * svg/animations/resources/attributeTypes.svg: Fix comment.
+        * svg/animations/script-tests/attributeTypes.js: Extend test.
+        (sample1):
+        (sample2):
+        (sample3):
+
 2012-03-03  Raymond Toy  <rtoy@google.com>
 
         DelayNode has a fixed one second max delay time
index d098e86..c118d0a 100644 (file)
@@ -4196,8 +4196,6 @@ BUGWK79863 WIN : svg/W3C-SVG-1.1/animate-elem-37-t.svg = IMAGE
 // EPOGER: To be rebaselined after skia rev. 3283 lands in webkit
 BUGCR116252 LINUX : platform/chromium-linux/fast/text/chromium-linux-fontconfig-renderstyle.html = IMAGE+TEXT
 
-BUGWK80053 DEBUG : svg/W3C-SVG-1.1/animate-elem-32-t.svg = CRASH PASS
-
 BUGWK80058 WIN RELEASE : fast/workers/stress-js-execution.html = CRASH PASS
 
 BUGWK80067 DEBUG : media/track/track-cues-pause-on-exit.html = PASS TEXT
index 951e370..35d803d 100644 (file)
@@ -12,32 +12,40 @@ PASS rect2.width.animVal.value is 100
 PASS getComputedStyle(rect2).fill is "#008000"
 PASS rect3.width.animVal.value is 100
 PASS getComputedStyle(rect3).fill is "#ff0000"
+PASS rect3.getAttribute('fill') is "red"
 PASS rect4.width.animVal.value is 100
 PASS getComputedStyle(rect4).fill is "#ff0000"
+PASS rect4.getAttribute('fill') is "red"
 PASS rect1.width.animVal.value is 55
 PASS getComputedStyle(rect1).fill is "#008000"
 PASS rect2.width.animVal.value is 100
 PASS getComputedStyle(rect2).fill is "#008000"
 PASS rect3.width.animVal.value is 100
 PASS getComputedStyle(rect3).fill is "#804000"
+PASS rect3.getAttribute('fill') is "red"
 PASS rect4.width.animVal.value is 100
 PASS getComputedStyle(rect4).fill is "#804000"
+PASS rect4.getAttribute('fill') is "red"
 PASS rect1.width.animVal.value is 100
 PASS getComputedStyle(rect1).fill is "#008000"
 PASS rect2.width.animVal.value is 100
 PASS getComputedStyle(rect2).fill is "#008000"
 PASS rect3.width.animVal.value is 100
 PASS getComputedStyle(rect3).fill is "#008000"
+PASS rect3.getAttribute('fill') is "red"
 PASS rect4.width.animVal.value is 100
 PASS getComputedStyle(rect4).fill is "#008000"
+PASS rect4.getAttribute('fill') is "red"
 PASS rect1.width.animVal.value is 100
 PASS getComputedStyle(rect1).fill is "#008000"
 PASS rect2.width.animVal.value is 100
 PASS getComputedStyle(rect2).fill is "#008000"
 PASS rect3.width.animVal.value is 100
 PASS getComputedStyle(rect3).fill is "#008000"
+PASS rect3.getAttribute('fill') is "red"
 PASS rect4.width.animVal.value is 100
 PASS getComputedStyle(rect4).fill is "#008000"
+PASS rect4.getAttribute('fill') is "red"
 PASS successfullyParsed is true
 
 TEST COMPLETE
index d268c41..35ba433 100644 (file)
@@ -17,7 +17,7 @@
     <animate id="an3" attributeType="auto" attributeName="fill" fill="freeze" from="red" to="green" begin="0s" dur="4s"/>
 </rect>
 
-<!-- 'fill' is a presentation attribute, mapped to CSS, attributeType is set to "CSS": this animation runs. -->
+<!-- 'fill' is a presentation attribute, mapped to CSS, attributeType is set to "XML": this animation runs. -->
 <rect x="150" y="150" width="100" height="100" fill="red">
     <animate id="an4" attributeType="XML" attributeName="fill" fill="freeze" from="red" to="green" begin="0s" dur="4s"/>
 </rect>
index 8c142e2..468ae87 100644 (file)
@@ -14,10 +14,12 @@ function sample1() {
     shouldBe("rect3.width.animVal.value", "100");
     //shouldBe("rect3.width.baseVal.value", "100");
     shouldBeEqualToString("getComputedStyle(rect3).fill", "#ff0000");
+    shouldBeEqualToString("rect3.getAttribute('fill')", "red");
 
     shouldBe("rect4.width.animVal.value", "100");
     //shouldBe("rect4.width.baseVal.value", "100");
     shouldBeEqualToString("getComputedStyle(rect4).fill", "#ff0000");
+    shouldBeEqualToString("rect4.getAttribute('fill')", "red");
 }
 
 function sample2() {
@@ -32,10 +34,12 @@ function sample2() {
     shouldBe("rect3.width.animVal.value", "100");
     //shouldBe("rect3.width.baseVal.value", "100");
     shouldBeEqualToString("getComputedStyle(rect3).fill", "#804000");
+    shouldBeEqualToString("rect3.getAttribute('fill')", "red");
 
     shouldBe("rect4.width.animVal.value", "100");
     //shouldBe("rect4.width.baseVal.value", "100");
     shouldBeEqualToString("getComputedStyle(rect4).fill", "#804000");
+    shouldBeEqualToString("rect4.getAttribute('fill')", "red");
 }
 
 function sample3() {
@@ -50,10 +54,12 @@ function sample3() {
     shouldBe("rect3.width.animVal.value", "100");
     //shouldBe("rect3.width.baseVal.value", "100");
     shouldBeEqualToString("getComputedStyle(rect3).fill", "#008000");
+    shouldBeEqualToString("rect3.getAttribute('fill')", "red");
 
     shouldBe("rect4.width.animVal.value", "100");
     //shouldBe("rect4.width.baseVal.value", "100");
     shouldBeEqualToString("getComputedStyle(rect4).fill", "#008000");
+    shouldBeEqualToString("rect4.getAttribute('fill')", "red");
 }
 
 function executeTest() {
index b099fca..e25f661 100644 (file)
@@ -1,3 +1,26 @@
+2012-03-04  Nikolas Zimmermann  <nzimmermann@rim.com>
+
+        Introduce SMIL overrideStyle, to make SVG stop mutating CSS styles directly
+        https://bugs.webkit.org/show_bug.cgi?id=79790
+
+        Reviewed by Dirk Schulze.
+
+        Restore SMIL animation behavior back to pre-r109342.
+        1) Always animate presentation attributes as CSS properties, regardless of the
+        attributeTypes value. Matches Opera/FF, and makes the attribute optional again
+        as specified in both SMIL & SVG.
+
+        Extend existing svg/animations/attributesTypes.html to verify this.
+
+        2) Switch setInstancesUpdatesBlocked calls to the right locations again, to
+        avoid chromium assertions (and/or extra work being done). Fixing that reveals another
+        problem: in the instance updating code path, we always called setTargetAttributeAnimatedCSSValue
+        even for XML animations.
+
+        * svg/SVGAnimationElement.cpp:
+        (WebCore::SVGAnimationElement::shouldApplyAnimation): Restore old logic.
+        (WebCore::SVGAnimationElement::setTargetAttributeAnimatedValue): Fix typo, move setInstancesUpdatesBlocked calls.
+
 2012-03-04  Jonathan Dong  <jonathan.dong@torchmobile.com.cn>
 
         [BlackBerry] upstream MediaPlayerPrivateBlackBerry.[cpp|h]
index cf5d9ad..dd6501b 100644 (file)
@@ -357,13 +357,16 @@ void SVGAnimationElement::setTargetAttributeAnimatedValue(const String& value)
     if (shouldApply == DontApplyAnimation)
         return;
 
+    if (targetElement->isStyled())
+        static_cast<SVGStyledElement*>(targetElement)->setInstanceUpdatesBlocked(true);
+
     if (shouldApply == ApplyCSSAnimation)
         setTargetAttributeAnimatedCSSValue(targetElement, attributeName, value);
     else
         setTargetAttributeAnimatedXMLValue(targetElement, attributeName, value);
 
     if (targetElement->isStyled())
-        static_cast<SVGStyledElement*>(targetElement)->setInstanceUpdatesBlocked(true);
+        static_cast<SVGStyledElement*>(targetElement)->setInstanceUpdatesBlocked(false);
 
     // If the target element has instances, update them as well, w/o requiring the <use> tree to be rebuilt.
     const HashSet<SVGElementInstance*>& instances = targetElement->instancesForElement();
@@ -376,11 +379,8 @@ void SVGAnimationElement::setTargetAttributeAnimatedValue(const String& value)
         if (shouldApply == ApplyCSSAnimation)
             setTargetAttributeAnimatedCSSValue(shadowTreeElement, attributeName, value);
         else
-            setTargetAttributeAnimatedCSSValue(shadowTreeElement, attributeName, value);
+            setTargetAttributeAnimatedXMLValue(shadowTreeElement, attributeName, value);
     }
-
-    if (targetElement->isStyled())
-        static_cast<SVGStyledElement*>(targetElement)->setInstanceUpdatesBlocked(false);
 }
 
 void SVGAnimationElement::resetAnimationState(const String& baseValue)
@@ -403,24 +403,15 @@ SVGAnimationElement::ShouldApplyAnimation SVGAnimationElement::shouldApplyAnimat
     if (!hasValidAttributeType() || !targetElement || attributeName == anyQName())
         return DontApplyAnimation;
 
-    AttributeType attributeType = this->attributeType();
-    switch (attributeType) {
-    case AttributeTypeAuto:
-        // For attributeType="auto", try CSS first. If that fails, try XML.
-    case AttributeTypeCSS:
-        // If attributeName is a known animatable SVG CSS property, apply the animation.
-        if (isTargetAttributeCSSProperty(targetElement, attributeName))
-            return ApplyCSSAnimation;
-        // If attributeName is unknown and ttributeType is not 'auto', don't apply the animation.
-        if (attributeType == AttributeTypeCSS)
-            return DontApplyAnimation;
-        // For attributeType="auto", try XML animation now.
-    case AttributeTypeXML:
-        return ApplyXMLAnimation;
-    };
+    // Always animate CSS properties, using the ApplyCSSAnimation code path, regardless of the attributeType value.
+    if (isTargetAttributeCSSProperty(targetElement, attributeName))
+        return ApplyCSSAnimation;
 
-    ASSERT_NOT_REACHED();
-    return DontApplyAnimation;
+    // If attributeType="CSS" and attributeName doesn't point to a CSS property, ignore the animation.
+    if (attributeType() == AttributeTypeCSS)
+        return DontApplyAnimation;
+
+    return ApplyXMLAnimation;
 }
 
 void SVGAnimationElement::calculateKeyTimesForCalcModePaced()