Open
Bug 1302948
(stylo-smil)
Opened 7 years ago
Updated 1 year ago
[meta] Stylo: Get SVG (SMIL) animations to work
Categories
(Core :: CSS Parsing and Computation, defect, P5)
Core
CSS Parsing and Computation
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox51 | --- | affected |
People
(Reporter: birtles, Unassigned)
References
(Depends on 1 open bug)
Details
(Keywords: meta)
Originally I thought we could do this by rebasing our SMIL implementation on top of Web Animations but after further thought I don't think we can. In order to do that we'd need to: * Implement CSS Motion spec * Promote many SVG attributes to properties (e.g. 'width', 'd' etc.) * Make 'display' property animatable * Finish implementing additive animation * Add extra interfaces like a special SVG timeline that includes playback control So, instead I think we're going to have to add the corresponding hooks into Servo's style system that we have for Gecko although we can probably make some simplifications along the way (like bug 1062106). In fact we should probably fix that bug first.
Comment 2•7 years ago
|
||
Manish's comment from bug 1342557: Bug 1329093 adds support for generation of declaration blocks from SVG pres attrs. There's a similar mechanism for SMIL-animated attrs but I haven't added support for that (should be easy to do, I just don't want to write untested code). We need to figure out a plan for SMIL and make these changes as a part of it.
Comment 3•7 years ago
|
||
SMIL also needs addition, accumulation and computing distance.
Comment 4•7 years ago
|
||
Putting Hiro on point to figure out the pieces here. Marking as P2 because IIRC Brian said we might not get to this before Taipei, but feel free to bump to P1 if we will.
Assignee: nobody → hikezoe
Priority: -- → P2
Reporter | ||
Comment 5•7 years ago
|
||
I've been working on this on and off for the last couple of days and I'm optimistic we can get rid of the animated mapped-attribute case altogether. That would mean the main point of integration we need is the SMIL override stylesheet. We currently store the declarations for that in DOM slots (under mSMILOverrideStyleDeclaration).
No longer depends on: 1329093
Comment 6•7 years ago
|
||
That's great news!
Reporter | ||
Comment 7•7 years ago
|
||
I had a look into what other parts of SMIL interface with the style system and came up with the notes below. This is assuming bug 1062106 gets rid of the mapped attribute handling (and I have a try run that indicates that should work). Summary: * We should try to rework the base value handling in SMIL to use the system we use in dom/animation. That will likely happen in bug 1315874. This is probably the hardest and highest-risk part so we should do this first. * We need to do distance and additive animation (bugs are already filed and marked as blocking this). This should be fairly straightforward and is fairly self-contained (so, if, for example, Manish was bored one day...
Comment 8•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #7) > I had a look into what other parts of SMIL interface with the style system > and came up with the notes below. This is assuming bug 1062106 gets rid of > the mapped attribute handling (and I have a try run that indicates that > should work). > > Summary: > > * We should try to rework the base value handling in SMIL to use the system > we use in dom/animation. > That will likely happen in bug 1315874. This is probably the hardest and > highest-risk part so we should > do this first. > * We need to do distance and additive animation (bugs are already filed and > marked as blocking this). > This should be fairly straightforward and is fairly self-contained (so, > if, for example, Manish was > bored one day... This sounds good. Let's get the work in bug 1315874 done and then find somebody a bit later to do the self-contained servo-side work. Manish's plate is pretty full right now with higher-risk less-self-contained work.
Reporter | ||
Comment 9•7 years ago
|
||
Oh, I just noticed now that my comment from yesterday got chopped off leaving only a fraction of what I wrote :( It seems like it got chopped off write where there was an emoji. Perhaps Bugzilla can't to emoji? I managed to save the second half anyway so here it is: nsSMILCSSValueType.cpp: * ValueWrapper is the type we store on the nsSMILValue that includes the StyleAnimationValue and property ID. It would be trivial to extend this to store a RefPtr<RawServoAnimationValue>, or even just an AnimationValue instead. * There's some interesting handling in GetZeroValueForUnit / FinalizeStyleAnimationValues that I originally thought was used for neutral values and addition (e.g. to-animation). However, I think it's actually there to handle by-animation. In nsSMILAnimationFunction::GetValues we appear to fill-in an empty value with the appropriate type in that case. There may be other cases too. I suspect we might just need to add this concept to Stylo's code -- i.e. a zero-value concept. (And this probably argues against the change we just made in bug 1339332 -- i.e. we should probably have made 'neutral value' with 'replace' mean zero value, and 'neutral value' with 'add' mean underlying value. :/) * We have some special handling for negative values because the CSS parser doesn't accept them but we don't want to clamp the output too early. See bug 501188. Given that the Servo values are pretty opaque on the Gecko side, perhaps we should fix bug 501188 for Servo? i.e. add a parsing mode that allows negative values? Or simply a wrapper function on the Servo side that does the same swapping (i.e. drops the '-', parses the result, makes it negative, then returns it to Gecko). * For nsSMILCSSValueType::Add we obviously need to implement additive animation. (Bug 1329878) * Likewise, for nsSMILCSSValueType::ComputeDistance we need to calculate the distance. (Bug 1332633) * Interpolation, we should already have, and hopefully all properties supported by SMIL are covered. We'll need to make sure we can override the interpolation for 'visibility', however, since it interpolates differently in SMIL to CSS. * ValueFromStringHelper does the special negative handling, as well as dividing out the text zoom. I suspect we'll need to do that on the Servo side too, so we'll probably just need a Servo function for creating suitable AnimationValue objects for SMIL. * nsSMILCSSValueType::ValueToString relies on StyleAnimationValue::UncomputeValue but I believe we already have Servo_AnimationValue_Serialize for this. nsSMILCSSProperty.cpp * This contains some pretty funky base value handling. It's also buggy since it can trigger transitions as we discovered in bug 1315874. Ideally we should move this over to using something like the base-value system we use for Web Animations. That would hopefully fix bug 1315874 at the same time. This is probably the biggest change to make to SMIL and the one that would reduce risk the most so we should probably do this next. * We currently store the SMIL animated style as a CSS declaration in DOM slots on the element. Note that we have three methods: Element::GetSMILOverrideStyle() - Returns an nsICSSDeclaration*, creating DOM slots if necessary. Element::GetSMILOverrideStyleDeclaration() - Returns a DeclarationBlock* if one exists. Element::SetSMILOverrideStyleDeclaration - Creates DOM slots if necessary, set the DeclarationBlock, and posts a suitable animation restyle (if |aNotify| is true). The setup seems to be that GetSMILOverrideStyle returns an nsDOMCSSAttributeDeclaration object with the special mIsSMILOverride flag set. In nsDOMCSSAttributeDeclaration::GetCSSDeclaration if mIsSMILOverride is true, we end up getting/settings the DeclarationBlock from Get/SetSMILOverrideStyleDeclaration. In nsSMILCSSProperty::GetBaseValue(), nsSMILCSSProperty::SetAnimValue, nsSMILCSSProperty::ClearAnimValue etc. we call GetSMILOverrideStyle and operate on the returned nsICSSDeclaration, getting and setting properties and thereby updating the corresponding DeclarationBlock. It's that DeclarationBlock that gets used when we resolve style, e.g. from nsHTMLCSSStyleSheet::ElementRulesMatching. Now DeclarationBlock is already able to store a ServoDeclarationBlock and we interact with it by setting strings so maybe this part already works? (Note: In nsSMILCSSProperty::IsPropertyAnimatable we could probably make writing-mode animatable now that we implement it.) nsSMILCompositor.cpp * The only piece that stands out here is the base value handling. nsSMILAnimationFunction.cpp * For nsSMILAnimationFunction::ComposeResult we need to support additive animation. Note that we don't need to support StyleAnimationValue::Accumulate. SMIL uses the same operation for both. The only exceptions are transforms and motion-on-a-path which don't use StyleAnimationValue but rather SVGTransformListSMILType (which only does a single transform function) and SVGMotionSMILType. * Again, there is some base value handling here that we will hopefully shift to using the same mechanism as in dom/animation. nsSMILAnimationController: * We'll probably need to add the appropriate restyle hint (eRestyle_StyleAttribute_Animations) posted in nsSMILAnimationController::AddStyleUpdatesTo as part of doing an animation-only restyle for CSS transitions.
Comment 10•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #7) > * We need to do distance and additive animation (bugs are already filed and > marked as blocking this). > This should be fairly straightforward and is fairly self-contained (so, > if, for example, Manish was > bored one day... Actually, my other bugs are waiting for animation-only restyle now, so I think I could implement ComputeDistance (and Additive animation if possible) for SMIL on servo side now. :)
Reporter | ||
Comment 11•7 years ago
|
||
Rewriting the summary of what I think we need to do: * We should try to rework the base value handling in SMIL to use the system we use in dom/animation. That will likely happen in bug 1315874. This is probably the hardest and highest-risk part so we should do this first. * We need to do distance and additive animation (bugs are already filed and marked as blocking this). This should be fairly straightforward and is fairly self-contained. * We need to add a SMIL override cascade level. I wonder if we can mimic the style attribute setup for this. The declarations for this are stored in a DeclarationBlock in DOM slots so hopefully this is not too hard to plug in. (Long term this should just get merged with other animation styles but I think it's too much to try to attempt that now.) * There are a few utility methods we'll possibly need on the Servo side, e.g. * A method to parse CSS property value allowing certain out-of-range values that would normally produce parse errors (e.g. an opacity of -1) * A method that takes a property name and returns a suitable zero value for by-animation (e.g. pass it 'margin-left' it gives you a '0px' computed value). * Perhaps a parameter to Servo's interpolation routine to turn off special visibility interpolation and make it behave like any other discrete property. * Various bits of plumbing (e.g. adapting ValueWrapper). This looks to be fairly straightforward. (I'm sure a wrote a much better summary yesterday. Hopefully I didn't miss anything this second time around.)
Reporter | ||
Comment 12•7 years ago
|
||
Turning this into a meta bug so we can hang some other bugs off it.
Updated•7 years ago
|
Priority: -- → P5
Reporter | ||
Updated•7 years ago
|
Priority: P5 → P1
Reporter | ||
Comment 13•7 years ago
|
||
Oops, sorry, I thought this was the actual bug, not the meta bug.
Priority: P1 → P5
Reporter | ||
Updated•6 years ago
|
Alias: stylo-smil
Updated•1 year ago
|
Severity: normal → S3
Updated•1 year ago
|
Summary: Stylo: Get SVG (SMIL) animations to work → [meta] Stylo: Get SVG (SMIL) animations to work
You need to log in
before you can comment on or make changes to this bug.
Description
•