Bug 38836 – SMIL Animation does not starts

• Component: SVG
• Status: RESOLVED FIXED

[marek]

Created attachment 55529 [details]
Two files inside, one working, one not, diff is pretty easy to see...

If there is SMIL animation inside of mask element, it does not not starts at all.

http://svg.kvalitne.cz/mix/spirit2.svg

But by simple adding one more animate element to the document, see

http://svg.kvalitne.cz/mix/spirit.svg

it is working as expected. I consider this to be a bug, since it is working properly in other browsers.
The only difference is presence of one more animate element...

[Alexey Proskuryakov]

Confirmed with r58908.

[Robin Cao]

The root cause of this bug is: the masker is not updated during animation.
This problem can only reproduce with 'animateTransform' and 'animateMotion'. Other animation elements use a different mechanism: 
Everything is routed through 'setAttribute' calls, then the masker gets invalidated in attribute changed callback SVGStyledElement::svgAttributeChanged'.

I have a quick fix for this bug.
Add the following code to the end of SVGAnimateTransformElement::applyResultsToTarget()

    if (targetElement->isStyled())
        static_cast<SVGStyledElement*>(targetElement)->invalidateResourcesInAncestorChain();

It works, but i'm not sure if this is the correct solution.

Dirk, do you have any suggestions?

[Dirk Schulze]

(In reply to comment #2)
> The root cause of this bug is: the masker is not updated during animation.
> This problem can only reproduce with 'animateTransform' and 'animateMotion'. Other animation elements use a different mechanism: 
> Everything is routed through 'setAttribute' calls, then the masker gets invalidated in attribute changed callback SVGStyledElement::svgAttributeChanged'.
> 
> I have a quick fix for this bug.
> Add the following code to the end of SVGAnimateTransformElement::applyResultsToTarget()
> 
>     if (targetElement->isStyled())
>         static_cast<SVGStyledElement*>(targetElement)->invalidateResourcesInAncestorChain();
> 
> It works, but i'm not sure if this is the correct solution.
> 
> Dirk, do you have any suggestions?

Well, isn't it better to do it like other elements (SVGRectElement, SVGGElement, ...)? It would look like this:

     RenderObject* renderer = targetElement->renderer();
     if (targetElement->isStyled()) {
        renderer->setNeedsTransformUpdate();
        renderer->setNeedsLayout(true);
    } else
        renderer->setNeedsLayout(true);

This is untested, but telling the renderer to just update the transform is maybe faster. And it's my opinion, that the code shouldn't handle resources manually here.

Niko may know it better, since he changed layouting lately.

[Dirk Schulze]

(In reply to comment #3)
> (In reply to comment #2)
> > The root cause of this bug is: the masker is not updated during animation.
> > This problem can only reproduce with 'animateTransform' and 'animateMotion'. Other animation elements use a different mechanism: 
> > Everything is routed through 'setAttribute' calls, then the masker gets invalidated in attribute changed callback SVGStyledElement::svgAttributeChanged'.
> > 
> > I have a quick fix for this bug.
> > Add the following code to the end of SVGAnimateTransformElement::applyResultsToTarget()
> > 
> >     if (targetElement->isStyled())
> >         static_cast<SVGStyledElement*>(targetElement)->invalidateResourcesInAncestorChain();
> > 
> > It works, but i'm not sure if this is the correct solution.
> > 
> > Dirk, do you have any suggestions?
> 
> Well, isn't it better to do it like other elements (SVGRectElement, SVGGElement, ...)? It would look like this:
> 
>      RenderObject* renderer = targetElement->renderer();
>      if (targetElement->isStyled()) {
>         renderer->setNeedsTransformUpdate();
>         renderer->setNeedsLayout(true);
>     } else
>         renderer->setNeedsLayout(true);
> 
> This is untested, but telling the renderer to just update the transform is maybe faster. And it's my opinion, that the code shouldn't handle resources manually here.
> 
> Niko may know it better, since he changed layouting lately.

Sorry, looked at SVGAnimateTransformElement again. We're already calling relayouting. It looks like you're right with invalidateResourcesInAncestorChain(), but you also need to call registerFromResources(renderer), see SVGStyledElement::svgAttributeChanged.
I still believe that this should be done in layout() on the renderer, but it's ok to handle it here for the moment.

[Philip Rogers]

This has been fixed. Possibly with the recent SMIL changes.