Bug 26102

Summary: [CAIRO] CSS3 -webkit-box-shadow support
Product: WebKit Reporter: adel <netdur>
Component: WebKitGTKAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, krit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 30960    
Bug Blocks:    
Attachments:
Description Flags
first test for shadow support in Canvas
none
Canvas shadow with fill and stroke support
none
Canvas shadow support for fill, stroke, drawImage
none
Patch enabling webkit-box-shadow support.
none
Updated webkit-box-shadow support
none
Minor revision, now all shadow effects work. oliver: review+

Description adel 2009-05-30 19:56:59 PDT
webkit gtk+ doesn't support CSS3 -webkit-box-shadow
Comment 1 Brent Fulgham 2009-10-14 12:54:43 PDT
This bug is also present in the WinCairo build.
Comment 2 Dirk Schulze 2009-10-19 11:47:44 PDT
Created attachment 41436 [details]
first test for shadow support in Canvas

This is the first patch for shadows on fill() anf fillRect() in Canvas. This is just for testing and not for review. It still needs some impovement as well as clean-up. stroke() and CSS shadows will follow too.
Comment 3 Dirk Schulze 2009-10-24 02:36:03 PDT
Created attachment 41779 [details]
Canvas shadow with fill and stroke support

This patch is more efficient and supports shadows for Canvas fill and stroke operations. It is also possible to use shadows for paths filled/stroked with gradients or patterns now. I deactivated CSS shadow support, since it needs more work.
Comment 4 Dirk Schulze 2009-10-24 04:36:07 PDT
Created attachment 41780 [details]
Canvas shadow support for fill, stroke, drawImage

also added support for drawImage
Comment 5 Brent Fulgham 2009-10-25 20:04:42 PDT
These changes work very nicely -- great work!  Any hope of some box-shadow support?
Comment 6 Dirk Schulze 2009-10-30 12:58:31 PDT
I broke the Canvas/SVG shadow code up to bug 30960
Comment 7 Brent Fulgham 2009-11-13 17:17:58 PST
See http://webkit.org/blog/86/box-shadow/ for an example of what this should look like.
Comment 8 Brent Fulgham 2009-11-15 17:57:43 PST
The reason the earlier box-shadow logic did not work is that the calculation of the clip-out rectangles was incorrect.  Note the use of cairo_rectangle; the Y dimension had been improperly coded as x2, which caused the clip region to be wrong, and therefore the areas of the path that were meant to be masked out were left, while areas we wanted drawn were clipped out.

===================================================================
--- GraphicsContextCairo.cpp    (revision 50998)
+++ GraphicsContextCairo.cpp    (working copy)
@@ -1113,7 +1155,7 @@
     cairo_t* cr = m_data->cr;
     double x1, y1, x2, y2;
     cairo_clip_extents(cr, &x1, &y1, &x2, &y2);
-    cairo_rectangle(cr, x1, x2, x2 - x1, y2 - y1);
+    cairo_rectangle(cr, x1, y1, x2 - x1, y2 - y1);
     cairo_rectangle(cr, r.x(), r.y(), r.width(), r.height());
     cairo_fill_rule_t savedFillRule = cairo_get_fill_rule(cr);
     cairo_set_fill_rule(cr, CAIRO_FILL_RULE_EVEN_ODD);

