Bug 118735 - feImage fails if referenced node contains radialGradient declaration
Summary: feImage fails if referenced node contains radialGradient declaration
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-16 08:35 PDT by Branimir Lambov
Modified: 2014-05-01 14:52 PDT (History)
8 users (show)

See Also:


Attachments
Testcase (587 bytes, image/svg+xml)
2013-07-16 08:35 PDT, Branimir Lambov
no flags Details
Patch (4.47 KB, patch)
2013-08-24 10:16 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.43 KB, patch)
2013-08-24 11:13 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (4.03 KB, patch)
2013-08-27 07:46 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Branimir Lambov 2013-07-16 08:35:42 PDT
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.
Comment 1 Rob Buis 2013-08-24 10:16:11 PDT
Created attachment 209544 [details]
Patch
Comment 2 Rob Buis 2013-08-24 11:13:56 PDT
Created attachment 209548 [details]
Patch
Comment 3 Dirk Schulze 2013-08-27 04:06:54 PDT
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.
Comment 4 Rob Buis 2013-08-27 07:42:53 PDT
(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.
Comment 5 Rob Buis 2013-08-27 07:46:09 PDT
Created attachment 209772 [details]
Patch
Comment 6 WebKit Commit Bot 2013-08-27 16:08:23 PDT
Comment on attachment 209772 [details]
Patch

Clearing flags on attachment: 209772

Committed r154713: <http://trac.webkit.org/changeset/154713>
Comment 7 WebKit Commit Bot 2013-08-27 16:08:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Philip Rogers 2014-04-29 20:28:59 PDT
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
Comment 9 Branimir Lambov 2014-04-30 04:51:21 PDT
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?
Comment 10 Philip Rogers 2014-04-30 14:25:29 PDT
(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.
Comment 11 Dirk Schulze 2014-05-01 00:00:56 PDT
(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?
Comment 12 Philip Rogers 2014-05-01 14:52:20 PDT
(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.