615788 - Reusing SVG patterns with objectBoundingBox units fails to update CTM of child content

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Reduced test case

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.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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)

Comment 4

[Robert Longson [:longsonr]]

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.

Comment 5

[Robert Longson [:longsonr]]

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

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Patch v1a

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.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 8

[Robert Longson [:longsonr]]

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

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #8)
> I understand what's happening sufficiently that further review is unnecessary
> IMHO.

Cheers, thanks for the quick review!

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 11

[Robert Longson [:longsonr]]

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

Comment 12

[Robert Longson [:longsonr]]

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

Comment 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.

Comment 14

[Robert Longson [:longsonr]]

(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.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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