Bug 54410 – SVG animation doesn't support attribute value 'inherit'

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

SVG animation doesn't support attribute value 'inherit' for CSS properties.

[Dirk Schulze]

Created attachment 82365 [details]
Patch

[WebKit Review Bot]

Attachment 82365 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7911651

[Dirk Schulze]

Created attachment 82367 [details]
Patch

[Dirk Schulze]

Forgot to mention that this fixes two more tests of the W3C test suite:

http://dev.w3.org/SVG/profiles/1.1F2/test/svg/animate-elem-84-t.svg
http://dev.w3.org/SVG/profiles/1.1F2/test/svg/animate-elem-85-t.svg

[Early Warning System Bot]

Attachment 82365 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7909708

[Nikolas Zimmermann]

Comment on attachment 82367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82367&action=review

Almost r=me, still some comments to be resolved, setting r- for the meanwhile.

> Source/WebCore/ChangeLog:19
> +        (WebCore::SVGAnimateElement::calculateAnimatedValue): Get the computed style of attribute for 'inherit'
> +        or 'currentColor' during the animation, since the values could be animated themselves.

When a property value is 'inherit' or 'currentColor' during the animation, get the computed style of the property since the values could be animated themselves.

> Source/WebCore/ChangeLog:28
> +        the attribute is an animatable CSS property with the use of a CSS property map in SVGStyledElement.

s/with the use of .../by using the CSS property map in SVGStyledElement/.

> Source/WebCore/ChangeLog:30
> +        (WebCore::SVGAnimationElement::setTargetAttributeAnimatedValue): Take targetElement instead of target
> +        for consistency reasons.

s/Take..../ -> s/target/targetElement/ for consistency.

> Source/WebCore/ChangeLog:33
> +        (WebCore::SVGStyledElement::isAnimatableCSSProperty): Check if attribute is a animatable CSS property.

s/Check if/.../ -> Checks if the CSS property is animable.

> Source/WebCore/svg/SVGAnimateElement.cpp:101
> +static inline String adujustForInheritance(SVGElement* targetElement, const String& attributeName)

For consistency: avoid the return value, and use a "String& attributeValue" as parameter?
s/adujust/adjust/

> Source/WebCore/svg/SVGAnimateElement.cpp:103
> +    // FIXME: Needs to handle animation types directly.

What does that mean?

> Source/WebCore/svg/SVGAnimateElement.cpp:163
> +        // Get current values on property value 'currentColor' as well as 'inherit'

Replace 'currentColor' / 'inherit' by their computed property values.

> Source/WebCore/svg/SVGAnimateElement.cpp:192
> +        // Get current values on property value 'currentColor' and 'inherit'

Same comment as I suggested above. And use periods at the end of sentences.

> Source/WebCore/svg/SVGAnimateElement.h:54
> +    // If we have 'currentColor' or 'inherit' as animation value, we need to grep the value during the animation

s/grep/grab/

> Source/WebCore/svg/SVGAnimateElement.h:56
> +    enum BasicValueChange {

Why BasicValueChange? Doesn't make a lot of sense to me.
enum AnimatedPropertyValueType {
RegularPropertyValue,
CurrentColorValue,
InheritValue
}; ?

> Source/WebCore/svg/SVGAnimateElement.h:59
> +        constantBasicValue,
> +        currentColorBasicValue,
> +        inheritBasicValue

Capitalize those enum values.

[Dirk Schulze]

Created attachment 82428 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 82428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82428&action=review

r=me. Great job!

> Source/WebCore/svg/SVGAnimateElement.cpp:299
> +static bool isCurrentColor(const String& value)

attributeValueIsCurrentColor?

[Dirk Schulze]

Committed r78543: <http://trac.webkit.org/changeset/78543>

[Dirk Schulze]

Committed r78552: <http://trac.webkit.org/changeset/78552>

[WebKit Review Bot]

http://trac.webkit.org/changeset/78560 might have broken GTK Linux 64-bit Debug