Last Comment Bug 534028 - SVG SMIL: Add support for animating mapped attributes
: SVG SMIL: Add support for animating mapped attributes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a4
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
http://people.mozilla.org/~dholbert/t...
Depends on: 552873 474049 566777
Blocks: 1062106 436276 904158
  Show dependency treegraph
 
Reported: 2009-12-10 12:32 PST by Daniel Holbert [:dholbert]
Modified: 2014-09-02 23:12 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch A: Mochitests (21.63 KB, patch)
2010-01-21 14:35 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
(Behavior 1) Patch B: Force attributeType="CSS" for Presentational Attrs (1.06 KB, patch)
2010-01-21 14:39 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
(Behavior 2) Patch B: Make GetAnimatedAttr() non-const (4.92 KB, patch)
2010-01-21 14:41 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
(Behavior 2) Patch C: Animate presentational attrs separately (25.95 KB, patch)
2010-01-21 14:49 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
Patch A: Make GetAnimatedAttr()'s nsIAtom argument non-const (4.92 KB, patch)
2010-01-25 18:48 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Review
Patch B: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule) (7.29 KB, patch)
2010-01-25 18:55 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
Patch C: Add nsSMILMappedAttribute & animated content style rule (19.90 KB, patch)
2010-01-25 19:05 PST, Daniel Holbert [:dholbert]
roc: superreview+
Details | Diff | Review
Patch D: Tests (30.65 KB, patch)
2010-01-25 19:06 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Review
Patch B v2: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule) (7.46 KB, patch)
2010-01-26 03:16 PST, Daniel Holbert [:dholbert]
roc: superreview+
Details | Diff | Review
Patch C v2: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc] (19.86 KB, patch)
2010-01-26 03:20 PST, Daniel Holbert [:dholbert]
dholbert: review+
dholbert: superreview+
Details | Diff | Review
Patch B v3: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule) [sr=roc] (8.07 KB, patch)
2010-01-26 18:37 PST, Daniel Holbert [:dholbert]
dholbert: superreview+
Details | Diff | Review
Patch C v3: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc] (19.67 KB, patch)
2010-01-29 16:31 PST, Daniel Holbert [:dholbert]
dholbert: review+
dholbert: superreview+
Details | Diff | Review
Patch C v3a: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc] (20.09 KB, patch)
2010-02-24 11:27 PST, Daniel Holbert [:dholbert]
dholbert: review+
dholbert: superreview+
Details | Diff | Review
Patch B v4: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule) [sr=roc] (7.17 KB, patch)
2010-03-05 12:58 PST, Daniel Holbert [:dholbert]
dbaron: review+
dholbert: superreview+
Details | Diff | Review
Patch C v3b: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc] (19.79 KB, patch)
2010-03-05 13:01 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Review

Description Daniel Holbert [:dholbert] 2009-12-10 12:32:03 PST
Filing this bug on supporting SMIL animation of the SVG attributes that are mapped to CSS properties.
Comment 1 Jonathan Watt [:jwatt] 2009-12-10 12:44:21 PST
The method I showed you earlier is nsSVGElement::UpdateContentStyleRule()

http://hg.mozilla.org/mozilla-central/annotate/ed6b195bae6d/content/svg/content/src/nsSVGElement.cpp#l1001
Comment 2 Daniel Holbert [:dholbert] 2009-12-10 12:49:46 PST
Cool, thanks.

For this bug, I think we just need to:
  (a) Create a subclass of nsSMILCSSProperty that overrides GetBaseValue, SetAnimValue, and ClearAnimValue.  These subclass methods would need to access the attribute on the target element, rather than the CSS property & SMILOverrideStyle. (The subclassed SetAnimValue / ClearAnimValue would also need to call the method that jwatt referenced in comment 1.)
  (b) Modify nsSMILCompositor::CreateSMILAttr() to reurn an instance of the above subclass, for mapped SVG attributes.
Comment 3 Daniel Holbert [:dholbert] 2010-01-08 15:59:30 PST
So, one outstanding question not addressed in Comment 2 is: where should we store the animated attribute-value?  i.e. where does nsISMILAttr::SetAnimValue "commit" the animated value to, in each sample?

To do this, we need a structure on each animation-targeted element that can store an animated value (a string) for each mapped attribute.

