Animating visibility and display attributes

[GoogleCodeExporter]

The file svggen/animate-elem-31-t.svg fails to animate correctly with 
Owlephant/Flash 10/forceflash=true.

This file contains a number of complex animations involving the visibility and 
display attributes.  This file animated correctly prior to the patch for Issue 
531.

The problems are:

1. Visibility of stroked and filled elements
When the visibility attribute of stroked and filled elements is initially 
"hidden", subsequent setting to "visible" results in nothing being rendered. 
Two conditions were missed in the 531 patch to ensure strokes and fills are 
performed even if visibility="hidden".  (SVGNode.nodeBeginFill 
nodeBeginStroke). 

2. Visibility="inherit" blocking inherited value
When SVGNode.setVisibility() is called, the drawSprite.alpha value is 
recursively set on all child nodes which do not have a visibility attribute 
specified.  If visibility="inherit", the recursion stops, but should continue.  
This scenario existed prior to the 531 patch but was not evident due to the 
full redraw of node branches.

3. SVGNode.getAnimAttribute() returning baseVal when no effective animations
In the case where a node is targeted by animations but none of them are 
effective at the time of calling getAnimAttribute, the return value is baseVal 
(the unanimated value).   When the attribute value is "inherit", baseVal will 
then be the unanimated parent  value. The return value should be null as if 
there were no animations at all (similar to !foundAnimation earlier in the 
function). Again, this action existed before but was not evident.

4. Animating the display attribute with to="inherit" fill='freeze'
The display attribute does not inherit and If not specified on an element the 
initial value is 'inline'. The display attribute can however have a value of 
"inherit". The example file contains some complex animations of display using 
'inherit' and fill="freeze".  The 531 patch only updates (invalidates) the 
targetNode while the animation is active.  When the animation is frozen at 
"inherit", any changes to a parent node are not propagated to targetNode. Prior 
to the 531 patch, the full redraw took care of child nodes.

The attached patch adds display to the array of ATTRIBUTES_NOT_INHERITED (saves 
searching up the tree to the root svg). To avoid the inherit propagation 
problem, any value other than "none" is treated as "inline".

The attached patch fixes the above problems.

Original issue reported on code.google.com by k...@svgmaker.com on 7 Oct 2010 at 6:12

Attachments:

• issue531-b.diff

[GoogleCodeExporter]

Original comment by bradneub...@gmail.com on 16 Nov 2010 at 9:44

• Added labels: Patch

[GoogleCodeExporter]

Ken-
Thanks for the patch! 

I'd like to integrate these fixes but it looks like there is further work to 
do. I have attached a test file that shows a problem with the patch provided. 
In this case, an error is triggered when the animate element is added. The 
problem is deep inside when it parses parameters for the animation. It calls 
parseMinIntervalDuration which calls this.getAttribute('min', '0'). This 
returns null. It was expected to return '0'. This causes an exception.

The problem with the patch is that it seems to have reworked the default value 
behavior of getAttribute() and no longer seems to honor the passed in default 
value. Now, one could argue that the default value should be handled inside 
getAttribute or at the end if the inheritance chain in SVGSVGNode and so forth. 
However, a counter argument is the 'fill' attribute which has a special default 
value in SVGAnimateNode. The 'fill' attribute means "how do I fill time after 
the animation is finished?" in SVGAnimateNode and the default is "remove" but 
it also means "what do I paint in the element?" in SVGNode and the default is 
"black". It may make sense to honor the default if provided, otherwise use an 
internal default. 

Note that if we try to support inheritance and default values inside 
getAttribute except for a few exceptions like 'fill', then I think we need need 
to review and update ATTRIBUTES_NOT_INHERITED and the default values in SVGNode 
and SVGSVGNode for all attributes, including the SMIL attributes 'min', 'max', 
'from', 'to', 'by', 'value', 'begin', 'end', 'repeatCount', 'repeatDur', 
'restart', 'additive', 'accumulate', 'calcMode', 'keyTimes', 'keySplines', 

Ultimately, the getAttribute, getStyle, getStyleOrAttribute, _getAttribute, 
etc, has become too complex. The complex design goes back to the original 
codebase so I have been reluctant to redesign it, but the endless add-ons have 
become cumbersome. I wonder if adding yet another function 
(getInheritedAttribute) is the right direction here given that there already 
exists an inherit flag in getAttribute. I am not sure, mainly because I do not 
fully understand the design change you are proposing with that new function or 
what was wrong beforehand. You provided a good explanation of what behavior 
changes your changes result in, but it would be helpful to provide a bit more 
of an explanation of what your intent is with respect to changing the way 
getAttribute works and why it is better. 

Maybe there is a way to break up this patch such that the smaller obvious 
improvements can be separated from the design changes, so that I may be able to 
integrate some of the changes more quickly. I'd also consider a more sweeping 
overhaul of the getAttribute family of functions if it simplified the design 
and provided equivalent functionality. I'll leave that choice up to you.
Thanks again.

