After applying the patch for bug 607537 and viewing the attached URL the Swiss flag that is applied to the animated path (i.e. animated 'd' attribute) doesn't resize to fill the path despite the fact that it is defined to fill the objectBoundingBox of the target element. i.e. <pattern id="fillSwitzerland" patternContentUnits="objectBoundingBox" width="100%" height="100%"> <rect fill="red" stroke="none" x="0" y="0" width="1" height="1"/> Instead gaps can be see between the path and the path stroke at some points. It's as if we're not using the latest animated state of the path geometry to calculate the bounding box for the pattern target content--or something of that sort. I haven't looked into this in any detail. This may prove to be dupe since we have a few similar bugs regarding not fetching the correct animated state of certain properties (e.g. bug 544809, bug 615658) but so far I don't think so since we're not actually animating the pattern properties here.

In the attached test case the pattern fails to stretch to fill the path as it grows. If we change the offset for the opacity animation on the pattern then we can see that the bounding box appears to be established at the moment the pattern becomes visible (even if only partially) and is fixed from then on. If we remove the animated opacity from the pattern then there appears to be no problem. Unfortunately using setCurrentTime and pauseAnimations seems to circumvent the problem so I haven't yet made a suitable reftest.

With this reduced test case, this bug no longer depends on bug 607537 not is it related to animating path data since a <rect> also produces the same problem.

(In reply to comment #1) > If we remove the animated opacity from the pattern then there appears to be no > problem. Just correcting that--the trigger appears to be the <g> rather than the opacity. If we remove the animated opacity but leave the <g> inside the pattern the same problem occurs. However, it's still the case that we seem to establish the size when the pattern content first becomes visible (i.e. opacity != 0)

the pattern's width doesn't change, it's always 100%. Unfortunately the meaning of 100% does change. Somehow the rect needs to communicate that to the pattern (or gradient). nsSVGPathGeometryFrame::AttributeChanged is where you get notified something is changing. You could see whether you've got a fill or stroke paint server e.g. nsSVGPaintServerFrame *ps = GetPaintServer(&style->mFill, nsSVGEffects::FillProperty()); then if ps != nsnull you just call nsSVGEffects::InvalidateRenderingObservers(ps); and that should work.

If the child ctm is not changing then I think comment 4 is wrong.

We basically need to do what we're already doing for clip paths: http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGClipPathFrame.cpp#102 Not sure who else needs to look at this as I'm not really familiar with what's going on in layout.

Just for further explanation, the reason this test was failing is that nsSVGGFrames cache the CanvasTM. So, provided the children of our pattern are <rect>s and the link we're ok, but once you introduce a parent <g> it fails. Also, regarding the earlier comments about animation. Basically, when opacity is 0 we bail out of painting, so as soon as animation was causing the element to be visible we'd cache the CanvasTM at that point and keep reusing it even when it had changed. This patch fixes the behaviour when we pattern dest changes either because the pattern is being using in multiple places, it changes due to script, or it changes due to animation.

I understand what's happening sufficiently that further review is unnecessary IMHO.

(In reply to comment #8) > I understand what's happening sufficiently that further review is unnecessary > IMHO. Cheers, thanks for the quick review!

Comment on attachment 497418 [details] [diff] [review] Patch v1a Requesting approval to land. Fixes broken scaling behaviour when we have a pattern contained in a <g> element when: a) the pattern is re-used b) the target content geometry changes due to script c) the target content geometry changes due to animation Includes test. Bug detected in real-world use case.

Don't masks have the same problem in nsSVGMaskFrame::ComputeMaskAlpha?

Probably best to have a new bug since this is reviewed and approved.

(In reply to comment #12) > Probably best to have a new bug since this is reviewed and approved. Thanks Robert. Yes, I had meant to look for similar pieces of code. I wonder if there are any others (e.g. in any of the filters). If you get to filing that bug, please leave a link here.

(In reply to comment #13) > (In reply to comment #12) > > Probably best to have a new bug since this is reviewed and approved. > > Thanks Robert. Yes, I had meant to look for similar pieces of code. I wonder if > there are any others (e.g. in any of the filters). Only masks.

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