Ful patch to follow.
Comment 9 Brent Fulgham 2009-11-15 21:59:05 PST
Created attachment 43262 [details]
Patch enabling webkit-box-shadow support.
Comment 10 Dirk Schulze 2009-11-15 23:51:36 PST
(In reply to comment #9)
> Created an attachment (id=43262) [details]
> Patch enabling webkit-box-shadow support.

Realy good catch with the clipping area. Just a sytling issue: You have an unused variable 'cr' at L588. This causes a warning adn you added a extra free line at L213. Can be fixed on landing. I would say r=me, but I'm not a reviewer :-(
Comment 11 Dirk Schulze 2009-11-16 00:43:38 PST
(In reply to comment #10)
> Realy good catch with the clipping area. Just a sytling issue: You have an
> unused variable 'cr' at L588. This causes a warning adn you added a extra free
> line at L213. Can be fixed on landing. I would say r=me, but I'm not a reviewer
> :-(

Hm. After reading the patch again, I found a minor problem. If the kernelSize is zero, you have to draw the shadow manualy with a rect and a solid color. kernelSizes with a value of zero, stop the rendering process on feGaussianBlur, according to the Spec of SVG. I handle this case on on createPlatformShadow(). Also it's a good idea to limit the kernelSize (see createPlatformShadow() again).
Comment 12 Dirk Schulze 2009-11-16 04:27:27 PST
(In reply to comment #11)
Sorry for comment 9. It's still a great patch. But there are more issues. You can't use source->setIsAlphaImage(true); in your code, because it won't blur the color's, just the alpha channel. That means you only get the right blurred image, on using black as shadow color. You either blur every channel  (very slow), or use masking with the shadow color, like we do on createPlatformShadow. The last solutions is faster, and drawPathShadow is doing it. You just need to replace the call

+    RefPtr<FilterEffect> blur = createShadowFilterEffect (shadowBuffer.release(), shadowBufferSize, kernelSize);
+
+    //draw the filter result to the context
+    context->drawImage(blur->resultImage()->image(), shadowRect);

with

    context->createPlatformShadow(shadowBuffer.release(), shadowColor, shadowRect, kernelSize);

It's also neccessary to give drawBorderlessRectShadow the current color of fillRect. This is needed to calculate the correct transparency of the shadow. And don't forget to multiply the alpha channel of the current color with the one of shadow color and set it as the new transparency level of shadow color.

It's not necessary to move the Filter creation out of createPlatformShadow().

The use of createPlatformShadow() in drawBorderlessRectShadow() will solve the issues of my previous comment. I hope i did not forget something.
Comment 13 Brent Fulgham 2009-11-16 11:20:31 PST
Created attachment 43316 [details]
Updated webkit-box-shadow support

Patch, revised per Dirk's suggestions.  Patch now properly handles rectangular shadow areas in the "border-radius-big.html" test.
Comment 14 Brent Fulgham 2009-11-16 12:11:32 PST
Created attachment 43317 [details]
Minor revision, now all shadow effects work.

Revised patch that now correctly supports rounded rects/ellipse shadow.
Comment 15 Dirk Schulze 2009-11-16 12:31:18 PST
(In reply to comment #14)
> Created an attachment (id=43317) [details]
> Minor revision, now all shadow effects work.
> 
> Revised patch that now correctly supports rounded rects/ellipse shadow.

Looks great now. Please delete cairo_t* cr = context->platformContext(); before landing. Still need some one who reviews the patch.
Comment 16 Alexey Proskuryakov 2009-11-16 12:44:21 PST
Comment on attachment 43317 [details]
Minor revision, now all shadow effects work.

> +static inline void drawBorderlessRectShadow(GraphicsContext* context, const FloatRect& rect, const Color& rectColor)

Seems surprising that this large function gets inlined.

> +    //calculate filter values

This is not how we write comments - there should be a space after "//", and the comment should be a full sentence starting with a capital letter, and ending with a period.

> +    float kernelSize (0.0);

float kernelSize = 0;

> -    cairo_rectangle(cr, x1, x2, x2 - x1, y2 - y1);
> +    cairo_rectangle(cr, x1, y1, x2 - x1, y2 - y1);

Seems worth its own ChangeLog explanation. Does this only affect shadows? If not, do we need a new test for a bug fixed here?

r=me, please consider these comments.
Comment 17 Brent Fulgham 2009-11-16 13:04:38 PST
(In reply to comment #16)
> (From update of attachment 43317 [details])
> > +static inline void drawBorderlessRectShadow(GraphicsContext* context, const FloatRect& rect, const Color& rectColor)
> 
> Seems surprising that this large function gets inlined.

Fixed
 
> > +    //calculate filter values
> 
> This is not how we write comments - there should be a space after "//", and the
> comment should be a full sentence starting with a capital letter, and ending
> with a period.

Sorry - I committed after olliej's review and missed this.  I'll try to correct it during a future change.
 
> > +    float kernelSize (0.0);
> 
> float kernelSize = 0;

Fixed

> > -    cairo_rectangle(cr, x1, x2, x2 - x1, y2 - y1);
> > +    cairo_rectangle(cr, x1, y1, x2 - x1, y2 - y1);
> 
> Seems worth its own ChangeLog explanation. Does this only affect shadows? If
> not, do we need a new test for a bug fixed here?

See Comment #8 for details.  This was a bug fix, which was identified due to the new drawing produced by the shadow effects.

> r=me, please consider these comments.
Comment 18 Brent Fulgham 2009-11-16 13:05:31 PST
Landed in http://trac.webkit.org/changeset/51046.
Comment 19 Alexey Proskuryakov 2009-11-16 15:07:51 PST
> > float kernelSize = 0;
>
> Fixed

In committed code, it's "float kernelSize = 0.0", which is actually different from what I suggested. Zero is magic in C++, as its can be casted to any type. On the other hand, 0.0 is double, which gets converted to a float here. Some compilers may warn about this.
Comment 20 Brent Fulgham 2009-11-17 17:17:06 PST
(In reply to comment #19)
> > > float kernelSize = 0;
> >
> > Fixed
> 
> In committed code, it's "float kernelSize = 0.0", which is actually different
> from what I suggested. Zero is magic in C++, as its can be casted to any type.
> On the other hand, 0.0 is double, which gets converted to a float here. Some
> compilers may warn about this.

Fixed.  I found another place where this same mistake was made and corrected it, too.

Also corrected the comments in the routines where these assignments were being made.

Landed in http://trac.webkit.org/changeset/51098.