Bug 24687 – Gradient text in canvas not working on chromium/skia.

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Stephen White]

Gradient text in canvas is not working on chromium/skia: the gradient is there, but is not scaled correctly (is solid colour, should be gradient).  See LayoutTest/fast/canvas/canvas-text-alignment.html.

[Stephen White]

Created attachment 28739 [details]
Fix for canvas text gradient problem.

[Dirk Schulze]

> +#if PLATFORM(CG)
>      if (boundingBoxMode() && !isPaintingText) {
> +#else
> +    if (boundingBoxMode()) {
> +#endif

It's not just CG, that uses this code. Please just use 

#if !PLATFORM(SKIA)
    if (boundingBoxMode() && !isPaintingText) {
#else
    if (boundingBoxMode()) {
#endif

if it is necessary. I think you will break some tests of batik. Have you checked this?
And the special skia code, that we have in SVGPaintServer, is just needed for fillAndStrokePath. (btw. this can be fixed by closing the path after filling or stroking inf gc:fillPath() and gc::strokePath()).

I'm still not sure if I get the problem. Gradients don't work for Skia in Canvas, right? And you fix SVG, because the transformation matrix is applied twice for Canvas. Wehre (beside SkiaFontWin.cpp) and why?

To add more platform code to SVG is maybe not the best way and it can break some tests. Can we fix in internally?

[Stephen White]

(In reply to comment #2)
> > +#if PLATFORM(CG)
> >      if (boundingBoxMode() && !isPaintingText) {
> > +#else
> > +    if (boundingBoxMode()) {
> > +#endif
> 
> It's not just CG, that uses this code. Please just use 
> 
> #if !PLATFORM(SKIA)
>     if (boundingBoxMode() && !isPaintingText) {
> #else
>     if (boundingBoxMode()) {
> #endif
> 
> if it is necessary. I think you will break some tests of batik. Have you
> checked this?

There were no regressions in the batik tests on Chromium on Win32 or Webkit on Mac.

I'm pretty sure that those other platforms will need this matrix in order to
do gradient text correctly.  It's only CG that won't need it, since it uses the gradient later in teardown() to fill a rect.  I can change it to be skia-only,
if you still want, but I'm pretty sure you're going to need this matrix for Cairo as well (I'm assuming that's what you're worried about).

> And the special skia code, that we have in SVGPaintServer, is just needed for
> fillAndStrokePath. (btw. this can be fixed by closing the path after filling or
> stroking inf gc:fillPath() and gc::strokePath()).

You're saying that we could remove the skia-specific code in SVGPaintServer?
That sounds good, but is orthogonal to this patch.

> I'm still not sure if I get the problem. Gradients don't work for Skia in
> Canvas, right? And you fix SVG, because the transformation matrix is applied
> twice for Canvas. Wehre (beside SkiaFontWin.cpp) and why?

Basically, the correct way to do gradient text is to scale up the gradient to 
the appropriate size (the bounding box, in this case) before rendering the text.
Canvas was already doing that (m_p0 was 0,0 and m_p1 was 600,600 in this test),
so when I added code in a previous CL to scale up the gradient in SkiaFontWin, it was getting scaled twice.  So I removed my previous code to scale the
gradient (in SkiaFontWin), and allowed the SVG path to set the matrix from 
the bounding box instead.

> To add more platform code to SVG is maybe not the best way and it can break
> some tests. Can we fix in internally?

I don't think there's any other way to fix it, since the lower-level 
rendering code needs that bounding box in order to correctly scale the 
gradient.  And the lower level doesn't know if it has been already set
(as in the canvas case) or unset (as in the SVG case).  The only reason 
CG doesn't need the matrix is that it doesn't use the gradient during 
text rendering; it applies it in teardown().

[Dirk Schulze]

I got it. Yes the current code in SVGPaintServerGradient does only work for CG correctly. Your patch is ok. :-). eseidel? r+?

[Eric Seidel (no email)]

Comment on attachment 28739 [details]
Fix for canvas text gradient problem.

Looks fine.

[Eric Seidel (no email)]

I expect this will break qt/gtk pixel results (if those exist), but I have no easy way to update those, and from what I can tell from Dirk and Stephen this will be a progression for both sets of results.

Also, Stephen, be sure to add the bug URL to the ChangeLog in the future. ;)

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/skia/SkiaFontWin.cpp
	M	WebCore/svg/graphics/SVGPaintServerGradient.cpp
Committed r41860

Thanks again for the patch!