...where "work" is defined as passing reasonable amount of SVG animation test suite so that remaining issues can be tracked as individual bugs.
Created attachment 20419 [details] Implement (most of) it It does - Full SMIL interval timing model including syncbase and event base timing (the hard part). - CSS and XML attribute animation. - Linear, discrete and spline calcModes. - Values animation with keyTimes and keySplines. - Link activated animations. - Pretty good support for <animate> and <set> animations - Basic support for <animateColor>, <animateMotion> and <animateTransform>. This passes some 35 of the 56 tests in W3C SVG animation test suite, a bunch more with some subtest failures. What is still missing - Additive animation with multiple animations operating on the same property. This is a major architectural feature in animation code. It shouldn't be too hard to add. - Only <animate> implements accumulate. - <animateMotion> does not do paths, keypoints, rotate. - <animateTransform> does not work with text, some transforms are broken. - calcMode paced is missing. - repeat, beginEvent, endEvent are missing. - accesskey() is missing. - JS does not see correct values for baseVal/animVal, changing values that are being animted for a script produces wrong results. Not maintaining separate presentation values for XML attributes is an architectural problem in our SVG code. - Some other stuff I forgot or do not know about. There are three new classes: - SMILTime which implements time math including special "indefinite" and "unresolved" values. - SMILTimeContainer which runs the clock and manages animations ("time container" is a SMIL term) - SMILTimingElement, an abstract class which inherits SVGElement and is inherited by SVGAnimationElement. It implements SMIL timing model semantics.
Same text, hopefully more readable: It does - Full SMIL interval timing model including syncbase and event base timing (the hard part). - CSS and XML attribute animation. - Linear, discrete and spline calcModes. - Values animation with keyTimes and keySplines. - Link activated animations. - Pretty good support for <animate> and <set> animations - Basic support for <animateColor>, <animateMotion> and <animateTransform>. This passes some 35 of the 56 tests in W3C SVG animation test suite, a bunch more with some subtest failures. What is still missing: - Additive animation with multiple animations operating on the same property. This is a major architectural feature in animation code. It shouldn't be too hard to add. - Only <animate> implements accumulate. - <animateMotion> does not do paths, keypoints, rotate. - <animateTransform> does not work with text, some transforms are broken. - calcMode paced is missing. - repeat, beginEvent, endEvent are missing. - accesskey() is missing. - JS does not see correct values for baseVal/animVal, changing values that are being animted for a script produces wrong results. Not maintaining separate presentation values for XML attributes is an architectural problem in our SVG code. - Some other stuff I forgot or do not know about. - SMILTime which implements time math including special "indefinite" and "unresolved" values. - SMILTimeContainer which runs the clock and manages animations ("time container" is a SMIL term) - SMILTimingElement, an abstract class which inherits SVGElement and is inherited by SVGAnimationElement. It implements SMIL timing model semantics.
Comment on attachment 20419 [details] Implement (most of) it First run through it (feel to tired for a full review atm :( ), it looks mostly fine, a few comments though: In +void SVGAnimateElement::applyAnimatedValueToElement(unsigned repeat) "valueToApple" seems like it may be a tyop :D I suspect parseNumberValueAndUnit will parse "100 px/%" etc which seems bogus Adding commented out code in SVGAnimateMotionElement::parseMappedAttributes seems like an accident, and isn't commented itself so i have no idea why :( Tyop in m_baseIndexInTranformList -- missing 's' Are you sure about the comment in SVGElement::updateStyleAttributeIfNeeded? i've seen plenty of examples of people using "style" attributes in svg, it seems odd for it to not be accessible form the dom... I'm not happy with doubleSort/sortTimeList. doubleSort makes an assumption that each m_beginTime will always be the first member of SMILTime which seems really icky...
Hey Antti, had a quick look, unfortunately not much time atm, but I'm impressed! Excellent work! I just want to comment on one issue, as it's related to the previous SVG AnimProperty Liveness work: > - JS does not see correct values for baseVal/animVal, changing values that are > being animted for a script produces wrong results. Not maintaining separate > presentation values for XML attributes is an architectural problem in our SVG > code. The architecture for handling these updates is basically existant. That's part of the animated_property_* mystery (start/stop##Property functions). The idea is that <animate> has to mark a certain property (if it animates an XML prop) as start()ed. The underlying value is cached in a HashMap in the SVGDocumentExtensions, and all calls to 'set##Property' only influence the value residing in the HashMap, but not the actual property itself. This assures that you can still call 'setAttribute(...)' from JS, while an animation is running. You may want to have a look at the concept. It's old and Eric & me came up with this more than a year ago, while planning the future animation framework. Greetings, Niko
(In reply to comment #4) > The architecture for handling these updates is basically existant. That's part > of the animated_property_* mystery (start/stop##Property functions). The idea > is that <animate> has to mark a certain property (if it animates an XML prop) > as start()ed. The underlying value is cached in a HashMap in the > SVGDocumentExtensions, and all calls to 'set##Property' only influence the > value residing in the HashMap, but not the actual property itself. This assures > that you can still call 'setAttribute(...)' from JS, while an animation is > running. You may want to have a look at the concept. It's old and Eric & me > came up with this more than a year ago, while planning the future animation > framework. Yeah, I know there is code there but I can't figure out if it does the right thing and indeed how it is supposed to be used. I think someone with better understanding of that code will have to wire it together. Note that there are two separate problems here with different solution. One for animated CSS properties and another for XML attributes. For CSS properties the solution is simple and the spec tells exactly what to do. You write your animated values to the override stylesheet and the base value is what you get with getComputedStyle() (without override style sheet). Since WebCore does not support override stylesheets, for now the values are written to inline stylesheet instead. Since many SVG attributes are mapped to CSS this applies to them as well. Pure XML attributes are the problematic ones. > > Greetings, > Niko >
Created attachment 20438 [details] Updated patch Olliej's comments and some other tweaks
(In reply to comment #3) > In +void SVGAnimateElement::applyAnimatedValueToElement(unsigned repeat) > > "valueToApple" seems like it may be a tyop :D Lol, fixed. > I suspect parseNumberValueAndUnit will parse "100 px/%" etc which seems bogus Added check for that. > Adding commented out code in SVGAnimateMotionElement::parseMappedAttributes > seems like an accident, and isn't commented itself so i have no idea why :( Replaced with comment. > Tyop in m_baseIndexInTranformList -- missing 's' Fixed. > Are you sure about the comment in SVGElement::updateStyleAttributeIfNeeded? > i've seen plenty of examples of people using "style" attributes in svg, it > seems odd for it to not be accessible form the dom... Yeah, that was a bad idea Reverted that part. > I'm not happy with doubleSort/sortTimeList. doubleSort makes an assumption > that each m_beginTime will always be the first member of SMILTime which seems > really icky... Yeah, that was cheesy. A leftover from before I switched to SMILTime, now fixed.
Created attachment 20459 [details] another updated patch Code cleanups and some more comments. One bug fix (base value string is compared to null instead of empty in SVGAnimationElement::unapplyAnimation() so that empty base value can still be restored) to make test 40 pass completely.
Comment on attachment 20459 [details] another updated patch So I'm not sure it even makes sense to do any kind of real hard review of this patch. Obviously this work is incomplete. I did a cursory review anyway: parseNumberValueAndUnit the .endsWith stuff is about as inefficient as possible. :) Normally we use hash tables when you have more than 3-4 values. In this case, we're re-walking the string each time. :) if (isAdditive()) { 97 valueToApply = ColorDistance::addColorsAndClamp(base, m_animatedColor).name(); 98 } else no { } needed. I'm in general not a big fan of shadowing: AnimationMode animationMode = this->animationMode(); m_cachedMax = -1.; -1 should have some name here. The comparisons with 0 should also use a variable of this same name. if (m_cachedMax.isValid()) return m_cachedMax; You might want to add using SVGNames; at the top of SMILTimingElement.cpp I don't like your use of copying parsers. We've spent a lot of time in the last 3 years removing copying parsers and replacing them with scaning parsers. I agree with you that the rest of this work is much more important than that small issue however. All in all, I'm very very very very glad to see this patch land. I really hope that you plan to push this through to completion. This is the 3rd animation system we've seen, and they've all been partial. :) Hopefully this is the last one we'll see. We talked a lot over IRC. You said you'd land the UnitBezier stuff separately. I don't need to see the updated patch. Thanks again.
Thanks for the review! Sending WebCore/ChangeLog Sending WebCore/GNUmakefile.am Sending WebCore/WebCore.pro Sending WebCore/WebCore.vcproj/WebCore.vcproj Sending WebCore/WebCore.xcodeproj/project.pbxproj Sending WebCore/dom/Document.cpp Sending WebCore/history/CachedPage.cpp Sending WebCore/svg/SVGAElement.cpp Sending WebCore/svg/SVGAnimateColorElement.cpp Sending WebCore/svg/SVGAnimateColorElement.h Sending WebCore/svg/SVGAnimateElement.cpp Sending WebCore/svg/SVGAnimateElement.h Sending WebCore/svg/SVGAnimateMotionElement.cpp Sending WebCore/svg/SVGAnimateMotionElement.h Sending WebCore/svg/SVGAnimateTransformElement.cpp Sending WebCore/svg/SVGAnimateTransformElement.h Sending WebCore/svg/SVGAnimationElement.cpp Sending WebCore/svg/SVGAnimationElement.h Sending WebCore/svg/SVGDocumentExtensions.cpp Sending WebCore/svg/SVGElement.cpp Sending WebCore/svg/SVGSVGElement.cpp Sending WebCore/svg/SVGSVGElement.h Sending WebCore/svg/SVGSetElement.cpp Sending WebCore/svg/SVGSetElement.h Deleting WebCore/svg/SVGTimer.cpp Deleting WebCore/svg/SVGTimer.h Sending WebCore/svg/SVGUseElement.cpp Deleting WebCore/svg/TimeScheduler.cpp Deleting WebCore/svg/TimeScheduler.h Adding WebCore/svg/animation Adding WebCore/svg/animation/SMILTime.cpp Adding WebCore/svg/animation/SMILTime.h Adding WebCore/svg/animation/SMILTimeContainer.cpp Adding WebCore/svg/animation/SMILTimeContainer.h Adding WebCore/svg/animation/SVGSMILElement.cpp Adding WebCore/svg/animation/SVGSMILElement.h Transmitting file data ............................... Committed revision 31801.
*** Bug 18187 has been marked as a duplicate of this bug. ***
*** Bug 18188 has been marked as a duplicate of this bug. ***