Bug 30479 – Add a CSS property that allows shadows to work for SVG content

• Component: SVG
• Status: RESOLVED FIXED

[Beth Dakin]

We want to add a property that is like -webkit-box-shadow for SVG. It should follow the path of the SVG rather than being a box. (Unless the SVG is a box of course!)

<rdar://problem/6942706>

[Beth Dakin]

Created attachment 41361 [details]
Patch

Here is a first-run patch. It adds a property called -webkit-shadow. Maybe we want to re-name that to -webkit-svg-shadow? Not sure.

[Dirk Schulze]

Why do we want this? We have SVG Filters feGaussianBlur and a fast bluring mode for shadows? Isn't it a duplication of functionality?

[Dean Jackson]

I would consider it analogous to the box and text shadow properties in CSS, or the shadow property in the canvas 2d context. Yes, you could use SVG filters, but this seems to be a simple common property that makes sense in CSS.

[Dean Jackson]

Furthermore, the CSS property allows easy authoring of different types of shadows (ie. different offsets, blur radii, etc). With SVG filters this would require multiple filter definitions, each added to the content of the page.

[Dirk Schulze]

Created attachment 41373 [details]
Test for -webkit-box-shadow and SVG Filters

Nevertheless, it would be interesting what css-shadows are doing with the Filter, Masking or Clipping code. Something like <mask style="-webkit-box-shadow" id="mask"> ....

I added An example for Filters as an attachment. This needs a filter enabled build for testing.

[Dirk Schulze]

