Bug 51869

Created attachment 77875 [details] Patch

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

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

(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?

(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?

(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.

Created attachment 77923 [details] Testcase

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).

Created attachment 77980 [details] Patch

(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.

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

(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.

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

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.

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

(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.

The commit-queue encountered the following flaky tests while processing attachment 78088 [details]: http/tests/xmlhttprequest/cookies.html bug 51976 (authors: ap@webkit.org and eric@webkit.org) The commit-queue is continuing to process your patch.

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>

All reviewed patches have been landed. Closing bug.

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

(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.

(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.

(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?

(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?