Open Bug 1302948 (stylo-smil) Opened 8 years ago Updated 2 years ago

[meta] Stylo: Get SVG (SMIL) animations to work

Categories

(Core :: CSS Parsing and Computation, defect, P5)

defect

Tracking

()

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.
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.
Blocks: stylo
Depends on: 1329093
SMIL also needs addition, accumulation and computing distance.
Depends on: 1329878, 1332633
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
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
That's great news!
Depends on: 1346663
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...
(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.
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.
(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. :)
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.)
Depends on: 1315874
Blocks: stylo-svg
Depends on: 1353202
Turning this into a meta bug so we can hang some other bugs off it.
Assignee: hikezoe → nobody
Keywords: meta
Priority: P2 → --
Depends on: 1355348
Depends on: 1355349
Priority: -- → P5
Depends on: 1357295
Depends on: 1357296
Priority: P5 → P1
Oops, sorry, I thought this was the actual bug, not the meta bug.
Priority: P1 → P5
Depends on: 1358955
Depends on: 1358966
Depends on: 1363574
Depends on: 1363880
Depends on: 1364279
Depends on: 1365472
Depends on: 1365855
No longer blocks: 1363045
Depends on: 1363045
Depends on: 1367293
Depends on: 1369588
Depends on: 1369614
Depends on: 1369277
Depends on: 1360133
Depends on: 1369624
Depends on: 1369625
Depends on: 1371150
Depends on: 1371174
Depends on: 1371196
Depends on: 1371199
Depends on: 1371480
Alias: stylo-smil
Depends on: 1371518
No longer blocks: stylo
Depends on: 1374161
Depends on: 1390352
Depends on: 1390357
Depends on: 1390364
Depends on: 1390367
Depends on: 1390384
Depends on: 1396483
Depends on: 1399799
No longer depends on: 1364279
Severity: normal → S3
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.