User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre If animating some filter attributes, like pointsAtY, pointsAtX... and in document is no other SMIL animation, rendering is not repainted until at least some user operation, like click, does not occur. Reproducible: Always Steps to Reproduce: 1. Open file from attachment or from URL 2. Image seems static, but when you start clicking on it 3. you can see, that animation is really running, because rendering changes Actual Results: looks like static image Expected Results: you should see animation If you add some other animation to the document, which forces redraw, then it "starts" to be working... (in second attachment)

Confirmed on today's nightly: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre

Comment on attachment 494797 [details] [diff] [review] patch with reftests The nsSVGFilters.cpp tweaks look good. I don't entirely understand the nsCSSFrameConstructor.cpp tweak, though: >diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp >--- a/layout/base/nsCSSFrameConstructor.cpp >+++ b/layout/base/nsCSSFrameConstructor.cpp >@@ -4878,27 +4878,27 @@ nsCSSFrameConstructor::FindSVGData(nsICo > SIMPLE_SVG_CREATE(feComposite, NS_NewSVGLeafFrame), >- SIMPLE_SVG_CREATE(feComponentTransfer, NS_NewSVGLeafFrame), >+ SIMPLE_SVG_CREATE(feComponentTransfer, NS_NewSVGContainerFrame), > SIMPLE_SVG_CREATE(feConvolveMatrix, NS_NewSVGLeafFrame), >- SIMPLE_SVG_CREATE(feDiffuseLighting, NS_NewSVGLeafFrame), >+ SIMPLE_SVG_CREATE(feDiffuseLighting, NS_NewSVGContainerFrame), > SIMPLE_SVG_CREATE(feDisplacementMap, NS_NewSVGLeafFrame), [...] > SIMPLE_SVG_CREATE(feOffset, NS_NewSVGLeafFrame), >- SIMPLE_SVG_CREATE(feSpecularLighting, NS_NewSVGLeafFrame), >+ SIMPLE_SVG_CREATE(feSpecularLighting, NS_NewSVGContainerFrame), What's the significance of this change?

Note how I'm handling invalidation for filter animation in bug 589439 without creating new frames.

