Bug 12501

Summary: SVG Text fails to respect opacity, fill-opacity and stroke-opacity
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: ml, oliver, rindahl
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.w3.org/Graphics/SVG/Test/20061213/htmlEmbedHarness/full-text-text-08-b.html
Attachments:
Description Flags
First attempt
oliver: review-
Work in progress
none
Initial patch
none
LayoutTests
none
Updated patch
oliver: review-
Updated LayoutTests
none
Final patch rwlbuis: review+

Description Eric Seidel (no email) 2007-01-31 04:36:51 PST
SVG Text fails to respect opacity, fill-opacity and stroke-opacity

see test case.
Comment 1 Rob Buis 2007-04-30 10:25:11 PDT
Created attachment 14274 [details]
First  attempt

This patch fixes the problem :)
Cheers,

Rob.
Comment 2 Oliver Hunt 2007-04-30 17:43:31 PDT
Comment on attachment 14274 [details]
First  attempt

I'd almost prefer a declaration

CGFloat opacity = style->svgStyle()->strokeOpacity();
colorComponents[3] = opacity;
....
Color(float(colorComponents[0]) * 255., float(colorComponents[1]) * 255., float(colorComponents[2]) * 255., float(opacity) * 255.0f)

I think it makes it more clear that you're deliberately ignoring the alpha channel specified by the style.

Also you have float(colorComponents[3] * 255.) which should be float(colorComponents[3]) * 255. to match the other cases

255. should be 255.0f to prevent any dumb usages of double

otherwise r=me
Comment 3 Eric Seidel (no email) 2007-04-30 18:47:32 PDT
This looks like something which should be turned into a Color constructor or a static function:

Color(float(colorComponents[0]) * 255., float(colorComponents[1]) * 255.,
float(colorComponents[2]) * 255., float(opacity) * 255.0f)

colorFromFloatComponents(colorComponents)

or
Color(colorComponents)

or
Color(colorComponents[0], colorComponents[1], colorComponents[2], 0)

Or similar.  Something to prevent forgetting a 255 somewhere or mixing up channels, as I've been known to do in the past in that code...
Comment 4 Oliver Hunt 2007-05-01 02:49:07 PDT
Comment on attachment 14274 [details]
First  attempt

I am suddenly squeamish about this patch...
Comment 5 Rob Buis 2007-05-01 02:54:13 PDT
Created attachment 14290 [details]
Work in progress

I noticed gradients and patterns were failing in this regard too, so here is the fix. Oliver and myself agreed to wait with this until Niko's big "textPath" patch before looking at this situation again.
Cheers,

Rob.
Comment 6 Nikolas Zimmermann 2007-08-07 19:16:50 PDT
Created attachment 15861 [details]
Initial patch
Comment 7 Nikolas Zimmermann 2007-08-07 19:17:15 PDT
*** Bug 14045 has been marked as a duplicate of this bug. ***
Comment 8 Nikolas Zimmermann 2007-08-07 19:18:44 PDT
Created attachment 15862 [details]
LayoutTests
Comment 9 Oliver Hunt 2007-08-07 19:27:38 PDT
Comment on attachment 15861 [details]
Initial patch

+        if (opacity < 1.0f)
+            childPaintInfo.context->endTransparencyLayer();
+
 #if ENABLE(SVG_EXPERIMENTAL_FEATURES)
         if (filter)
             filter->applyFilter(childPaintInfo.context, boundingBox);
 #endif
-
-        if (opacity < 1.0f)
-            childPaintInfo.context->endTransparencyLayer();

seems wrong, as you end up applying the filter to the result of the opacity blend, rather than blending the result of the filter.
Comment 10 Nikolas Zimmermann 2007-08-09 11:35:47 PDT
Created attachment 15886 [details]
Updated patch
Comment 11 Nikolas Zimmermann 2007-08-09 11:36:11 PDT
Created attachment 15887 [details]
Updated LayoutTests
Comment 12 Oliver Hunt 2007-08-09 14:22:33 PDT
Comment on attachment 15886 [details]
Updated patch

r-, i want to get rid of the ugly ass use of void* :D
Comment 13 Nikolas Zimmermann 2007-08-10 05:20:57 PDT
(In reply to comment #12)
> (From update of attachment 15886 [details] [edit])
> r-, i want to get rid of the ugly ass use of void* :D
Fixed, as discussed on IRC.

Greetings,
Niko
Comment 14 Nikolas Zimmermann 2007-08-10 05:41:11 PDT
Created attachment 15898 [details]
Final patch

Only change to the already reviewed patch, is remove the void* usage for filter in non-experimental-feature-builds.
Comment 15 Rob Buis 2007-08-10 06:22:17 PDT
Comment on attachment 15898 [details]
Final patch

Looks fine now.
Comment 16 Nikolas Zimmermann 2007-08-10 06:26:58 PDT
Landed in r24988.