820629 - animateTransform shouldn't animate non-transform-flavored attributes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase (shouldn't animate)

When working on a testcase for another bug, I noticed that
    <animateTransform attributeName="width" dur="1s"
                      from="0" to="100" repeatCount="indefinite" />
successfully animates the "width" attribute in my Firefox Nightly build.

That's "animateTransform", animating the "width" attribute.

Google chrome and opera don't animate in this circumstance. I think they're right.

Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20121211 Firefox/20.0

Comment 1

[Cameron McCormack (:heycam)]

Note that we still should animate gradientTransform="" and patternTransform="".

Comment 2

[Daniel Holbert [:dholbert]]

Ah, good point. Broadening summary.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 691204 [details] [diff] [review]
patch

Thanks for taking this -- patch looks great! Just a few nits:

>+bool
>+nsSVGAnimateTransformElement::GetTargetAttributeName(PRInt32 *aNamespaceID,
>+                                                     nsIAtom **aLocalName) const
>+{
>+  if (nsSVGAnimateTransformElementBase::GetTargetAttributeName(aNamespaceID, 

Fix trailing whitespace-------------------------------------------------------^

>+++ b/layout/reftests/svg/smil/transform/animate-width-1.svg
>@@ -0,0 +1,22 @@
>+<?xml version="1.0" encoding="Windows-1252"?>

I've never seen this encoding in SVG files in our codebase before (though now that I search, it looks like we've got a small handful of them).  Is it actually useful/necessary? There doesn't seem to be anything Windows-specific (or any special characters) in this test, AFAICT. Maybe just drop the encoding, or do encoding="UTF-8" to be more general? (Not a big deal, feel free to leave it if you prefer)

>+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"

Add a newline before "xmlns" to keep this under 80 characters.

Comment 5

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> 
> I've never seen this encoding in SVG files in our codebase before (though
> now that I search, it looks like we've got a small handful of them).  Is it
> actually useful/necessary? There doesn't seem to be anything
> Windows-specific (or any special characters) in this test, AFAICT. Maybe
> just drop the encoding, or do encoding="UTF-8" to be more general? (Not a
> big deal, feel free to leave it if you prefer)

It's added automatically by the Visual Studio editor. I strip it out whenever I remember, but sometimes I forget.

Comment 6

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=3799ce68232e

Comment 7

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=54481169f7ae

Comment 8

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf469b6aef6d

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bf469b6aef6d

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 691204 [details] [diff] [review]
patch

Review of attachment 691204 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGAnimateTransformElement.cpp
@@ +42,5 @@
>                          nsAttrValue& aResult);
>  
>    // nsISMILAnimationElement
>    virtual nsSMILAnimationFunction& AnimationFunction();
> +  virtual bool GetTargetAttributeName(PRInt32 *aNamespaceID,

For future reference, int32_t

Comment 11

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8afa0fd12d00

Comment 12

[Daniel Holbert [:dholbert]]

(oops, should've caught that in review -- thanks!)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8afa0fd12d00