The default bug view has changed. See this FAQ.

SVG SMIL: Add support for animating mapped attributes

RESOLVED FIXED in mozilla1.9.3a4

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9.3a4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 11 obsolete attachments)

4.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
30.65 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.17 KB, patch
dbaron
: review+
dholbert
: superreview+
Details | Diff | Splinter Review
19.79 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
Filing this bug on supporting SMIL animation of the SVG attributes that are mapped to CSS properties.
(Assignee)

Updated

7 years ago
OS: Linux → All
Hardware: x86 → All
The method I showed you earlier is nsSVGElement::UpdateContentStyleRule()

http://hg.mozilla.org/mozilla-central/annotate/ed6b195bae6d/content/svg/content/src/nsSVGElement.cpp#l1001
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
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?
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 5

7 years ago
(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
(Assignee)

Comment 6

7 years ago
er, s/attribute-value/property value/.
(Assignee)

Comment 7

7 years ago
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.
(Assignee)

Comment 8

7 years ago
Created attachment 422835 [details] [diff] [review]
Patch A: Mochitests
(Assignee)

Comment 9

7 years ago
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)
(Assignee)

Comment 10

7 years ago
Created attachment 422839 [details] [diff] [review]
(Behavior 2) Patch B: Make GetAnimatedAttr() non-const

Here's a helper patch needed for "Behavior 2"...
(Assignee)

Comment 11

7 years ago
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.
(Assignee)

Updated

7 years ago
Attachment #422842 - Attachment is patch: true
Attachment #422842 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
(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.
(Assignee)

Comment 14

7 years ago
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
(Assignee)

Comment 15

7 years ago
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...
(Assignee)

Comment 16

7 years ago
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.
Attachment #422835 - Attachment is obsolete: true
Attachment #422837 - Attachment is obsolete: true
Attachment #422839 - Attachment is obsolete: true
Attachment #422842 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #423470 - Flags: review?(roc)
(Assignee)

Comment 17

7 years ago
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).
(Assignee)

Updated

7 years ago
Attachment #423471 - Flags: superreview?(roc)
Attachment #423471 - Flags: review?(dbaron)
(Assignee)

Comment 18

7 years ago
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.
Attachment #423472 - Flags: superreview?(roc)
Attachment #423472 - Flags: review?(dbaron)
(Assignee)

Comment 19

7 years ago
Created attachment 423473 [details] [diff] [review]
Patch D: Tests
(Assignee)

Updated

7 years ago
Attachment #423473 - Flags: review?(roc)
Attachment #423470 - Flags: review?(roc) → review+
+  // 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?
Attachment #423472 - Flags: superreview?(roc) → superreview+
Attachment #423473 - Flags: review?(roc) → review+
(Assignee)

Comment 21

7 years ago
(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
(Assignee)

Comment 22

7 years ago
Created attachment 423505 [details] [diff] [review]
Patch B v2: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule)

Reposting patch B with that fixed.
Attachment #423505 - Flags: superreview?(roc)
Attachment #423505 - Flags: review?(dbaron)
(Assignee)

Comment 23

7 years ago
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.
Attachment #423471 - Attachment is obsolete: true
Attachment #423472 - Attachment is obsolete: true
Attachment #423506 - Flags: superreview+
Attachment #423506 - Flags: review?(dbaron)
Attachment #423472 - Flags: review?(dbaron)
Attachment #423471 - Flags: superreview?(roc)
Attachment #423471 - Flags: review?(dbaron)
(Assignee)

Comment 24

7 years ago
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.
Attachment #423506 - Attachment description: Patch C v2: Add nsSMILMappedAttribute & animated content style rule. [sr=roc] → Patch C v2: Add nsSMILMappedAttribute & animated content style rule. [r=roc sr=roc]
Attachment #423506 - Flags: review?(dbaron) → review+
Attachment #423505 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 25

7 years ago
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).
Attachment #423505 - Attachment is obsolete: true
Attachment #423723 - Flags: superreview+
Attachment #423723 - Flags: review?(dbaron)
Attachment #423505 - Flags: review?(dbaron)
(Assignee)

Comment 26

7 years ago
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
Attachment #423506 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #424347 - Flags: superreview+
Attachment #424347 - Flags: review+
(Assignee)

Comment 27

7 years ago
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
(Assignee)

Updated

7 years ago
Whiteboard: [needs review dbaron]
(Assignee)

Comment 28

7 years ago
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)
Attachment #424347 - Attachment is obsolete: true
Attachment #428768 - Flags: superreview+
Attachment #428768 - Flags: review+
(Assignee)

Comment 29

7 years ago
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").
(Assignee)

Comment 30

7 years ago
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.
Attachment #423723 - Attachment is obsolete: true
Attachment #430710 - Flags: superreview+
Attachment #430710 - Flags: review?(dbaron)
Attachment #423723 - Flags: review?(dbaron)
(Assignee)

Comment 31

7 years ago
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
Attachment #428768 - Attachment is obsolete: true
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
Attachment #430710 - Flags: review?(dbaron) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [needs review dbaron]
(Assignee)

Comment 33

7 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 34

7 years ago
(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.
(Assignee)

Updated

7 years ago
Depends on: 552873
(Assignee)

Comment 35

7 years ago
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
Keywords: dev-doc-needed
Target Milestone: --- → mozilla1.9.3a4
(Assignee)

Updated

7 years ago
Depends on: 566777
Keywords: dev-doc-needed
(Assignee)

Updated

4 years ago
Blocks: 904158
Blocks: 1062106
You need to log in before you can comment on or make changes to this bug.