How does -webkit-box-shadow handles objects in objectBoundingBox mode? Will it scale with increasing the Object size (SVG's can scale width the window-size)?  Or will be a shadow with 3 CSS px radius allways be 3px width, independet of the current size of the object or SVG?

[Nikolas Zimmermann]

I am a bit worried about this patch as it is. I share Dirks opinion that it's duplicating functionality, but I see how it could be useful. Though there need to be way more testcases seeing how it interacts with vital SVG components (masking, clipping, filters, animations, use) etc..

This may take longer than writing the code, I'm aware of that, but we need to be share we don't create problems.

[Beth Dakin]

Dirk, the main reason we wanted to implement a CSS shortcut for this is that we have a feature request for shadows in SVG and the fastest way we could deliver this to them is with this CSS property. We figure filters will take a lot longer to deliver as a solution since it seems it will take a lot of engineering to make them fast. The filters for drop shadow in Firefox right now are pretty chunky, and we assume we'd have the same problem.

Niko, I agree this needs more test cases, but I don't think we need to worry about this patch. It is adding new  opt-in functionality so no existing content will regress. It is common when we add new CSS features that we find refinement-style bugs after the initial implementation is checked in and people start playing with the property.

[Dirk Schulze]

(In reply to comment #8)
> Dirk, the main reason we wanted to implement a CSS shortcut for this is that we
> have a feature request for shadows in SVG and the fastest way we could deliver
> this to them is with this CSS property. We figure filters will take a lot
> longer to deliver as a solution since it seems it will take a lot of
> engineering to make them fast. The filters for drop shadow in Firefox right now
> are pretty chunky, and we assume we'd have the same problem.
> 
> Niko, I agree this needs more test cases, but I don't think we need to worry
> about this patch. It is adding new  opt-in functionality so no existing content
> will regress. It is common when we add new CSS features that we find
> refinement-style bugs after the initial implementation is checked in and people
> start playing with the property.

To add CSS shadows to SVG sounds reasonable to me too now. But I still think that we should take care about some points. Shadows shouldn't be allowed on some SVG elements like mask, clip, clipPath, filter, fe* (maybe as well on the filtered object) and possibly some more. It should work on userSpaceOnUse and objectBoundingBox.

I agree that we won't find all possible problems on implementing a new feature, but it's neccessary to make sure that they won't cause problems on the obvious places. Some simple test cases might be enough.

[Beth Dakin]

Sounds good. I will work on some test cases and post a new patch.

[Eric Seidel (no email)]

I like the idea.  I think it's scary.  And I worry how it will interact with other parts of SVG. :(  I will need to look closer.

So how does this work? Does it work for text elements?  Any arbitrary path?  Does it shadow filtered content?  When is the shadow applied, before or after filtering/masking/clipping?  Likely we're going to need more testing... (which it sounds like Beth is working on).

Neat idea regardless.

[Beth Dakin]

Eric, if you take a look at the patch, you will see it is very simple. Most of the patch is just setting up the CSS property. The part that actually makes it work is just two lined in SVGRenderSupport.cpp. That is literally the only place that we do anything to affect the rendering right now.

[Eric Seidel (no email)]

Crazy what CG gives you for free. :)

 97     if (ShadowData* shadow = svgStyle->shadow())
 98         paintInfo.context->setShadow(IntSize(shadow->x, shadow->y), shadow->blur, shadow->color);

I'll have to stare at it a bit to figure out exactly how the current code will interact with this and if those interactions are what we'd expect.  Thank your the pointer.  In my brief look at the patch before lunch I got a little lost.

[Beth Dakin]

Haha, I know CG is great, and the patch is misleading because you need to do so much just to hook up the CSS side of the property. Anyway, thanks for taking a look. I appreciate your eyes on it because I am very new to the SVG thing. Let me know what you find!

[Dirk Schulze]

(In reply to comment #13)
> Crazy what CG gives you for free. :)
> 
>  97     if (ShadowData* shadow = svgStyle->shadow())
>  98         paintInfo.context->setShadow(IntSize(shadow->x, shadow->y),
> shadow->blur, shadow->color);
> 
> I'll have to stare at it a bit to figure out exactly how the current code will
> interact with this and if those interactions are what we'd expect.  Thank your
> the pointer.  In my brief look at the patch before lunch I got a little lost.

Doesn't Skia do the same?

[Dean Jackson]

I'll have a try at answering various questions.

Eric asked:
> Does it work for text elements?  Any arbitrary path? 

Yes, I believe it should apply.

> Does it shadow filtered content? 

Depends what you mean here. See below.

> When is the shadow applied, before or after filtering/masking/clipping?

I think it should come after fill and stroke, before clipping, masking and filters. 

However, someone asked if the shadow should be included when the object is used as a mask. I think the answer is yes, the same way filters are used.

This is not an issue for clip path, as that is geometry only.

Now, more tricky parts...

What should happen if you shadow a group? Should it act like opacity, where we conceptually render into an offscreen and then composite? I'm pretty sure CG doesn't do that for us (nor any other graphics library used by WebKit). I would like to think that the shadows should work that way though.

About scaling. If the shadow is 3px offset and the object is scaled by 2, then the shadow will appear 6px offset. This is the same as regular SVG transforms on stroke width, and CSS transforms.

How does this sound? I'm talking out of my rear for this because I haven't looked at the code implications in detail.

[Eric Seidel (no email)]

I hope to look at the patch in more detail later today.  Dean's examples at least give us something to test.

[Dirk Schulze]

(In reply to comment #16)
> What should happen if you shadow a group? Should it act like opacity, where we
> conceptually render into an offscreen and then composite? I'm pretty sure CG
> doesn't do that for us (nor any other graphics library used by WebKit). I would
> like to think that the shadows should work that way though.

I also think that WebKit should create a shadow of the group. Does CG render a shadow of a group, if it is placed in a new TransparencyLayer? It would also be possible to draw it into a new ImageBuffer like SVGMaskElement is doing it. But both ways might be expensive.

[Beth Dakin]

Created attachment 41534 [details]
Patch with more tests

Here's a patch with a bunch more tests. I believe I made a test to address each of the concerns that has been expressed.

[Beth Dakin]

Created attachment 41691 [details]
Patch after talking with Dan

Dan and I went over the patch in person, and he caught a few things. Namely, this patch deals with the OwnPtr for the ShadowData much more effectively, and it expressly forbids -webkit-shadow from specifying multiple shadows.

[Beth Dakin]

Committed with r49959.

[Eric Seidel (no email)]

Thank you Dan.

Turns out my (very belated) review submission conflicted with yours, but here are my comments anyway:

Looks like you moved valueForShadow, not sure why.

 70     PassRefPtr<CSSValue> valueForShadow(const ShadowData*, int) const;
int probably needs an arg name.


SVG DocTypes are deprecated these days:
 2 <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Basic//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-basic.dtd">

Bunch of extra newlines in:
svg/css/circle-in-mask-with-shadow.svg

I really need to fix bug 29506 so that we could see your images during review. :(

Looks fine to me too.  Thanks for doing this.

[Beth Dakin]

I moved valueForShadow to make it a member function so it could be used within CSSComputerStyleDeclaration and sub-class SVGCSSComputedStyleDeclaration.

Good to know about the DocTypes.

Thanks Eric!

[Eric Seidel (no email)]

http://jwatt.org/svg/authoring/ has more infos re: the doctypes if you're curious.