Bug 12501 - SVG Text fails to respect opacity, fill-opacity and stroke-opacity
Summary: SVG Text fails to respect opacity, fill-opacity and stroke-opacity
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL: http://www.w3.org/Graphics/SVG/Test/2...
Keywords:
: 14045 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-01-31 04:36 PST by Eric Seidel (no email)
Modified: 2007-08-10 06:26 PDT (History)
3 users (show)

See Also:


Attachments
First attempt (2.20 KB, patch)
2007-04-30 10:25 PDT, Rob Buis
oliver: review-
Details | Formatted Diff | Diff
Work in progress (6.62 KB, patch)
2007-05-01 02:54 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Initial patch (12.92 KB, patch)
2007-08-07 19:16 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
LayoutTests (19.44 KB, patch)
2007-08-07 19:18 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (25.85 KB, patch)
2007-08-09 11:35 PDT, Nikolas Zimmermann
oliver: review-
Details | Formatted Diff | Diff
Updated LayoutTests (26.02 KB, patch)
2007-08-09 11:36 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch (53.63 KB, patch)
2007-08-10 05:41 PDT, Nikolas Zimmermann
rwlbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.