(In reply to comment #5) > What's the significance of this change? These filters can have child elements. Without the parents being containers, the children don't get frames.

I'm not saying my approach is "right" or "better" BTW, just that it's worth looking at.

(In reply to comment #6) > Note how I'm handling invalidation for filter animation in bug 589439 without > creating new frames. And you think that's better? In this case it could work as the children don't have any css properties. What do you think Daniel.

If we're going to go down your route Jonathan we should properly suppress the frames in nsCSSFrameConstructor and not pretend to create them when in any working markup we actually won't

(In reply to comment #6) > Note how I'm handling invalidation for filter animation in bug 589439 without > creating new frames. In particular: +void +nsSVGComponentTransferFunctionElement::DidAnimateNumberList(PRUint8 aAttrEnum) +{ + // We don't have a frame, so use our parent's + nsCOMPtr<nsIDOMSVGFEComponentTransferElement> compTrans = + do_QueryInterface(GetParent()); + if (compTrans) { + // nsSVGLeafFrame doesn't implement AttributeChanged. + nsIFrame* frame = static_cast<nsSVGFE*>(GetParent())->GetPrimaryFrame(); + if (frame) { + nsSVGEffects::InvalidateRenderingObservers(frame); + } + } (In reply to comment #9) > And you think that's better? In this case it could work as the children don't > have any css properties. What do you think Daniel. It strikes me as more conservative -- it's probably better to use existing frames rather than creating extra frames that we (apparently?) don't otherwise need. I'd defer to you guys & roc on this :) but my initial leaning is to prefer jwatt's method. (If we go with that, though, we should probably *assert* that we don't have a frame -- that way, if we ever add a frame for the child elements for some other reason, we'll know to update this code to use our own frame.) Also, Robert: could you add a reftest for feSpecularLighting as well? (You've got tests for the other two tweaked elements from Comment 5's quoted chunk, which look great -- we might as well have test coverage for that third one, too, though.)

I have no strong preference, but it would be nice not to allocate memory for frames if we don't need them. (In reply to comment #10) > If we're going to go down your route Jonathan we should properly suppress the > frames in nsCSSFrameConstructor and not pretend to create them when in any > working markup we actually won't It would be nice if the code wasn't misleading, agreed. (In reply to comment #11) > (If we go with that, though, we should probably *assert* that we don't have a > frame -- that way, if we ever add a frame for the child elements for some other > reason, we'll know to update this code to use our own frame.) Actually I was going to suggest that we use a kind of mixture of Robert's and my approach. Specifically to have DidAnimateAttr look up the element parent chain for the first element for which GetPrimaryFrame returns something, rather than just calling GetPrimaryFrame once. (It should probably assert that it doesn't hit an nsSVGFilterElement.)

Comment on attachment 494797 [details] [diff] [review] patch with reftests > protected: > virtual NumberAttributesInfo GetNumberInfo(); >+ virtual void DidAnimateNumber(PRUint8 aAttrEnum); > virtual EnumAttributesInfo GetEnumInfo(); >+ virtual void DidAnimateEnum(PRUint8 aAttrEnum); > Could we not have: > protected: > virtual NumberAttributesInfo GetNumberInfo(); >+ virtual void DidAnimateNumber(PRUint8 aAttrEnum) { >+ DidAnimateAttr(this); >+ } > virtual EnumAttributesInfo GetEnumInfo(); >+ virtual void DidAnimateEnum(PRUint8 aAttrEnum) { >+ DidAnimateAttr(this); >+ } > I know since these are virtual we don't get the benefits of inlining of the code, but since the implementation is only a single line it might as well be here, which saves some searching up and down the file to check on implementations when examining the code.

dholbert points out that with such a loop the invalidation could be wrong in the face of display:none. So rather than a loop in DidAnimateAttr, how about passing the parent to DidAnimateAttr. nsSVGComponentTransferFunctionElement::DidAnimateEnum(PRUint8 aAttrEnum) { if (Parent()) { DidAnimateAttr(Parent()); } }

I believe checking the parent isn't null is necessary since the SMIL code can hold onto element's via cached nsSMILValues, right dholbert?

Good question -- the null-parent check would be unnecessary. In a given SMIL sample, there's no way that we can evaluate animations on an element that lacks a parent (and trigger DidAnimateEnum() calls on it). As soon as an element is detached from the tree, any <animate> children will be immediately unregistered (& won't participate in the next SMIL sample), and any animations that use xlink:href to target its ID will not find it in the next sample. > since the SMIL code can hold onto element's via cached nsSMILValues That's true (via some types of nsSMILValues that have owning references, and also via nsSMILTargetIdentifiers). However.... (a) the point above stands (those elements can't be targeted by animations) ...and also... (b) those nsSMILValues and nsSMILTargetIdentifiers are only used for equality-checking (c) those nsSMILValues and nsSMILTargetIdentifiers (and their references) will be dropped in the first sample after their target is removed from the tree. So, to sum up: in e.g. nsSVGComponentTransferFunctionElement::DidAnimateEnum -- we should assert that we don't have a frame, and either assert or comment that we assume we *do* have a parent element.

Comment on attachment 495225 [details] [diff] [review] address review comments You can use GetFlattenedTreeParent instead of nsSVGUtils::GetElementParent if you change DidAnimateAttr's argument to nsIContent. (At some point nsSVGUtils::GetElementParent should die, or at least be changed to use GetFlattenedTreeParent.)

I'll leave that for bug 484966 if that's OK.

Seems like you might as well use GetFlattenedTreeParent in the code you're already adding, but either way...

Pushed http://hg.mozilla.org/mozilla-central/rev/f227a6e64b3f

And http://hg.mozilla.org/mozilla-central/rev/17bc5bbcb0c6 since somehow 'hg import' didn't add the new test files!

Actually, that wasn't 'hg import's fault - this was one of the patches I pushed that wasn't created using 'hg export', so I had to apply it manually but forgot I then needed to manually add new files.