For animation to work with the "auto" attributeType, we need a way to check whether a particular string is the name of a CSS property in SVG. According to SVG 1.1 Section 19.2.5, attributeType="auto" means: > The implementation should match the attributeName to an attribute for the > target element. The implementation must first search through the list of > CSS properties for a matching property name, and if none is found, search > the default XML namespace for the element. Currently, we use IsAttributeMapped() to test for CSS-property-ness, here: http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationController.cpp#480 But IsAttributeMapped() returns false (& makes us do the wrong thing) for these CSS properties: "color-interpolation-filters", "flood-color", "flood-opacity", and "font". (probably others, as well) "nsCSSProps::LookupProperty" is not an acceptable replacement -- that would give us the wrong behavior for properties used in HTML that aren't honored by SVG. (For example, an animation with attributeName="width" attributeType="auto" *should* target the SVG width _attribute_, since there's no width _property_ in CSS.) In bug 474049 comment 82, Brian Birtles proposed that we add a method nsISMILAnimationElement::IsCSSProperty to encapsulate this implementation, which would be implemented by the concrete class nsSVGAnimationElement. This allows the "host language" (SVG) to manage the implementation, instead of SMIL code having to do it. This sounds like a good strategy to me. This method could perhaps start out using either of the approximations mentioned above (IsAttributeMapped / LookupProperty), with some special-case-handling code for the properties that we get wrong.

This "auto" feature strikes me as a really bad idea. It means that in the future if we add any CSS properties which have the same name as any current or future SVG attribute names, we run the risk of breaking Web content. That's a pretty big constraint on platform evolution. Could we possibly deprecate/remove it in the spec and not implement it?

(In reply to comment #0) > since there's > no width _property_ in CSS.) Sorry -- of course I meant s/CSS/SVG/ there. (meant to say: there's no width _property_ in SVG) (In reply to comment #1) > Could we possibly deprecate/remove it in the spec and not implement it? As bad as attributeType="auto" is, it's the default value for that attribute. It seems to me like that would make it hard to deprecate.

(In reply to comment #0) > there's no width _property_ in SVG. The CSS width and height properties do have meaning on outer <svg>, and in that case the attributes are NOT mapped into style (they act as intrinsic dimensions). It's the only case I'm aware of where SVG attribute names are the same as CSS property names without the attributes being mapped into style. For this reason the SVG WG was even talking about dropping the attributeType attribute altogether. I guess from what roc said that would be a bad idea though.

I'll post to www-svg about this. For now, I suggest we treat the default value as meaning attribute, and require "CSS" for CSS properties, and leave this bug alone.

I've sort of changed my mind. I think attributeType="auto" is bad, but tolerable looking forward, so we should probably just do it. The spec does list the animatable properties explicitly. We need to have that list in our code; new CSS properties will not automatically be added to it, the SVG spec will have to change before a property becomes animatable. Maybe the code we already have is OK?

(In reply to comment #5) > Maybe the code we already have is OK? Sorry, I wasn't thinking. The code is clearly not OK as-is. I think we just need a hardcoded list somewhere, or possibly a new field or flag in nsCSSPropList?

There's a hard-coded list of animatable properties in my "smil_css" patch on bug 474049 -- it's in nsSMILCSSUtils::GetSMILTypeForProperty. That method (combined with nsCSSProps::LookupProperty, to convert string --> property ID) should do the trick. I'm updating that patch to use this strategy (and fix this bug) right now.

(In reply to comment #7) > I'm updating that patch to use this strategy (and fix this bug) right now. Done, with tests: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/cb43eda985a8 So, this will be fixed in bug 474049 (where that patch, smil_css, will eventually land). How should I resolve this bug -- as a dupe of bug 474049? Or maybe as INVALID, with a dependency on bug 474049?

It's valid. The dependency is marked, and that's all that's needed.

Fixed by check-in for bug 474049.

Right, thanks for closing!