[Webkit-unassigned] [Bug 150388] SVG Animation (SMIL) on <text> or <tspan> doesn't work on second run

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=150388

--- Comment #16 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
When animating CSS attributes (aka XMLandCSSAnimation attributes) like the 'x' and 'y' attributes of the <rect> element, SVGAnimateElementBase::applyResultsToTarget() has to call applyCSSPropertyToTargetAndInstances() which will update the style of the SVGRectElement. The style is used later when calculating the layout rectangle of the SVGRectElement. The animated values are stored in another place separate form the SVGElement.

But when animating presentation attributes with SVG DOM (aka XMLAnimation attributes) like the 'x' and 'y' attributes of the <text> element, no style change is done. These attributes are represented as SVGLengthListValues and have to be updated and retrieved form the 'm_x' and 'm_y' members of the SVGTextElement itself.

When an SVGAnimationElement starts animating, it creates a new SVGAnimatedType for its member m_animatedType through the animator startAnimValAnimation() in SVGAnimateElementBase::resetAnimatedType(). For example, when animating the 'm_y' member of the SVGTextElement, SVGAnimatedLengthListAnimator::startAnimValAnimation() will be called which will call SVGAnimatedTypeAnimator::constructFromBaseValue(). This function creates a new SVGLengthListValues and then calls SVGAnimatedTypeAnimator::executeAction() with action = StartAnimationAction. And this will call SVGAnimatedListPropertyTearOff::animationStarted() which will set the SVGAnimatedListPropertyTearOff::m_values of the object 'm_y' of SVGTextElement to the same SVGLengthListValues that is stored in SVGAnimationElement::m_animatedType.

The problem happens when animating an XMLAnimation attribute with multiple SVGAnimationElements. SVGAnimatedListPropertyTearOff::animationStarted() will be called only once with the SVGLengthListValues of the m_animatedType of the first SVGAnimationElement. This means no matter how many SVGAnimationElements are acting on the same attribute, changing only one SVGAnimationElement::m_animatedProperty through SVGAnimationElement::updateAnimation() can affect the layout of the SVGElement. This depends on the order of processing the SVGAnimationElements by SMILTimeContainer::updateAnimations(). The first SVGAnimationElement will be considered the resultElement in SMILTimeContainer::updateAnimations(). So if the first SVGAnimationElement happens to be the first one that points of the same AnimatedType which the SVGElement property TearOff object points to, then the attribute will be animated. Otherwise nothing will happen.

I think the fix should be the following. The m_animatedProperty of all the SVGAnimationElements should point to the same SVGAnimatedType object which is also pointed to by the TearOff object of the attribute of the SVGElement. This can be done if SVGAnimatedTypeAnimator::constructFromBaseValue() creates a new AnimatedType from currentBaseValue() only if the TearOff object is not animating. Otherwise it reuses the currentBaseValue(). This means we need to change the return type of SVGAnimatedTypeAnimator::constructFromBaseValue() form std::unique_ptr<> to be RefPtr<>.

Because of using the templates by SVGAnimatedTypeAnimator::constructFromBaseValue() and because SVGAnimatedType uses DataUnion for its data and because of the TearOff classes of the scaler data types do not return ref counted objects, the solution can't be generalized and the fix may not be very straightforward.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170123/6a78b647/attachment-0001.html>