Bug 150388 – SVG Animation (SMIL) on <text> or <tspan> doesn't work on second run

• Component: SVG
• Status: RESOLVED FIXED

[Tobi Reif]

One SVG Animation (SMIL) doesn't work on second run.

Open http://tobireif.com/svg/bubbles.svg in Safari .
Click on "bubbles",
minimise (click on the "minus" button of the grey SVG box),
maximise (click on the the left button of the minimised grey box).
Now try to minimise again (as above) - the whole animation should happen as in the first run. Instead, the text "a bubble" doesn't move at all.

Safari 9.0 (11601.1.56).

Info: Works in Chrome.

[Radar WebKit Bug Importer]

<rdar://problem/23685782>

[Antoine Quint]

Works fine in Firefox as well.

[Antoine Quint]

Created attachment 266626 [details]
Reduction

I don't know if this is the same bug, but I've reduced the orignal sample at http://tobireif.com/svg/bubbles.svg to a much simpler SVG, attached to this bug.

This test case now has two animations on a <text> element, one that animates it down (id="down") and one that animates it back up (id="up"). Using script, we start the down animation, when that completes we start the up animation, and when that completes we start the down animation a second time.

The first two animations run, but the third doesn't appear to visually. It does however seem to run in the timeline as it fires an `endEvent` after a bit.

[Antoine Quint]

There seems to be something specific to <text> and the `y` attribute here. Animating the `y` attribute of a <rect> works fine, animating the `opacity` of a <text> works fine too.

[Antoine Quint]

The y attribute of a <text> is an SVGAnimatedLengthList while the y attribute of a <rect> is an SVGAnimatedLength, this is probably why the behaviour differs.

What might happen here is that the two <animate> elements are running, and for some reason it's picking the animated value from the wrong <animate> elements, in this case the "up" element when the "down" element is actively running while the "down" element is done.

[Antoine Quint]

Removing the "up" animation element during the final animation stretch applies the value correctly. Pretty sure this bug is about elements not picking the right animated value from its elements.

[Antoine Quint]

When the animations behave as expected, the `animatedLengthList` SVGLengthList animated in SVGAnimatedLengthListAnimator::calculateAnimatedValue() is the same I can access in SMILTimeContainer::updateAnimations() right before this chunk of code is called:

// This will calculate the contribution from the animation and add it to the resultsElement.
if (!animation->progress(elapsed, resultElement, seekToTime) && resultElement == animation)
    resultElement = nullptr;

with the following expression:

((SVGTextPositioningElement*)(animation->targetElement()))->y()

So at some point, a new SVGLengthList is created for the SVGAnimatedType* we pass to SVGAnimatedLengthListAnimator::calculateAnimatedValue() in SVGAnimateElementBase::calculateAnimatedValue() with the expression `resultAnimationElement.m_animatedType.get()`.

[Antoine Quint]

The new SVGLengthList is created as part of SVGAnimateElementBase::resetAnimatedType() due to this condition in SVGSMILElement::progress():

    // Only reset the animated type to the base value once for the lowest priority animation that animates and contributes to a particular element/attribute pair.
    if (this == resultElement && animationIsContributing)
        resetAnimatedType();

… specifically with this code:

std::unique_ptr<SVGAnimatedType> SVGAnimatedLengthListAnimator::startAnimValAnimation(const SVGElementAnimatedPropertyList& animatedTypes)
{
    return SVGAnimatedType::createLengthList(constructFromBaseValue<SVGAnimatedLengthList>(animatedTypes));
}

I can't put my finger on it quite yet, but something is wrong either in the condition under which resetAnimatedType() is called or within that method's implementation.

[Tobi Reif]

Thanks for investigating!

I hope the bug can be fixed soon :)

[Antoine Quint]

Haven't had much time to look into this recently but I will not forget about it.

[Tobi Reif]

OK :)

[Simon Fraser (smfr)]

One difference between <text> and <rect> is that <rect> uses ApplyXMLandCSSAnimation, but <text> uses ApplyXMLAnimation

[Simon Fraser (smfr)]

The "bad" value for the animating y() property is being fetched in SVGTextLayoutAttributesBuilder::fillCharacterDataMap()

[Tobi Reif]

Regarding the new/current bug report title:
The animation target is a tspan (not a <text/> element) - perhaps that's relevant?

Regarding the issue report itself:
The grey "closing cross" also doesn't get moved by Safari on second run.

[Said Abou-Hallawa]

Created attachment 299362 [details]
test case (line + text)

This test case shows the bug of animating the <text> and the <line> element in the second time. The problem is not reproducible for the <rect> element.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 299609 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 299609 [details]
Patch

This is not an ideal or a complete solution. But it fixes the bug and it is a good way to record my investigation.

[Simon Fraser (smfr)]

Comment on attachment 299609 [details]
Patch

