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+

adel
Reported 2009-05-30 19:56:59 PDT
webkit gtk+ doesn't support CSS3 -webkit-box-shadow
Attachments
first test for shadow support in Canvas (9.77 KB, patch)
2009-10-19 11:47 PDT, Dirk Schulze
no flags
Canvas shadow with fill and stroke support (11.22 KB, patch)
2009-10-24 02:36 PDT, Dirk Schulze
no flags
Canvas shadow support for fill, stroke, drawImage (13.59 KB, patch)
2009-10-24 04:36 PDT, Dirk Schulze
no flags
Patch enabling webkit-box-shadow support. (4.86 KB, patch)
2009-11-15 21:59 PST, Brent Fulgham
no flags
Updated webkit-box-shadow support (2.71 KB, patch)
2009-11-16 11:20 PST, Brent Fulgham
no flags
Minor revision, now all shadow effects work. (3.00 KB, patch)
2009-11-16 12:11 PST, Brent Fulgham
oliver: review+
Brent Fulgham
Comment 1 2009-10-14 12:54:43 PDT
This bug is also present in the WinCairo build.
Dirk Schulze
Comment 2 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.
Dirk Schulze
Comment 3 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.
Dirk Schulze
Comment 4 2009-10-24 04:36:07 PDT
Created attachment 41780 [details] Canvas shadow support for fill, stroke, drawImage also added support for drawImage
Brent Fulgham
Comment 5 2009-10-25 20:04:42 PDT
These changes work very nicely -- great work! Any hope of some box-shadow support?
Dirk Schulze
Comment 6 2009-10-30 12:58:31 PDT
I broke the Canvas/SVG shadow code up to bug 30960
Brent Fulgham
Comment 7 2009-11-13 17:17:58 PST
See http://webkit.org/blog/86/box-shadow/ for an example of what this should look like.
Brent Fulgham
Comment 8 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.
Brent Fulgham
Comment 9 2009-11-15 21:59:05 PST
Created attachment 43262 [details] Patch enabling webkit-box-shadow support.
Dirk Schulze
Comment 10 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 :-(
Dirk Schulze
Comment 11 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).
Dirk Schulze
Comment 12 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.
Brent Fulgham
Comment 13 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.
Brent Fulgham
Comment 14 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.
Dirk Schulze
Comment 15 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.
Alexey Proskuryakov
Comment 16 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.
Brent Fulgham
Comment 17 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.
Brent Fulgham
Comment 18 2009-11-16 13:05:31 PST
Alexey Proskuryakov
Comment 19 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.
Brent Fulgham
Comment 20 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.
Note You need to log in before you can comment on or make changes to this bug.