We could, of course, do this by adding a new member-variable to the nsSVGElement class.  But that would make every SVG element a bit bigger (by at least sizeof(void*)), and this new structure probably won't even be used by most SVG elements, anyway.  So, I think it'd be better to glom these animated values onto an already-existing chunk of member data, in a way that won't take up any space in the non-animated case.

SO: I'm currently thinking of storing them in mAttrsAndChildren, as an nsAttrValue with a new nsAttrValue::ValueType. (eAnimatedString or something)

I think this would require some new animation-specific methods to nsAttrAndChildArray, to specifically Get/Set the *animated* nsAttrValue for a particular attribute-name.  It'd also require that the existing methods on that class -- e.g. GetAttr, SetAttr, IndexOfAttr -- be modified to explicitly skip any animated-type entries that they find.  These animated values would then trivially take effect, becuase UpdateContentStyleRule already iterates through mAttrsAndChildren and parses all the mapped attributes it finds there.  (UpdateContentStyleRule would just need a minor tweak to make sure it preferentially applies the _animated_ version over the _non-animated_ version.)

Is this reasonable?  Are there other solutions that would be better?
Comment 4 Boris Zbarsky [:bz] 2010-01-08 20:02:51 PST
That sounds like it would slow down GetAttr/SetAttr somewhat for all consumers, right?

