Bug 5968 – Add support for spreadMethod=reflect and repeat on SVG gradients (for CoreGraphics platforms)

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

gradients does not support spreadMethod

This is easy to see in this test case:
http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-pservers-grad-10-b.html

There is no support for this type of thing in CG, so we would have to do our own.  This is an extremely 
uncommon feature, so this is very low priority.

[Eric Seidel (no email)]

This is a pretty obscure feature.  It would be nice to support this (and we'll likely have to, since the W3C 1.1 test suite covers it), however it's not particularly high priority work.

[Gavin Sherlock]

Looks like:

spreadMethod=pad is supported, but that:

spreadMethod=reflect and
spreadMethod=repeat

are buggy

[Dirk Schulze]

Only CG doesn't support spreadMethods.

[Simon Fraser (smfr)]

I have some WIP code in bug 28152 to synthesize repeating gradients. Reflection would not be hard either.

[Matt Bishop]

I don't think I would call it "extremely" uncommon.  Hard to implement perhaps, but a useful part of SVG that has been in the spec since the beginning.

FWIW, Firefox seems to handle it OK.

[Simon Fraser (smfr)]

Test at LayoutTests/svg/W3C-SVG-1.1/pservers-grad-10-b.svg

[Radar WebKit Bug Importer]

<rdar://problem/44102127>

[Said Abou-Hallawa]

Created attachment 348854 [details]
test case

Attaching a simpler test case.

[Justin Michaud]

Created attachment 349371 [details]
Patch

[Tim Horton]

Comment on attachment 349371 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        Add support for spreadMethod=repeat and reflect. Also, the opacity of a gradient is now
> +        be the result of multiplying stop-opacity with the opacity of the color.

"is now be" is not quite proper grammar.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:116
> +                FloatPoint gradientVectorNorm = data.point1 + -data.point0;

Is there a reason you can't just subtract and instead add a negative? This looks super weird to me.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:129
> +                CGFloat pixelSize = abs(CGContextConvertSizeToUserSpace(platformContext, CGSizeMake(1, 1)).width);

s/abs/CGFAbs/, I think that will fix the build (at least one of the build problems)

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:137
> +                        CGContextDrawLinearGradient(platformContext, gradient, CGPointMake(flip? end : end - width, 0), CGPointMake(flip? end - width : end, 0), 0);

Definitely should have spaces before the ternary operator's question marks. Also this (and the one below) are quite long lines, maybe pull the points out into locals? I'm not sure.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:148
> +                    CGContextRestoreGState(platformContext);

The weird "where's the save that corresponds to this restore" thought that popped into my head illustrates precisely why we like RAII objects for things like this :) Please use GraphicsContextStateSaver or CGContextStateSaver instead of this manual goop.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:153
> +                U_FALLTHROUGH;

just FALLTHROUGH;, no U_

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:160
>              bool needScaling = data.aspectRatio != 1;

Should you use the already-computed-and-stored gradient instead of calling platformGradient() again below? (I think so!)

> Source/WebCore/svg/SVGStopElement.cpp:102
> +    return colorWithOverrideAlpha(svgStyle.stopColor().rgb(), svgStyle.stopColor().alpha() / 255.0 * svgStyle.stopOpacity());

Might want a fastDivideBy255? (Also not totally sure what's happening here).

> LayoutTests/svg/gradients/spreadMethodAlpha-expected.svg:5
> +    <rect x="10" y="120" width="115" height="55" fill="rgba(255,0,0,0.5)"/>

We try not to use red in "good" results (I understand that the W3C test above does, but ... don't follow them :) )

[Justin Michaud]

Created attachment 349405 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 349405 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:118
> +                FloatPoint gradientVectorNorm(data.point1 - data.point0);

Normally we'd use a FloatSize to represent the delta between two points, but sadly we haven't implemented .dot() for FloatSize.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:120
> +                CGFloat angle = acos(gradientVectorNorm.dot(FloatPoint(1, 0)));

Rather than FloatPoint(1, 0) you can say { 1, 0 } to let the compiler deduce the type.

[Simon Fraser (smfr)]

Please use the better title I just entered in this bug.

[Justin Michaud]

Created attachment 349688 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 349688 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:121
> +                CGContextRotateCTM(platformContext, angle);

What is the origin of this rotation?

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:125
> +                FloatPoint point0 = CGPointApplyAffineTransform(data.point0, transform);
> +                FloatPoint point1 = CGPointApplyAffineTransform(data.point1, transform);

because the origin will impact whether this point transformation is correct, right?

[Justin Michaud]

Created attachment 349809 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 349809 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:144
> +                    // Find first gradient position to the left of the bounding box
> +                    int n = CGFloor((boundingBox.origin.x - gradientStart) / width);
> +                    gradientStart += n * width;
> +                    if (!(n % 2))
> +                        flip = false;

I think your first version of this patch where you manually do the left side then the right side was much more understandable than this. I can't convince myself that this code is correct.

[WebKit Commit Bot]

Comment on attachment 349809 [details]
Patch

Clearing flags on attachment: 349809

Committed r236024: <https://trac.webkit.org/changeset/236024>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 349809 [details]
Patch

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

> Source/WebCore/svg/SVGStopElement.cpp:102
> +    float colorAlpha = svgStyle.stopColor().alpha() / 255.0;

Should use alphaAsFloat here instead of dividing by 255.0.

[Said Abou-Hallawa]

*** Bug 199569 has been marked as a duplicate of this bug. ***