RESOLVED FIXED 150388
SVG Animation (SMIL) on <text> or <tspan> doesn't work on second run
https://bugs.webkit.org/show_bug.cgi?id=150388
Summary SVG Animation (SMIL) on <text> or <tspan> doesn't work on second run
Tobi Reif
Reported 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.
Attachments
Reduction (1.19 KB, image/svg+xml)
2015-12-04 06:42 PST, Antoine Quint
no flags
test case (line + text) (1.93 KB, image/svg+xml)
2017-01-20 11:58 PST, Said Abou-Hallawa
no flags
Patch (52.52 KB, patch)
2017-01-24 10:15 PST, Said Abou-Hallawa
no flags
Patch (88.56 KB, patch)
2017-01-29 17:31 PST, Said Abou-Hallawa
no flags
Patch (92.05 KB, patch)
2017-01-29 19:59 PST, Said Abou-Hallawa
no flags
Patch (90.25 KB, patch)
2017-02-06 11:51 PST, Said Abou-Hallawa
simon.fraser: review-
test case (line + text + marker) (2.72 KB, image/svg+xml)
2017-02-06 11:55 PST, Said Abou-Hallawa
no flags
Patch (4.34 KB, patch)
2019-04-02 12:02 PDT, Said Abou-Hallawa
no flags
Radar WebKit Bug Importer
Comment 1 2015-11-30 02:00:21 PST
Antoine Quint
Comment 2 2015-11-30 02:03:57 PST
Works fine in Firefox as well.
Antoine Quint
Comment 3 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.
Antoine Quint
Comment 4 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.
Antoine Quint
Comment 5 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.
Antoine Quint
Comment 6 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.
Antoine Quint
Comment 7 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()`.
Antoine Quint
Comment 8 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.
Tobi Reif
Comment 9 2016-01-15 01:24:37 PST
Thanks for investigating! I hope the bug can be fixed soon :)
Antoine Quint
Comment 10 2016-01-18 05:45:20 PST
Haven't had much time to look into this recently but I will not forget about it.
Tobi Reif
Comment 11 2016-01-18 12:39:21 PST
OK :)
Simon Fraser (smfr)
Comment 12 2017-01-19 10:38:35 PST
One difference between <text> and <rect> is that <rect> uses ApplyXMLandCSSAnimation, but <text> uses ApplyXMLAnimation
Simon Fraser (smfr)
Comment 13 2017-01-19 10:58:24 PST
The "bad" value for the animating y() property is being fetched in SVGTextLayoutAttributesBuilder::fillCharacterDataMap()
Tobi Reif
Comment 14 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.
Said Abou-Hallawa
Comment 15 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.
Said Abou-Hallawa
Comment 16 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.
Said Abou-Hallawa
Comment 17 2017-01-24 10:15:03 PST
Said Abou-Hallawa
Comment 18 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.
Simon Fraser (smfr)
Comment 19 2017-01-24 11:24:22 PST
Comment on attachment 299609 [details] Patch I wonder if SVGAnimatedType should just be a WTF::Variant now.
Said Abou-Hallawa
Comment 20 2017-01-29 17:31:42 PST
Said Abou-Hallawa
Comment 21 2017-01-29 19:59:40 PST
Said Abou-Hallawa
Comment 22 2017-02-06 11:51:39 PST
Said Abou-Hallawa
Comment 23 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.
Simon Fraser (smfr)
Comment 24 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*.
Said Abou-Hallawa
Comment 25 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.
Tobi Reif
Comment 26 2017-04-21 03:13:09 PDT
I hope this bug can get fixed soon 😃
Tobi Reif
Comment 27 2017-10-17 04:14:25 PDT
Is there any hope?
Tobi Reif
Comment 28 2017-10-17 04:16:13 PDT
The current URL of the SVG animation demo is https://tobireif.com/demos/bubbles/ .
Tobi Reif
Comment 29 2018-01-06 00:39:03 PST
I hope this can still get fixed.
Simon Fraser (smfr)
Comment 30 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.
Tobi Reif
Comment 31 2018-04-12 10:15:26 PDT
It would be great if this issue could get fixed 😀
Said Abou-Hallawa
Comment 32 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.
Tobi Reif
Comment 33 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/ .
Said Abou-Hallawa
Comment 34 2019-04-02 12:02:50 PDT
WebKit Commit Bot
Comment 35 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>
WebKit Commit Bot
Comment 36 2019-04-02 17:26:11 PDT
All reviewed patches have been landed. Closing bug.
Said Abou-Hallawa
Comment 37 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.
Tobi Reif
Comment 38 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!
Tobi Reif
Comment 39 2019-04-04 01:57:46 PDT
It works in the latest Safari Tech Preview - thanks!
Tobi Reif
Comment 40 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!
Note You need to log in before you can comment on or make changes to this bug.