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.
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.
Looks like: spreadMethod=pad is supported, but that: spreadMethod=reflect and spreadMethod=repeat are buggy
Only CG doesn't support spreadMethods.
I have some WIP code in bug 28152 to synthesize repeating gradients. Reflection would not be hard either.
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.
Test at LayoutTests/svg/W3C-SVG-1.1/pservers-grad-10-b.svg
<rdar://problem/44102127>
Created attachment 348854 [details] test case Attaching a simpler test case.
Created attachment 349371 [details] Patch
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 :) )
Created attachment 349405 [details] Patch
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.
Please use the better title I just entered in this bug.
Created attachment 349688 [details] Patch
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?
Created attachment 349809 [details] Patch
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 on attachment 349809 [details] Patch Clearing flags on attachment: 349809 Committed r236024: <https://trac.webkit.org/changeset/236024>
All reviewed patches have been landed. Closing bug.
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.
*** Bug 199569 has been marked as a duplicate of this bug. ***