Original comment by grick23@gmail.com on 28 Nov 2010 at 9:10

• Changed state: Accepted
• Added labels: Priority-Medium
• Removed labels: Priority-Low

Attachments:

• i556.html
• i556.svg

[GoogleCodeExporter]

Rick,

The changes in the patch shouldn't have affected the defaultValue behavior.  At 
least, there was no intention to.  Looking closer, I see where the problem is.  
getInheritedAttribute should return defaultValue at the end, rather than null.  
I don't remember having this problem when I made the patch.  Why doesn't it 
happen in the normal case (not adding an animate element via script)?

I really wasn't trying to make any sweeping design changes, just trying to do 
the minimum to get display animations using inherit to work.  
getInheritedAttribute was just an attempt to short circuit the attribute 
processing for display while trying not to affect the rest of the code.  To be 
honest, I don't really care about animating display with a value of inherit.  I 
just wanted animate-elem-31-t.svg to pass.

I should have split this patch up into 4 issues.  Too lazy to make 4 reduced 
test cases.  I'll work on that.

As for how getAttribute etc works... yes, complex.  I'll put some thought into 
it.

I compiled r1269 and ran the test file and I get an error in 
SVGAnimateNode/timeIntervalEnded().  Looks like targetNode is not defined yet.  
That is after the exception you found with the patch but still in 
parseParameters.

Original comment by k...@svgmaker.com on 29 Nov 2010 at 7:48

[GoogleCodeExporter]

> Why doesn't it happen in the normal case (not adding an animate element via 
script)?

The only thing I can think of is that in the "adding via script" case, the 
inheritance search up the tree doesn't reach the root SVGSVGNode - invoking the 
erroneous return of null from getInheritedAttribute.  This would imply that the 
node hasn't been inserted into the tree yet?   Normally, the search would make 
it to the root SVG node and defaultValue would be returned by 
SVGSVGNode.getAttribute.

Original comment by k...@svgmaker.com on 30 Nov 2010 at 4:28

[GoogleCodeExporter]

Yes, that makes sense. Here is the code in SVGViewerWeb.js_appendChild:

            for (var i:int = 0; i < addMe.length; i++) {
                // parse the newly appended elements into SVGNode and
                // all of its children as well
                var element:SVGNode = parent.parseNode(addMe[i]);
                element.forceParse();

                // now actually append the element to our display
                parent.appendSVGChild(element);
            }

The forceParse() is causing the animation parameters to be parsed, where the 
exception occurs. Then, right after, the new element is added to the parent. We 
can consider swapping those calls but I suspect that is not the right solution.

I can take another look at your patch with the getInheritedAttribute change to 
see if that makes sense but I still have concerns with the whole complexity 
thing. So I am torn between wanting to integrate a patch from the community, 
which I consider a high priority obligation, but also realizing it will take a 
lot more time than I was hoping to really give this area of code the thorough 
review it deserves.

Here is what I am going to do. Patches that I can integrate fairly easily are 
top priority and I've given my best shot at that with your patch but it did not 
work out on my first pass. I hope I have provided help with improving the patch 
and I will come back to this, but for now I am going to proceed onto the next 
item on my priority list. I have been planning to look at all the sizing 
issues: 291, 314, 427, 357, 451, and 512. After that and maybe a few other 
things, I'll come back to this issue and I'll do the best I can to integrate 
the patch(es) again. If you are able to make improvements that would be great 
otherwise I'll work with what is available. thanks

Original comment by grick23@gmail.com on 2 Dec 2010 at 3:53

[GoogleCodeExporter]

No problem Rick.

I'm working on breaking out the first and third items of the original post.

The 2nd item - Visibility="inherit" blocking inherited value - although I know 
it happens, I can't make a reduced test case to visually show the problem.  
This is because in the actionscript, visibility functions like display does.  
That is, as soon as one node is set to hidden, the alpha of the sprite is set 
to zero.  This means that any visibility="visible" further down the tree won't 
be seen.  So until visibility works like it should, fixing setVisibility as 
proposed won't have any effect.

The 4th item - Animating the display attribute with to="inherit" fill='freeze' 
- which introduced getInheritedAttribute is not an important issue (at all) in 
my opinion.  A visually identical result can be achieved by using "inline" 
instead of "inherit" in the SVG.   I only attempted to "fix" this for the sake 
of getting svggen/animate-elem-31-t.svg to work.

I agree with your concerns about the complexity issue.  It wouldn't bother me 
if you didn't get back to this issue, or even closed it.

Just a note, I think that there is still a problem with i556.html further on in 
parseParameters.  SVGAnimateNode.timeIntervalEnded() may need an "if 
(targetNode)".

Original comment by k...@svgmaker.com on 2 Dec 2010 at 6:35