Is there a reason to not just store the animated stuff as properties on the node, if it's expected to be rare?
Comment 5 Daniel Holbert [:dholbert] 2010-01-11 13:47:54 PST
(In reply to comment #4)
> That sounds like it would slow down GetAttr/SetAttr somewhat for all consumers,
> right?

Yeah, I suppose the "explicitly skip any animated-type entries" part (from comment 3) would require additional type-checking and would slow down GetAttr/SetAttr.

(In reply to comment #4 again)
> Is there a reason to not just store the animated stuff as properties on the
> node, if it's expected to be rare?

Ah, via nsINode::SetProperty/UnsetProperty/GetProperty/DeleteProperty.  That looks like a nice possibility.  Seems like I could just define a new "property category" in nsPropertyTable.h [1], and then (with that category) I could use the attribute-name as the property key and the animated value as the attribute-value.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsPropertyTable.h#99
Comment 6 Daniel Holbert [:dholbert] 2010-01-11 15:11:03 PST
er, s/attribute-value/property value/.
Comment 7 Daniel Holbert [:dholbert] 2010-01-21 14:32:23 PST
So, after mostly finishing this bug's patch, I realized that the spec has a paragraph that contradicts my expectations about how this should work.  

Apparently the spec says that animations on Presentational Attributes (e.g. <animate attributeName="fill" attributeType="XML"...>) should have "the same behavior" as animations on the CSS property -- that is, we should just ignore |attributeType|, and always behave as if attributeType="CSS" for animations on mapped attributes.

* Adobe SVG viewer matches the specified behavior. (ignores attributeType)
* Opera matches my initial expectations. (treats "CSS" and "XML" differently)
* Webkit has a strange mix of the two behaviors.

I just posted to www-svg about this, with more detail:
  http://lists.w3.org/Archives/Public/www-svg/2010Jan/0066.html
  http://lists.w3.org/Archives/Public/www-svg/2010Jan/0067.html

I've got patches for both behaviors. (I initially wrote a patch that matched my expectations & Opera's behavior, and then upon discovering the spec issue, I wrote a separate trivial patch to do what it calls for -- to just ignore attributeType for mapped attrs).  Not sure which we should use -- I'll post both here in a sec.
Comment 8 Daniel Holbert [:dholbert] 2010-01-21 14:35:06 PST
Created attachment 422835 [details] [diff] [review]
Patch A: Mochitests
Comment 9 Daniel Holbert [:dholbert] 2010-01-21 14:39:49 PST
Created attachment 422837 [details] [diff] [review]
(Behavior 1) Patch B: Force attributeType="CSS" for Presentational Attrs

Here's a patch that implements "Behavior 1" from my first www-svg post. (matching the spec & Adobe SVG Viewer)
Comment 10 Daniel Holbert [:dholbert] 2010-01-21 14:41:05 PST
Created attachment 422839 [details] [diff] [review]
(Behavior 2) Patch B: Make GetAnimatedAttr() non-const

Here's a helper patch needed for "Behavior 2"...
Comment 11 Daniel Holbert [:dholbert] 2010-01-21 14:49:52 PST
Created attachment 422842 [details] [diff] [review]
(Behavior 2) Patch C: Animate presentational attrs separately

... and this is the main patch for "Behavior 2".

Two TODOs for this patch, **if we end up choosing its behavior**:
 - Need a non-hacky implementation for "nsSMILMappedAttribute::FlushChangesToTargetAttr". (It's a hack right now -- it just needs to clear the target element's mContentStyleRule, and there's no direct way to do that right now, particularly when starting with a nsIContent* pointer.)
 - Bug: if we do "by-animation" of the "XML" and "CSS" version of e.g. 'fill' simultaneously, with no inline-style "base value" to mask the XML animation, then the XML animation's effects will "bleed" into the CSS animation's base value.  To fix this, we'd really need to *clear* the XML animation's animated-value, when we look up the CSS animation's base value.
Comment 12 Daniel Holbert [:dholbert] 2010-01-21 14:56:28 PST
With the "Behavior 1" patch, we match Adobe SVG Viewer on the "xmlVsCssChart_v1.svg" testcase that I included in my second www-svg post:
http://people.mozilla.org/~dholbert/tests/smil/compat_tests/xmlVsCssChart_v1.svg

With the "Behavior 2" patches, we match Opera on the "xmlVsCssChart_v1.svg"  testcase, with the exception of the upper-left rect -- we render that one as gray, due to the second TODO/bug mentioned in my previous comment.
Comment 13 Daniel Holbert [:dholbert] 2010-01-21 15:00:36 PST
(In reply to comment #12)
> With the "Behavior 2" patches, we match Opera on the "xmlVsCssChart_v1.svg" 
> testcase, with the exception of the upper-left rect -- we render that one as
> gray, due to the second TODO/bug mentioned in my previous comment.
We also make the upper-right testcase (additive font-size) too big, due to the same bug.
Comment 14 Daniel Holbert [:dholbert] 2010-01-22 14:04:53 PST
After more consideration:
 - I believe the "Behavior 2" patches are the right way to go, as I stated in this post to www-svg:
http://lists.w3.org/Archives/Public/www-svg/2010Jan/0074.html

 - I actually don't think that the second TODO/bug in Comment 11 (& 12 & 13) is a bug after all.  If we have an additive animation on a CSS property, and its base value derives from an XML attribute, and that XML attribute itself is animated, then that makes the CSS animation's base value a "moving target", and that's OK. (IMHO this is comparable to animations which use "inherit" or "currentColor" as a "moving target" animation endpoint, when the properties that determine those values are being animated)  I posted about this to www-svg, too:
http://lists.w3.org/Archives/Public/www-svg/2010Jan/0075.html
Comment 15 Daniel Holbert [:dholbert] 2010-01-22 18:22:18 PST
NOTE: In order to avoid triggering CSS transitions from SMIL-animated mapped attributes (while still allowing CSS transitions from scripted changes to those attributes), we actually need to store the animated values in a **separate** style rule from mContentStyleRule.  (See bug 537139 for a related issue with SMIL animation of CSS properties -- as in that bug, we want to only want to walk the animated style-rule during *animation* restyles, and NOT during non-animation restyles.)

Perhaps this rule should be stored in nsINode::SetProperty() as well, so that we don't have to add a pointer to all |nsSVGElement| instances...
Comment 16 Daniel Holbert [:dholbert] 2010-01-25 18:48:50 PST
Created attachment 423470 [details] [diff] [review]
Patch A: Make GetAnimatedAttr()'s nsIAtom argument non-const

Obsoleting all old patches, as I've done a fair bit of refactoring since posting them. Per comment 14, I'm going with the "Behavior 2" strategy (the more intuitive behavior -- treating attributeType="CSS" and attributeType="XML" as distinct animation targets).

This first patch just removes the |const| from nsIContent::GetAnimatedAttr's nsIAtom parameter, since we'll need to convert it to a string (to see what property it matches), and nsIAtom::ToString is non-const.
Comment 17 Daniel Holbert [:dholbert] 2010-01-25 18:55:01 PST
Created attachment 423471 [details] [diff] [review]
Patch B: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule)

This patch takes the guts of nsSVGElement::UpdateContentStyleRule and refactors it out into a utility class, "MappedAttrParser".

My next patch will use this same utility class for parsing animated attribute-values (in UpdateAnimatedContentStyleRule).  The utility class is particularly useful in my next patch because it lets us lazy-initialize the nsICSSParser & the nsCSSDeclaration, when enumerating the animated attributes in the property table via a callback method.  This is messy to do without a helper class, and it's a win because it lets us avoid initializing a nsCSSDeclaration/nsICSSParser in the general case (when no mapped attributes are animated).
Comment 18 Daniel Holbert [:dholbert] 2010-01-25 19:05:04 PST
Created attachment 423472 [details] [diff] [review]
Patch C: Add nsSMILMappedAttribute & animated content style rule

This patch adds the "nsSMILMappedAttribute" class, which is just a specialization of nsSMILCSSProperty as described in comment 2.

This patch also adds a new nsICSSStyleRule to nsSVGElement, to behave like mContentStyleRule but for the animated values of mapped attributes. This rule has to be distinct from mContentStyleRule in order to keep SMIL from triggering CSS transitions, as described in comment 15.  To avoid increasing the size of nsSVGElement, I'm storing this new style rule in the element's Property Table, since it will be rarely needed.
Comment 19 Daniel Holbert [:dholbert] 2010-01-25 19:06:01 PST
Created attachment 423473 [details] [diff] [review]
Patch D: Tests
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-26 01:05:57 PST
+  // If we've parsed any values for mapped attributes, this method will create
+  // a nsICSSStyleRule for their parsed values.  Otherwise, it does nothing.
+  void CreateStyleRule(nsICSSStyleRule** aInstancePtrResult);

And leaves *aInstancePtrResult unchanged?

Maybe this should return already_AddRefed<nsICSStyleRule> and return null if there are no mapped attributes?
Comment 21 Daniel Holbert [:dholbert] 2010-01-26 03:14:27 PST
(In reply to comment #20)
> Maybe this should return already_AddRefed<nsICSStyleRule> and return null if
> there are no mapped attributes?

Yeah, I like that better -- fixed in patch queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bbfa9bad84a0
Comment 22 Daniel Holbert [:dholbert] 2010-01-26 03:16:47 PST
Created attachment 423505 [details] [diff] [review]
Patch B v2: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule)

Reposting patch B with that fixed.
Comment 23 Daniel Holbert [:dholbert] 2010-01-26 03:20:08 PST
Created attachment 423506 [details] [diff] [review]
Patch C v2: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc]

And here's Patch C, updated to call the new version of MappedAttrParser::CreateStyleRule.

Carrying forward sr=roc.
Comment 24 Daniel Holbert [:dholbert] 2010-01-26 12:11:48 PST
Comment on attachment 423506 [details] [diff] [review]
Patch C v2: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc]

