Bug 118735 – feImage fails if referenced node contains radialGradient declaration

• Component: SVG
• Status: RESOLVED CONFIGURATION CHANGED

[Branimir Lambov]

Created attachment 206784 [details]
Testcase

Steps to reproduce the problem:
Use feImage to reference a branch that contains any radialGradient (including an empty definition). See the attached feimage-radial-broken.svg.

What is the expected behavior?
feImage should not be affected by the presence of a radialGradient declaration.

The attached SVG should show two identical rectangles with black bars.

What went wrong?
The feImage is drawn using the wrong transform. The transform changes with the size of the containing SVG (e.g. window resize).

If one removes the empty radialGradient declaration, the image is drawn correctly.

Works on Opera 12.10 and IE 10, fails on Chrome and Safari.

See also crbug.com/260709, crbug.com/257648.

[Rob Buis]

Created attachment 209544 [details]
Patch

[Rob Buis]

Created attachment 209548 [details]
Patch

[Dirk Schulze]

Comment on attachment 209548 [details]
Patch

Can the test really reproduce the problem? IIRC without your patch the test would pass as well. Wouldn't it? Maybe a ref test would be better here?

Fix looks good.

[Rob Buis]

(In reply to comment #3)
> (From update of attachment 209548 [details])
> Can the test really reproduce the problem? IIRC without your patch the test would pass as well. Wouldn't it? Maybe a ref test would be better here?
> 
> Fix looks good.

You are right, will fix.

[Rob Buis]

Created attachment 209772 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 209772 [details]
Patch

Clearing flags on attachment: 209772

Committed r154713: <http://trac.webkit.org/changeset/154713>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

I think we should roll out this patch.

This patch stops us from propagating the relative length flag up the tree for renderers that are not SVGGraphicsElements. This has the effect of fixing this specific issue because the gradient element is not an SVGGraphicsElement, but it masks the real bug: SVGFEImage.cpp doesn't understand relative lengths (http://trac.webkit.org/browser/trunk/Source/WebCore/svg/graphics/filters/SVGFEImage.cpp?rev=167295#L94). This probably regressed the propagation of relative lengths for non-SVGGraphicsElements such as SVGPatternElements.

This small tweak to the testcase will still trigger the root cause:
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="300px" height="300px">
  <g>
    <filter filterUnits="userSpaceOnUse" primitiveUnits="userSpaceOnUse" id="flt" x="0" y="0" width="50" height="100">
      <feImage xlink:href="#img" />
    </filter>
    <rect filter="url(#flt)" width="50" height="100"/>

    <g transform="translate(50 0)">
      <g id="img">
        <rect width="1%" height="100" fill="green"/>
        <rect width="50" height="100" fill="green"/>
      </g>
    </g>
  </g>
</svg>

We should probably also fix the quite interesting while loop here: http://trac.webkit.org/browser/trunk/Source/WebCore/svg/SVGElement.cpp#L1130

[Branimir Lambov]

Philip, aren't all resources (such as patterns, gradients, masks etc.) only relative to their point of use and only reflected in the appearance of their point of use? And since their point of use can be anywhere in the DOM (or in a different document altogether), why would they need to propagate any of their properties to their parent?

[Philip Rogers]

(In reply to comment #9)
> Philip, aren't all resources (such as patterns, gradients, masks etc.) only relative to their point of use and only reflected in the appearance of their point of use? And since their point of use can be anywhere in the DOM (or in a different document altogether), why would they need to propagate any of their properties to their parent?

What about clipPath? It is not an SVGGraphicsElement but it grabs it's bounds from its parent so we need to propagate its relative length info up the tree. See http://jsfiddle.net/6UxrQ/1

The more general issue with this patch is the testcase I posted in #8. This patch papers over a small subset of a real bug in SVGFeImage.cpp.

[Dirk Schulze]

(In reply to comment #10)
> (In reply to comment #9)
> > Philip, aren't all resources (such as patterns, gradients, masks etc.) only relative to their point of use and only reflected in the appearance of their point of use? And since their point of use can be anywhere in the DOM (or in a different document altogether), why would they need to propagate any of their properties to their parent?
> 
> What about clipPath? It is not an SVGGraphicsElement but it grabs it's bounds from its parent so we need to propagate its relative length info up the tree. See http://jsfiddle.net/6UxrQ/1
> 
> The more general issue with this patch is the testcase I posted in #8. This patch papers over a small subset of a real bug in SVGFeImage.cpp.

Does Blink have a solution for the real problem yet?

[Philip Rogers]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Philip, aren't all resources (such as patterns, gradients, masks etc.) only relative to their point of use and only reflected in the appearance of their point of use? And since their point of use can be anywhere in the DOM (or in a different document altogether), why would they need to propagate any of their properties to their parent?
> > 
> > What about clipPath? It is not an SVGGraphicsElement but it grabs it's bounds from its parent so we need to propagate its relative length info up the tree. See http://jsfiddle.net/6UxrQ/1
> > 
> > The more general issue with this patch is the testcase I posted in #8. This patch papers over a small subset of a real bug in SVGFeImage.cpp.
> 
> Does Blink have a solution for the real problem yet?

Sadly, Blink does not.

[Brent Fulgham]

This seems to work properly in Safari, but not in Chrome or Firefox.

[Radar WebKit Bug Importer]

<rdar://problem/97100663>