Bug 5968 - Add support for spreadMethod=reflect and repeat on SVG gradients (for CoreGraphics platforms)
Summary: Add support for spreadMethod=reflect and repeat on SVG gradients (for CoreGra...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P4 Normal
Assignee: Nobody
URL: http://www.w3.org/Graphics/SVG/Test/2...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2005-12-06 03:53 PST by Eric Seidel
Modified: 2018-09-16 21:42 PDT (History)
12 users (show)

See Also:


Attachments
test case (1.08 KB, image/svg+xml)
2018-09-04 15:10 PDT, Said Abou-Hallawa
no flags Details
Patch (132.47 KB, patch)
2018-09-10 19:19 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (133.18 KB, patch)
2018-09-11 10:00 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (133.24 KB, patch)
2018-09-13 12:07 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (143.38 KB, patch)
2018-09-14 14:41 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2005-12-06 03:53:02 PST
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.
Comment 1 Eric Seidel 2006-01-26 14:56:30 PST
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.
Comment 2 Gavin Sherlock 2008-08-01 06:05:34 PDT
Looks like:

spreadMethod=pad is supported, but that:

spreadMethod=reflect and
spreadMethod=repeat

are buggy
Comment 3 Dirk Schulze 2009-06-13 22:05:16 PDT
Only CG doesn't support spreadMethods.
Comment 4 Simon Fraser (smfr) 2010-01-30 21:24:56 PST
I have some WIP code in bug 28152 to synthesize repeating gradients. Reflection would not be hard either.
Comment 5 Matt Bishop 2011-02-11 18:18:39 PST
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.
Comment 6 Simon Fraser (smfr) 2017-04-27 21:54:10 PDT
Test at LayoutTests/svg/W3C-SVG-1.1/pservers-grad-10-b.svg
Comment 7 Radar WebKit Bug Importer 2018-09-04 10:28:03 PDT
<rdar://problem/44102127>
Comment 8 Said Abou-Hallawa 2018-09-04 15:10:22 PDT
Created attachment 348854 [details]
test case

Attaching a simpler test case.
Comment 9 Justin Michaud 2018-09-10 19:19:44 PDT
Created attachment 349371 [details]
Patch
Comment 10 Tim Horton 2018-09-10 21:50:55 PDT
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 :) )
Comment 11 Justin Michaud 2018-09-11 10:00:59 PDT
Created attachment 349405 [details]
Patch
Comment 12 Simon Fraser (smfr) 2018-09-11 11:03:46 PDT
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.
Comment 13 Simon Fraser (smfr) 2018-09-11 11:07:22 PDT
Please use the better title I just entered in this bug.
Comment 14 Justin Michaud 2018-09-13 12:07:38 PDT
Created attachment 349688 [details]
Patch
Comment 15 Simon Fraser (smfr) 2018-09-13 14:28:18 PDT
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?
Comment 16 Justin Michaud 2018-09-14 14:41:13 PDT
Created attachment 349809 [details]
Patch
Comment 17 Simon Fraser (smfr) 2018-09-14 16:35:24 PDT
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.
Comment 18 WebKit Commit Bot 2018-09-14 16:55:45 PDT
Comment on attachment 349809 [details]
Patch

Clearing flags on attachment: 349809

Committed r236024: <https://trac.webkit.org/changeset/236024>
Comment 19 WebKit Commit Bot 2018-09-14 16:55:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Darin Adler 2018-09-16 21:42:21 PDT
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.