Bug 32346

Summary: SVG property -webkit-shadow should apply shadow on the result after compositing
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: SVGAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, eric, krit, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
New patch oliver: review+

Description Beth Dakin 2009-12-09 14:21:33 PST
* STEPS TO REPRODUCE
1. create an svg shape with stroke (preferably, thick stroke) and solid fill
2. apply -webkit-shadow on the <svg> container

* RESULTS
Currently, the shadows are applied seperately to the stroke and fill

*EXPECTED RESULTS
The shadow should apply to the composite of the stroke and fill

<rdar://problem/7389404>
Comment 1 Beth Dakin 2009-12-09 14:33:42 PST
Created attachment 44563 [details]
Patch

Patch!
Comment 2 mitz 2009-12-09 17:07:19 PST
Comment on attachment 44563 [details]
Patch

I wonder if this can’t be done more efficiently:
1) Clip to the bounding box (perhaps adjusted to encompass the shadow?) before beginning the transparency layer, to get the layer to have the optimal size (like we do for opacity)
2) If we have both opacity and a shadow, use a single transparency layer for both (perhaps requires multiplying the shadow’s alpha by the opacity?)
Comment 3 Beth Dakin 2009-12-09 19:16:09 PST
Created attachment 44589 [details]
New patch

Here's a new patch that addresses Dan's comments. I clipped the transparency layer to the size of the shadow, but I was not able to use a single transparency layer for both opacity and shadows. When I tried to use a single layer, the color of the shadow (which *was* at the correct alpha level) was visible behind the partially-transparent fill color. This seems wrong since it is not the behavior of box-shadow.
Comment 4 WebKit Review Bot 2009-12-09 19:20:59 PST
style-queue ran check-webkit-style on attachment 44589 [details] without any errors.
Comment 5 Beth Dakin 2009-12-09 22:08:12 PST
Fixed with r51936.
Comment 6 Dirk Schulze 2009-12-11 03:08:55 PST
*** Bug 30930 has been marked as a duplicate of this bug. ***
Comment 7 Dirk Schulze 2009-12-11 04:05:02 PST
Just noticed, that this patch does not fix the shadow problem on Gtk.
Comment 8 Beth Dakin 2009-12-11 11:46:03 PST
In the CG, transparency layers specifically apply to shadows. The CG documentation says:

"After the transparency layer is ended, the result is composited into the context using the global alpha and shadow state of the context. "

My guess is that the same is not true for the GTK-equivalent. We may need a fancier solution there.
Comment 9 Dirk Schulze 2009-12-11 12:47:24 PST
(In reply to comment #8)
> In the CG, transparency layers specifically apply to shadows. The CG
> documentation says:
> 
> "After the transparency layer is ended, the result is composited into the
> context using the global alpha and shadow state of the context. "
> 
> My guess is that the same is not true for the GTK-equivalent. We may need a
> fancier solution there.

On Gtk we draw the shadow just a moment before we have a drawing operation (fillPath, strokePath, drawPath, drawImage, drawLine, ...) independent of the transparency layer.
A partly fix would be to solve bug 6564 and use drawPath to draw the stroke and fill at once. This won't fix shadows on stroked/filled texts and stroked images on SVG.

I think Qt has the same problem and I don't know how the shadow system of Skia works.
Comment 10 Dirk Schulze 2009-12-15 08:59:15 PST
T(In reply to comment #3)
> Created an attachment (id=44589) [details]
> New patch

The patch is responsible for a regression on svg/filters/shadow-on-rect-with-filter.svg.
Comment 11 Beth Dakin 2009-12-15 14:57:48 PST
Oh no! I see it now. The test fails in the pixel test only. Looks like the bounding box is clipping away far too much here. opacity has the same problem actually; in other words, if you replace the shadow style in that test case with "opacity: 0.5," we clip too much in the same way. This deserves a new bug report, especially since it is not unique to this patch or shadows. Filing one now.
Comment 12 Beth Dakin 2009-12-15 15:02:15 PST
Filed https://bugs.webkit.org/show_bug.cgi?id=32584
Comment 13 Eric Seidel (no email) 2009-12-28 22:42:11 PST
Attachment 44589 [details] was posted by a committer and has review+, assigning to Beth Dakin for commit.
Comment 14 Eric Seidel (no email) 2009-12-28 23:32:23 PST
Looks like this was already landed as r51936.