Oops, I hadn't really meant to request r=dbaron on patch C.  Marking r=roc (per IRC discussion w/ him), to avoid needlessly lengthening review queues.
Comment 25 Daniel Holbert [:dholbert] 2010-01-26 18:37:34 PST
Created attachment 423723 [details] [diff] [review]
Patch B v3: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule) [sr=roc]

(Reposting patch B with two minor things changed -- I'm now preserving the comment that explains why we need "mParser->SetSVGMode(PR_TRUE);", and I added a missing space in a warning message).
Comment 26 Daniel Holbert [:dholbert] 2010-01-29 16:31:12 PST
Created attachment 424347 [details] [diff] [review]
Patch C v3: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc]

I've updated Patch C (nsSMILMappedAttribute implementation) to layer on top of  bug 542731's nsSMILCSS* cleanup patch ('Patch B' on that bug).

Basically:
 - In nsSMILMappedAttribute::ValueFromString, the nsSMILValue-initialization/destruction work now happens inside the nsSMILCSSValueType static helper method.
 - In nsSMILMappedAttribute::SetAnimValue, I've also restructured the logic to use an early return, to save on indentation and make things more straightforward.

Changes here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/diff/6f25b404235e/smil_mappedAttrs

Carrying forward r+sr=roc
Comment 27 Daniel Holbert [:dholbert] 2010-01-29 16:34:42 PST
Forgot to mention: the new Patch C also includes one more FlushChangesToTargetAttr() call -- without that call, some of this bug's tests start failing when I enable the optimization from bug 533291. 

Here's the change that added that call, in my patch queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/50bc0a4c6fe9
Comment 28 Daniel Holbert [:dholbert] 2010-02-24 11:27:48 PST
Created attachment 428768 [details] [diff] [review]
Patch C v3a: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc]

