animateTransform shouldn't animate non-transform-flavored attributes

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dholbert, Assigned: longsonr)

Tracking

Trunk
mozilla20
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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
Note that we still should animate gradientTransform="" and patternTransform="".
Ah, good point. Broadening summary.
Summary: animateTransform shouldn't animate attributes other than "transform" → animateTransform shouldn't animate non-transform-flavored attributes
Posted patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #691204 - Flags: review?(dholbert)
Attachment #691204 - Attachment is patch: true
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.
Attachment #691204 - Flags: review?(dholbert) → review+
(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.
OS: Linux → All
Hardware: x86_64 → All
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/bf469b6aef6d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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
(oops, should've caught that in review -- thanks!)
You need to log in before you can comment on or make changes to this bug.