4.92 KB, patch
|Details | Diff | Splinter Review|
30.65 KB, patch
|Details | Diff | Splinter Review|
7.17 KB, patch
|Details | Diff | Splinter Review|
19.79 KB, patch
|Details | Diff | Splinter Review|
Filing this bug on supporting SMIL animation of the SVG attributes that are mapped to CSS properties.
The method I showed you earlier is nsSVGElement::UpdateContentStyleRule() http://hg.mozilla.org/mozilla-central/annotate/ed6b195bae6d/content/svg/content/src/nsSVGElement.cpp#l1001
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.
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?
(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 , and then (with that category) I could use the attribute-name as the property key and the animated value as the attribute-value.  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsPropertyTable.h#99
er, s/attribute-value/property value/.
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.
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)
Created attachment 422839 [details] [diff] [review] (Behavior 2) Patch B: Make GetAnimatedAttr() non-const Here's a helper patch needed for "Behavior 2"...
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.
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.
(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.
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
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...
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 #423470 - Flags: review?(roc)
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).
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 #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+
(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
Created attachment 423505 [details] [diff] [review] Patch B v2: Add "MappedAttrParser" helper class (& use it in UpdateContentStyleRule) Reposting patch B with that fixed.
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)
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 #423505 - Flags: superreview?(roc) → superreview+
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).
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
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
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)
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").
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.
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+
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: 9 years ago
Resolution: --- → FIXED
(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.
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
Target Milestone: --- → mozilla1.9.3a4
You need to log in before you can comment on or make changes to this bug.