Just tweaking Patch C to match the updated checks for "is it safe to apply animated style" from bug 537139.  ('updated' = on par with 'fix v2' there)
Comment 29 Daniel Holbert [:dholbert] 2010-03-04 21:28:37 PST
I need to un-bitrot Patch B -- it touches chunks of nsSVGElement.cpp that changed 2 days ago on m-c (in changeset f6beeb315747, "Bug 523496: DeCOMtaminate nsCSSParser").
Comment 30 Daniel Holbert [:dholbert] 2010-03-05 12:58:47 PST
Created attachment 430710 [details] [diff] [review]
Patch B v4: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule) [sr=roc]

Here's an update to Patch B to fix bitrot noted in comment 29.  I've also removed some code that checked for allocation failure, to simplify MappedAttrParser, now that we've got infallible |new|.

Carrying forward sr=roc, & re-requesting r=dbaron.
Comment 31 Daniel Holbert [:dholbert] 2010-03-05 13:01:55 PST
Created attachment 430712 [details] [diff] [review]
Patch C v3b: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc]

This is just a minor bitrot fix to Patch C.  I also removed a comment about ignoring the return value of "mappedAttrParser->ParseMappedAttrValue()", since that method now returns void (instead of PRBool) in the new version of Patch B. (due to guarantees from infallible |new|).

Carrying forward r+sr=roc
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-03-12 15:04:13 PST
Comment on attachment 430710 [details] [diff] [review]
Patch B v4: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule) [sr=roc]

>+  nsRefPtr<nsICSSStyleRule> rule;

This should be nsCOMPtr<nsICSSStyleRule>, not nsRefPtr.

r=dbaron with that
Comment 33 Daniel Holbert [:dholbert] 2010-03-16 16:24:57 PDT
Landed, with comment 32 fixed, and with bitrot fixed for Bug 534136's new use of nsAtomString (and using nsAtomString):
 http://hg.mozilla.org/mozilla-central/rev/3b169d4e085c
 http://hg.mozilla.org/mozilla-central/rev/782e82ce2c29
 http://hg.mozilla.org/mozilla-central/rev/33632ecef69c
 http://hg.mozilla.org/mozilla-central/rev/cf62ca96cd0d

Also landed one more tests patch...
 http://hg.mozilla.org/mozilla-central/rev/9e96f8553258
...to split the smil-transitions-interaction-* tests into "a" and "b" versions - 'a' for CSS properties & 'b' for XML attributes.
Comment 34 Daniel Holbert [:dholbert] 2010-03-16 23:00:25 PDT
(In reply to comment #33)
> Also landed one more tests patch...
>  http://hg.mozilla.org/mozilla-central/rev/9e96f8553258
> ...to split the smil-transitions-interaction-* tests into "a" and "b" versions

FWIW, this final patch caused this scary-but-innocuous message to appear, when someone does "hg pull -u" to get this changeset:
>warning: detected divergent renames of layout/reftests/svg/smil/smil-transitions-interaction-1.svg to:
> layout/reftests/svg/smil/smil-transitions-interaction-1a.svg
> layout/reftests/svg/smil/smil-transitions-interaction-1b.svg
(same for -2, -3, and -4)

This patch was from running these commands and then hg qrefresh'ing:
> hg mv smil-transitions-interaction-1.svg  smil-transitions-interaction-1a.svg
> hg cp smil-transitions-interaction-1a.svg smil-transitions-interaction-1b.svg
> [ /me edits smil-transitions-interaction-1b.svg to make it different ]

I guess that operation confused mq or hg.  Anyway, I verified that the files are correct in m-c, so nothing's actually wrong.
Comment 35 Daniel Holbert [:dholbert] 2010-04-09 13:59:05 PDT
Adding "dev-doc-needed" keyword, at Blizzard's suggestion, since this will be in 1.9.3a4.

This doesn't need a lot of custom documentation -- mostly just a mention that it's possible to target these attributes (e.g. "font-size", "stroke") separately from the corresponding CSS properties, by adding attributeType="XML" to the <animate> element.

Seems like this would go on (or be linked off of) this work-in-progress page from bug 216462 comment 207:
  https://developer.mozilla.org/en/SVG_animation_%28SMIL%29_in_Firefox

Note You need to log in before you can comment on or make changes to this bug.