1302948 - (stylo-smil) [meta] Stylo: Get SVG (SMIL) animations to work

Status: NEW

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

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

[Hiroyuki Ikezoe (:hiro)]

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

[Hiroyuki Ikezoe (:hiro)]

SMIL also needs addition, accumulation and computing distance.

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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).

Comment 6

[Bobby Holley (:bholley)]

That's great news!

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

[Bobby Holley (:bholley)]

(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.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

[Boris Chiou [:boris]]

(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. :)

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.)

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Turning this into a meta bug so we can hang some other bugs off it.

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Oops, sorry, I thought this was the actual bug, not the meta bug.