I wonder if SVGAnimatedType should just be a WTF::Variant now.

[Said Abou-Hallawa]

Created attachment 300078 [details]
Patch

[Said Abou-Hallawa]

Created attachment 300084 [details]
Patch

[Said Abou-Hallawa]

Created attachment 300745 [details]
Patch

[Said Abou-Hallawa]

Created attachment 300746 [details]
test case (line + text + marker)

Adding a new test case which includes an std::pair SVGAnimatedType: the angleAndEnumeration. The red drawing should be hidden at the end of the animations.

[Simon Fraser (smfr)]

Comment on attachment 300745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300745&action=review

> Source/WebCore/svg/SVGAnimatedType.cpp:15
>   * Copyright (C) Research In Motion Limited 2011. All rights reserved.
> + * Copyright (C) 2017 Apple Inc.  All rights reserved.
>   *
> - * This library is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Library General Public
> - * License as published by the Free Software Foundation; either
> - * version 2 of the License, or (at your option) any later version.
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
>   *
> - * This library is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Library General Public License for more details.
> - *
> - * You should have received a copy of the GNU Library General Public License
> - * along with this library; see the file COPYING.LIB.  If not, write to
> - * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> - * Boston, MA 02110-1301, USA.
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE

You can't just change a license like this. You have to make it a dual-licensed file if you make substantive changes.

> Source/WebCore/svg/SVGAnimatedType.h:185
> +    Variant<
> +        std::pair<SVGAngleValue*, unsigned*>,
> +        bool*,
> +        Color*,
> +        unsigned*,
> +        int*,
> +        std::pair<int*, int*>,
> +        SVGLengthValue*,
> +        SVGLengthListValues*,
> +        float*,
> +        SVGNumberListValues*,
> +        std::pair<float*, float*>,
> +        SVGPathByteStream*,
> +        SVGPointListValues*,
> +        SVGPreserveAspectRatioValue*,
> +        FloatRect*,
> +        String*,
> +        SVGTransformListValues*

I'm really confused about why the Variant stores pointers to primitive types, rather than callers sharing a Variant*.

[Said Abou-Hallawa]

Comment on attachment 300745 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300745&action=review

>> Source/WebCore/svg/SVGAnimatedType.h:185
>> +        SVGTransformListValues*
> 
> I'm really confused about why the Variant stores pointers to primitive types, rather than callers sharing a Variant*.

I split changing SVGAnimatedType to hold a variant of primitive type in https://bugs.webkit.org/show_bug.cgi?id=168586.

[Tobi Reif]

I hope this bug can get fixed soon 😃

[Tobi Reif]

Is there any hope?

[Tobi Reif]

The current URL of the SVG animation demo is https://tobireif.com/demos/bubbles/ .

[Tobi Reif]

I hope this can still get fixed.

[Simon Fraser (smfr)]

Comment on attachment 300745 [details]
Patch

Said, can you break this patch up into the Variant changes, which should not affect behavior, and then a followup that fixes the bug.

[Tobi Reif]

It would be great if this issue could get fixed 😀

[Said Abou-Hallawa]

I verified that the patch in https://bugs.webkit.org/show_bug.cgi?id=191237 fixes this bug. I will not close this one as a dupe because I think we need to add a layout test for this bug.

[Tobi Reif]

> I verified that the patch in https://bugs.webkit.org/show_bug.cgi?id=191237
> fixes this bug.

So great to that there's progress regarding this bug! Thank you!

Please post a quick note here as soon as I can use Safari Tech Preview to verify the fix for the reported issue as observed at https://tobireif.com/demos/bubbles/ .

[Said Abou-Hallawa]

Created attachment 366517 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 366517 [details]
Patch

Clearing flags on attachment: 366517

Committed r243780: <https://trac.webkit.org/changeset/243780>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

When opening the demo page https://tobireif.com/demos/bubbles/, WebKit debug fired an assertion. This bug did exist before the re-factoring of https://bugs.webkit.org/show_bug.cgi?id=191237. I filed https://bugs.webkit.org/show_bug.cgi?id=196518 to track this issue.

[Tobi Reif]

Thanks so much to all of you!

I updated the info at https://tobireif.com/demos/bubbles/ :
"Doesn't work correctly in Safari 12.1 and lower versions, but should work correctly in higher versions (starting at some version). Here's the WebKit bug report [ https://bugs.webkit.org/show_bug.cgi?id=150388 ]."

I hope I can soon use Safari Tech Preview to verify the fix, and I hope the fix will soon appear in Safari stable.

Thanks again!

[Tobi Reif]

It works in the latest Safari Tech Preview - thanks!

[Tobi Reif]

This now works in the latest stable Safari (version 13), so I removed the text regarding this bug from the demo page:

https://tobireif.com/demos/bubbles/
  -> "show demo info"

Thanks again!