Bug 12501 – SVG Text fails to respect opacity, fill-opacity and stroke-opacity

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

SVG Text fails to respect opacity, fill-opacity and stroke-opacity

see test case.

[Rob Buis]

Created attachment 14274 [details]
First  attempt

This patch fixes the problem :)
Cheers,

Rob.

[Oliver Hunt]

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

[Eric Seidel (no email)]

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

[Oliver Hunt]

Comment on attachment 14274 [details]
First  attempt

I am suddenly squeamish about this patch...

[Rob Buis]

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.

[Nikolas Zimmermann]

Created attachment 15861 [details]
Initial patch

[Nikolas Zimmermann]

*** Bug 14045 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Created attachment 15862 [details]
LayoutTests

[Oliver Hunt]

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.

[Nikolas Zimmermann]

Created attachment 15886 [details]
Updated patch

[Nikolas Zimmermann]

Created attachment 15887 [details]
Updated LayoutTests

[Oliver Hunt]

Comment on attachment 15886 [details]
Updated patch

r-, i want to get rid of the ugly ass use of void* :D

[Nikolas Zimmermann]

(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

[Nikolas Zimmermann]

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.

[Rob Buis]

Comment on attachment 15898 [details]
Final patch

Looks fine now.

[Nikolas Zimmermann]

Landed in r24988.