Bug 52531 - REGRESSION(75139): SVG gradients are not applied to texts
Summary: REGRESSION(75139): SVG gradients are not applied to texts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 52724
Blocks: 51869 52640
  Show dependency treegraph
 
Reported: 2011-01-16 03:00 PST by Dirk Schulze
Modified: 2011-01-25 10:54 PST (History)
4 users (show)

See Also:


Attachments
Patch (111.33 KB, patch)
2011-01-18 01:27 PST, Helder Correia
no flags Details | Formatted Diff | Diff
context -> layerContext. This fixes the SVG text gradient fill. (1.86 KB, patch)
2011-01-24 23:16 PST, Helder Correia
no flags Details | Formatted Diff | Diff
Translate layerContext CTM - make pixel test succeed again (2.08 KB, patch)
2011-01-24 23:35 PST, Helder Correia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-01-16 03:00:07 PST
The change on GraphcisContextCG in http://trac.webkit.org/changeset/75139 caused a problem with filling and stroking texts with gradients in SVG. The order of concatCTM and clipRect was mixed up.
Comment 1 Helder Correia 2011-01-18 01:27:01 PST
Created attachment 79253 [details]
Patch
Comment 2 Dirk Schulze 2011-01-18 01:32:37 PST
Comment on attachment 79253 [details]
Patch

Hm. the question is, why is the shadow not black? Do you see similar behaviors on Canvas rect with shadow? Or Canvas texts and shadow?  (filled with a gradient of course).
Comment 3 Helder Correia 2011-01-18 01:59:30 PST
(In reply to comment #2)
> (From update of attachment 79253 [details])
> Hm. the question is, why is the shadow not black? Do you see similar behaviors on Canvas rect with shadow? Or Canvas texts and shadow?  (filled with a gradient of course).

Looks like I asked the same thing at the same time on https://bugs.webkit.org/show_bug.cgi?id=51869#c24 :)

On canvas, the shadow is black for rect and text (or whatever color is set to it). The non-black SVG text-shadow is the behavior I get if I go back some revisions. Same in Google Chrome Dev Channel on Mac. So, this is not something that was broken recently.
Comment 4 Dirk Schulze 2011-01-18 02:33:36 PST
Comment on attachment 79253 [details]
Patch

I trust you, if you say it was wrong before you committed your patch. r=me.
Comment 5 Nikolas Zimmermann 2011-01-18 07:28:42 PST
I tested this patch on Leopard, it fixes _most_ issues.
But svg/css/composite-shadow-text.svg remains broken, the first gradient doesn't show up.

Gradient on text remains broken, when a shadow is also applied to the target.
<rect fill="url(#someGradient" style="-webkit-svg-shadow: ..../">
Comment 6 WebKit Commit Bot 2011-01-18 09:19:34 PST
Comment on attachment 79253 [details]
Patch

Clearing flags on attachment: 79253

Committed r76029: <http://trac.webkit.org/changeset/76029>
Comment 7 WebKit Commit Bot 2011-01-18 09:19:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Dirk Schulze 2011-01-18 10:10:19 PST
(In reply to comment #5)
> I tested this patch on Leopard, it fixes _most_ issues.
> But svg/css/composite-shadow-text.svg remains broken, the first gradient doesn't show up.
> 
> Gradient on text remains broken, when a shadow is also applied to the target.
> <rect fill="url(#someGradient" style="-webkit-svg-shadow: ..../">

I feared it. I have hoped that SVG Text + text-shadow covers it :-( Niko, can you check if removing the if and it's content makes it run again?
Comment 9 Simon Fraser (smfr) 2011-01-18 10:24:47 PST
(In reply to comment #3)

> On canvas, the shadow is black for rect and text (or whatever color is set to it). The non-black SVG text-shadow is the behavior I get if I go back some revisions. Same in Google Chrome Dev Channel on Mac. So, this is not something that was broken recently.

But when was it broken?
Comment 10 Dirk Schulze 2011-01-18 10:40:47 PST
(In reply to comment #9)
> (In reply to comment #3)
> 
> > On canvas, the shadow is black for rect and text (or whatever color is set to it). The non-black SVG text-shadow is the behavior I get if I go back some revisions. Same in Google Chrome Dev Channel on Mac. So, this is not something that was broken recently.
> 
> But when was it broken?

We never had test cases for SVG texts, filled with a gradient together with a text shadow. All test cases were with solid colors and they still run. It's likely that it never worked. Needs to be checked.
Comment 11 Nikolas Zimmermann 2011-01-19 01:46:10 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #3)
> > 
> > > On canvas, the shadow is black for rect and text (or whatever color is set to it). The non-black SVG text-shadow is the behavior I get if I go back some revisions. Same in Google Chrome Dev Channel on Mac. So, this is not something that was broken recently.
> > 
> > But when was it broken?
> 
> We never had test cases for SVG texts, filled with a gradient together with a text shadow. All test cases were with solid colors and they still run. It's likely that it never worked. Needs to be checked.

Well we have svg/css/composite-shadow-text.svg, added recently which covers svg gradient + -webkit-svg-shadow, that is broken atm.
Comment 12 Dirk Schulze 2011-01-19 10:51:32 PST
Helder, do you plan to fix it, or should we roll out the patch?
Comment 13 Helder Correia 2011-01-19 17:36:14 PST
(In reply to comment #12)
> Helder, do you plan to fix it, or should we roll out the patch?

Of course, we need composite-shadow-text.svg to work again. I'll look at it asap. Any hints are more than welcome, since I've never touched SVG code and I'm new to CG.
Comment 14 Helder Correia 2011-01-24 23:16:32 PST
Created attachment 80025 [details]
context -> layerContext. This fixes the SVG text gradient fill.
Comment 15 Helder Correia 2011-01-24 23:35:24 PST
Created attachment 80026 [details]
Translate layerContext CTM - make pixel test succeed again
Comment 16 Dirk Schulze 2011-01-25 00:14:20 PST
Comment on attachment 80026 [details]
Translate layerContext CTM - make pixel test succeed again

Makes sense. r=me
Comment 17 Helder Correia 2011-01-25 10:33:27 PST
(In reply to comment #16)
> (From update of attachment 80026 [details])
> Makes sense. r=me

Thanks. Can you please reopen the bug (and possibly reapply R+) so that the patch gets sent to the CQ? Or maybe land it manually.
Comment 18 Dirk Schulze 2011-01-25 10:37:56 PST
reopening for landing the patch.
Comment 19 WebKit Commit Bot 2011-01-25 10:54:37 PST
Comment on attachment 80026 [details]
Translate layerContext CTM - make pixel test succeed again

Clearing flags on attachment: 80026

Committed r76612: <http://trac.webkit.org/changeset/76612>
Comment 20 WebKit Commit Bot 2011-01-25 10:54:43 PST
All reviewed patches have been landed.  Closing bug.