Bug 51869 – Shadows are not drawn when filling a rect with a gradient fillStyle on Mac and Chromium-Mac

• Component: Canvas
• Status: RESOLVED FIXED

[Helder Correia]

In CG, a gradient doesn't count as a fill. First, we must fill a rect with a shadow set, then draw the gradient over the fill.

[Helder Correia]

Created attachment 77875 [details]
Patch

[Helder Correia]

This is my very first contact with CG. Please let me know if the fix is not the most correct.

[Simon Fraser (smfr)]

Does this work if some of the gradient stops have alpha?

[Helder Correia]

(In reply to comment #3)
> Does this work if some of the gradient stops have alpha?

Right. Apparently, the shadow is still there but remains solid, which I believe is not the right behavior (at least compared to what we do in Qt). Any pointers?

[Simon Fraser (smfr)]

(In reply to comment #4)
> (In reply to comment #3)
> > Does this work if some of the gradient stops have alpha?
> 
> Right. Apparently, the shadow is still there but remains solid, which I believe is not the right behavior (at least compared to what we do in Qt). Any pointers?

The canvas spec implies that the opacity of the shadow should follow the alpha of the content being drawn.

In my testing, CG does this just fine. Can you attach a testcase that shows the failure?

[Helder Correia]

(In reply to comment #5)
> The canvas spec implies that the opacity of the shadow should follow the alpha of the content being drawn. In my testing, CG does this just fine. Can you attach a testcase that shows the failure?

Go to http://ie.microsoft.com/testdrive/Graphics/CanvasPad/ and click on shadows. Then replace all the code with:

ctx.shadowOffsetX = 40;  
ctx.shadowOffsetY = 40; 
ctx.shadowBlur = 10;  
ctx.shadowColor = "rgba(255, 0, 0, 0.5)";

var gradient = ctx.createLinearGradient(0, 0, 250, 0);
gradient.addColorStop(0, 'rgba(0, 0, 200, 0.3)');
gradient.addColorStop(1, 'rgba(200, 200, 200, 0.9)');

ctx.fillStyle = gradient;
ctx.shadowColor = "rgba(255, 0, 0, 1)";
ctx.fillRect(0, 0, 340, 200);

Finally click to preview. Without the patch, no shadow is shown. With the patch, the shadow does not follow the gradient's alpha. Same happens with the test included in the patch.

[Simon Fraser (smfr)]

Created attachment 77923 [details]
Testcase

[Simon Fraser (smfr)]

The real issue is that GraphicsContext::fillRect(const FloatRect& rect) is clipping to the rect. So the shadow is being drawn, it's just getting clipped to the rect being filled (you can see this if you remove the shadow; the gradient looks lighter).

So you may have to draw the gradient into a transparency layer (clipped), and then render that (unclipped).

[Helder Correia]

Created attachment 77980 [details]
Patch

[Helder Correia]

(In reply to comment #8)
> The real issue is that GraphicsContext::fillRect(const FloatRect& rect) is clipping to the rect.

Indeed. Patch 77980 seems to fix that. I'm not sure if the addition of Gradient::paint(CGContextRef) is the best way to go, but ATM seems like the most convenient to me.

> So you may have to draw the gradient into a transparency layer (clipped), and then render that (unclipped).

I can't seem to clip the layer. If I add CGContextClipToRect(layerContext, rect) after the creation of the layer, the top box will be slightly translated in both axis. I guess I'm missing something possibly obvious.

[Build Bot]

Attachment 77980 [details] did not build on win:
Build output: http://queues.webkit.org/results/7324387

[Simon Fraser (smfr)]

(In reply to comment #10)

This patch looks close.

> I can't seem to clip the layer. If I add CGContextClipToRect(layerContext, rect) after the creation of the layer, the top box will be slightly translated in both axis. I guess I'm missing something possibly obvious.

I'd expect the layer to clip for you.

You should also do a test here where the context has been transformed (e.g. rotated) before the drawing of the gradient.

[Helder Correia]

Created attachment 78073 [details]
Extra test step for rotation; fix Win build (hopefully)

[Simon Fraser (smfr)]

Comment on attachment 78073 [details]
Extra test step for rotation; fix Win build (hopefully)

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

Does this patch alter any of the canvas/ test results?

> WebCore/ChangeLog:10
> +        The solution is to draw the gradient into a transparency layer,

Strictly speaking, a transparency layer is something else. This is just a CGLayer.

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:608
> -        CGContextSaveGState(context);
> -        CGContextClipToRect(context, rect);
> +        CGLayerRef layer = CGLayerCreateWithContext(context, CGSizeMake(rect.width(), rect.height()), 0);
> +        CGContextRef layerContext = CGLayerGetContext(layer);
> +        m_state.fillGradient->paint(layerContext);
>          CGContextConcatCTM(context, m_state.fillGradient->gradientSpaceTransform());
> -        m_state.fillGradient->paint(this);
> -        CGContextRestoreGState(context);
> +        CGContextDrawLayerAtPoint(context, CGPointMake(rect.left(), rect.top()), layer);
> +        CGLayerRelease(layer);

You should only create the CGLayerRef if you know it should render with a shadow.

[Helder Correia]

Created attachment 78088 [details]
No layer if no shadow required; update new test expected

[Helder Correia]

(In reply to comment #14)
> Does this patch alter any of the canvas/ test results?

Only the three Philip tests that it solves :)

> You should only create the CGLayerRef if you know it should render with a shadow.

Of course, added in patch 78088.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 78088 [details]:

http/tests/xmlhttprequest/cookies.html bug 51976 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 78088 [details]
No layer if no shadow required; update new test expected

Clearing flags on attachment: 78088

Committed r75139: <http://trac.webkit.org/changeset/75139>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Patrick R. Gansterer]

Comment on attachment 78088 [details]
No layer if no shadow required; update new test expected

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

> WebCore/platform/graphics/Gradient.h:40
> +#if PLATFORM(WIN)
> +#include <CoreGraphics/CoreGraphics.h>
> +#endif

Why is this PLATFORM(WIN) and not PLATFOMR(CG)?

FYI: This broke WinCE: http://build.webkit.org/builders/WinCE%20Release%20%28Build%29/builds/169

[Helder Correia]

(In reply to comment #20)
> Why is this PLATFORM(WIN) and not PLATFOMR(CG)? FYI: This broke WinCE: 

Sorry for that. IIUC, you've already fixed it, thank you.

[Dirk Schulze]

(In reply to comment #21)
> (In reply to comment #20)
> > Why is this PLATFORM(WIN) and not PLATFOMR(CG)? FYI: This broke WinCE: 
> 
> Sorry for that. IIUC, you've already fixed it, thank you.

Causes a regression with SVG texts and gradients. See bug 52531.

[Dirk Schulze]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > Why is this PLATFORM(WIN) and not PLATFOMR(CG)? FYI: This broke WinCE: 
> > 
> > Sorry for that. IIUC, you've already fixed it, thank you.
> 
> Causes a regression with SVG texts and gradients. See bug 52531.

It would be interesting as well, how your patch works with SVG texts, filled with gradient and CSS shadow. 

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" >
<defs>
<linearGradient id="gradient" x1="0%" y1="0%" x2="100%" y2="0%">
<stop offset="0%" stop-color="red"/>
<stop offset="100%" stop-color="blue"/>
</linearGradient>
</defs>
<text x="200" y="200" style="font-size: 200px; text-shadow: #000 20px 20px 20px;" fill="url(#gradient)">SVG</text>
</svg>

Can you add a test case with pixel test please?

[Helder Correia]

(In reply to comment #23)
> Can you add a test case with pixel test please?

Done in bug 52509.

BTW, I'd like to confirm with you: with Qt the shadow appears in black shades (following the gradient). In CG, the shadow has the same colors of the gradient. Isn't the former the correct behavior?