Bug 26102 – [CAIRO] CSS3 -webkit-box-shadow support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[adel]

webkit gtk+ doesn't support CSS3 -webkit-box-shadow

[Brent Fulgham]

This bug is also present in the WinCairo build.

[Dirk Schulze]

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.

[Dirk Schulze]

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.

[Dirk Schulze]

Created attachment 41780 [details]
Canvas shadow support for fill, stroke, drawImage

also added support for drawImage

[Brent Fulgham]

These changes work very nicely -- great work!  Any hope of some box-shadow support?

[Dirk Schulze]

I broke the Canvas/SVG shadow code up to bug 30960

[Brent Fulgham]

See http://webkit.org/blog/86/box-shadow/ for an example of what this should look like.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 43262 [details]
Patch enabling webkit-box-shadow support.

[Dirk Schulze]

(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 :-(

[Dirk Schulze]

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

[Dirk Schulze]

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

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 43317 [details]
Minor revision, now all shadow effects work.

Revised patch that now correctly supports rounded rects/ellipse shadow.

[Dirk Schulze]

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

[Alexey Proskuryakov]

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.

[Brent Fulgham]

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

[Brent Fulgham]

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

[Alexey Proskuryakov]

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

[Brent Fulgham]

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