Bug 40985 – gradient stroke on lines does not work

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

Created attachment 59368 [details]
test case

The lines are black instead of bluish.

[Dirk Schulze]

I'm a bit confused, that the other browsers (Firefox, Opera) draw the stroke with the gradient. An object with a one dimensional boudingBox (height or width = 0) shouldn't be stroked with a gradient, but instead use a fallback color or ignore stroke at all. And a line doesn't have a height. We use black instead of ignoring the stroke, because of compatibillity reasons to older Firefox and Opera browsers.

(In reply to comment #1)
> I'm a bit confused, that the other browsers (Firefox, Opera) draw the stroke with the gradient. An object with a one dimensional boudingBox (height or width = 0) shouldn't be stroked with a gradient, but instead use a fallback color or ignore stroke at all. And a line doesn't have a height. We use black instead of ignoring the stroke, because of compatibillity reasons to older Firefox and Opera browsers.

The gradient here is specified with gradientUnits="userSpaceOnUse".
At the beginning of SVG 1.1 section 13.2.1 is written that gradients apply to fill and stroke properties of graphics elements. There is no mention for exceptions.

Where exactly the SVG specification effectively states that gradients do not apply to lines?

[Dirk Schulze]

(In reply to comment #2)
> (In reply to comment #1)
> > I'm a bit confused, that the other browsers (Firefox, Opera) draw the stroke with the gradient. An object with a one dimensional boudingBox (height or width = 0) shouldn't be stroked with a gradient, but instead use a fallback color or ignore stroke at all. And a line doesn't have a height. We use black instead of ignoring the stroke, because of compatibillity reasons to older Firefox and Opera browsers.
> 
> The gradient here is specified with gradientUnits="userSpaceOnUse".
> At the beginning of SVG 1.1 section 13.2.1 is written that gradients apply to fill and stroke properties of graphics elements. There is no mention for exceptions.
> 
> Where exactly the SVG specification effectively states that gradients do not apply to lines?

Sorry, I'm not sure where to find this paragraph atm. Maybe the following test can help you more: http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-pservers-grad-17-b.html The gradient is not drawn on all objetcs, since some of them have a one dimensional objectBoundingBox (width or height == 0).

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > I'm a bit confused, that the other browsers (Firefox, Opera) draw the stroke with the gradient. An object with a one dimensional boudingBox (height or width = 0) shouldn't be stroked with a gradient, but instead use a fallback color or ignore stroke at all. And a line doesn't have a height. We use black instead of ignoring the stroke, because of compatibillity reasons to older Firefox and Opera browsers.
> > 
> > The gradient here is specified with gradientUnits="userSpaceOnUse".

> > Where exactly the SVG specification effectively states that gradients do not apply to lines?
> 
> Sorry, I'm not sure where to find this paragraph atm. Maybe the following test can help you more: http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-pservers-grad-17-b.html The gradient is not drawn on all objetcs, since some of them have a one dimensional objectBoundingBox (width or height == 0).

If you really want to be helpful you may try to clear your confusion in first place.

The section that deals with bounding box (SVG 1.1, 7.11) says:
"Keyword objectBoundingBox should not be used when the geometry of the applicable element has no width or no height, such as the case of a horizontal or vertical line, even when the line has actual thickness when viewed due to having a non-zero stroke width since stroke width is ignored for bounding box calculations. When the geometry of the applicable element has no width or height and objectBoundingBox is specified, then the given effect (e.g., a gradient or a filter) will be ignored."

There is no rule in the latest SVG 1.1 specification which prohibits applying gradients specified with gradientUnits="userSpaceOnUse" to horizontal or vertical lines.

In the W3C test you gave the gradient is ignored on 3 objects because they are defined as horizontal lines AND the gradient is implicitly specified with gradientUnits="objectBoundingBox".

I don’t see a sane reason for imposing an additional (not mentioned in the SVG specification) discontinuous behavior similar to that described in the cited paragraph. It simply doesn’t make sense from a practical perspective to have a gradient on an inclined line, but not on a non-inclined (horizontal or vertical) line.

[Dirk Schulze]

(In reply to comment #4)

> If you really want to be helpful you may try to clear your confusion in first place.

If you realy want to be helpful you shouldn't be so ignorant and search for the relevant parts in the Spec yourself to avoid confusions in the first line.

> 
> The section that deals with bounding box (SVG 1.1, 7.11) says:
> "Keyword objectBoundingBox should not be used when the geometry of the applicable element has no width or no height, such as the case of a horizontal or vertical line, even when the line has actual thickness when viewed due to having a non-zero stroke width since stroke width is ignored for bounding box calculations. When the geometry of the applicable element has no width or height and objectBoundingBox is specified, then the given effect (e.g., a gradient or a filter) will be ignored."
> 
> There is no rule in the latest SVG 1.1 specification which prohibits applying gradients specified with gradientUnits="userSpaceOnUse" to horizontal or vertical lines.

You're right, gradients should just be ignored on the use of objectBoundingBox and an width or height == 0. Thanks for pointing this out. The issue is in 
http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderSVGResource.cpp#L137
We should check somehow, if the SVGGradientElement/SVGPatternElement resource units or contentUnits work with objectBoundingBox. Isn't that difficult.
First we need to remove the isEmpty boundingBox check at this place. If strokePaintingResource is not 0, we have to check it's element (either SVGGradientElement or SVGPatternElement) for the unit spaces. And set strokePaintingResource back to 0 if one of the units is equal to objectBoundingBox and the boundingBox is empty (width or height == 0).

[Nikolas Zimmermann]

Bug fixed, attaching patch soon.

[Nikolas Zimmermann]

Created attachment 70255 [details]
Patch

Integrated test case, within an already existing testcase in LayoutTests/svg/custom.
This moves us further to SVG 1.1 2nd edition compilance, and we now match Opera perfectly in several new testcases.

[Andreas Kling]

Comment on attachment 70255 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70255&action=review

LGTM, but I don't know the code well enough to say r+ with confidence.

> WebCore/rendering/RenderSVGResource.cpp:36
> +static inline RenderSVGResource* requestPaintingResource(const RenderSVGResourceMode& mode, RenderObject* object, const RenderStyle* style, Color& fallbackColor)

Passing RenderSVGResource as const-reference seems overkill.

> WebCore/rendering/RenderSVGResource.cpp:78
> +                Color visitedColor = visitedPaint->color();

const Color& visitedColor = visitedPaint->color();

[Dirk Schulze]

Comment on attachment 70255 [details]
Patch

Talked about two little snippets with Niko. Rest looks great. Test were using SVGFonts but the reference was wrong in the test. r=me

[Nikolas Zimmermann]

Landed in r69413.

[Csaba Osztrogonác]

(In reply to comment #10)
> Landed in r69413.

It broke svg/custom/gradient-with-1d-boundingbox.svg on Qt bot:
http://build.webkit.org/results/Qt%20Linux%20Release/r69413%20%2821757%29/svg/custom/gradient-with-1d-boundingbox-pretty-diff.html

Could you check it, please?

[Csaba Osztrogonác]

I checked the pixel result, it passes,
so I added Qt specific expected file:
http://trac.webkit.org/changeset/69443