Bug 150388 - SVG Animation (SMIL) on <text> or <tspan> doesn't work on second run
Summary: SVG Animation (SMIL) on <text> or <tspan> doesn't work on second run
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on: 191237
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-21 02:43 PDT by Tobi Reif
Modified: 2019-09-25 01:19 PDT (History)
10 users (show)

See Also:


Attachments
Reduction (1.19 KB, image/svg+xml)
2015-12-04 06:42 PST, Antoine Quint
no flags Details
test case (line + text) (1.93 KB, image/svg+xml)
2017-01-20 11:58 PST, Said Abou-Hallawa
no flags Details
Patch (52.52 KB, patch)
2017-01-24 10:15 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (88.56 KB, patch)
2017-01-29 17:31 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (92.05 KB, patch)
2017-01-29 19:59 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (90.25 KB, patch)
2017-02-06 11:51 PST, Said Abou-Hallawa
simon.fraser: review-
Details | Formatted Diff | Diff
test case (line + text + marker) (2.72 KB, image/svg+xml)
2017-02-06 11:55 PST, Said Abou-Hallawa
no flags Details
Patch (4.34 KB, patch)
2019-04-02 12:02 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobi Reif 2015-10-21 02:43:47 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-11-30 02:00:21 PST
<rdar://problem/23685782>
Comment 2 Antoine Quint 2015-11-30 02:03:57 PST
Works fine in Firefox as well.
Comment 3 Antoine Quint 2015-12-04 06:42:09 PST
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.
Comment 4 Antoine Quint 2015-12-04 06:43:10 PST
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.
Comment 5 Antoine Quint 2015-12-04 07:41:40 PST
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.
Comment 6 Antoine Quint 2015-12-04 08:12:26 PST
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.
Comment 7 Antoine Quint 2015-12-15 03:34:29 PST
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()`.
Comment 8 Antoine Quint 2015-12-15 06:43:57 PST
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.
Comment 9 Tobi Reif 2016-01-15 01:24:37 PST
Thanks for investigating!

I hope the bug can be fixed soon :)
Comment 10 Antoine Quint 2016-01-18 05:45:20 PST
Haven't had much time to look into this recently but I will not forget about it.
Comment 11 Tobi Reif 2016-01-18 12:39:21 PST
OK :)
Comment 12 Simon Fraser (smfr) 2017-01-19 10:38:35 PST
One difference between <text> and <rect> is that <rect> uses ApplyXMLandCSSAnimation, but <text> uses ApplyXMLAnimation
Comment 13 Simon Fraser (smfr) 2017-01-19 10:58:24 PST
The "bad" value for the animating y() property is being fetched in SVGTextLayoutAttributesBuilder::fillCharacterDataMap()
Comment 14 Tobi Reif 2017-01-20 02:06:31 PST
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.
Comment 15 Said Abou-Hallawa 2017-01-20 11:58:30 PST
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.
Comment 16 Said Abou-Hallawa 2017-01-23 10:55:08 PST
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.
Comment 17 Said Abou-Hallawa 2017-01-24 10:15:03 PST
Created attachment 299609 [details]
Patch
Comment 18 Said Abou-Hallawa 2017-01-24 10:16:29 PST
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.
Comment 19 Simon Fraser (smfr) 2017-01-24 11:24:22 PST
Comment on attachment 299609 [details]
Patch

I wonder if SVGAnimatedType should just be a WTF::Variant now.
Comment 20 Said Abou-Hallawa 2017-01-29 17:31:42 PST
Created attachment 300078 [details]
Patch
Comment 21 Said Abou-Hallawa 2017-01-29 19:59:40 PST
Created attachment 300084 [details]
Patch
Comment 22 Said Abou-Hallawa 2017-02-06 11:51:39 PST
Created attachment 300745 [details]
Patch
Comment 23 Said Abou-Hallawa 2017-02-06 11:55:52 PST
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.
Comment 24 Simon Fraser (smfr) 2017-02-06 17:02:45 PST
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*.
Comment 25 Said Abou-Hallawa 2017-02-19 23:36:01 PST
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.
Comment 26 Tobi Reif 2017-04-21 03:13:09 PDT
I hope this bug can get fixed soon 😃
Comment 27 Tobi Reif 2017-10-17 04:14:25 PDT
Is there any hope?
Comment 28 Tobi Reif 2017-10-17 04:16:13 PDT
The current URL of the SVG animation demo is https://tobireif.com/demos/bubbles/ .
Comment 29 Tobi Reif 2018-01-06 00:39:03 PST
I hope this can still get fixed.
Comment 30 Simon Fraser (smfr) 2018-01-24 13:29:04 PST
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.
Comment 31 Tobi Reif 2018-04-12 10:15:26 PDT
It would be great if this issue could get fixed 😀
Comment 32 Said Abou-Hallawa 2019-03-04 18:44:12 PST
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.
Comment 33 Tobi Reif 2019-03-05 01:26:52 PST
> 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/ .
Comment 34 Said Abou-Hallawa 2019-04-02 12:02:50 PDT
Created attachment 366517 [details]
Patch
Comment 35 WebKit Commit Bot 2019-04-02 17:26:09 PDT
Comment on attachment 366517 [details]
Patch

Clearing flags on attachment: 366517

Committed r243780: <https://trac.webkit.org/changeset/243780>
Comment 36 WebKit Commit Bot 2019-04-02 17:26:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Said Abou-Hallawa 2019-04-02 17:30:16 PDT
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.
Comment 38 Tobi Reif 2019-04-03 02:24:55 PDT
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!
Comment 39 Tobi Reif 2019-04-04 01:57:46 PDT
It works in the latest Safari Tech Preview - thanks!
Comment 40 Tobi Reif 2019-09-25 01:19:08 PDT
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!