Bug 9197

Summary: CSS3: Borders with border-radius and double, groove, or ridge styles should look better
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: CSSAssignee: Alex Taylor <darwin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andri, bdakin, darwin, dglazkov, eric, gustavo, ian, joost, Justin, mitz, mjs, ojan, rik, simon.fraser, vikingjs, webkit-ews, webkit.org, webkit.review.bot, xan.lopez
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.w3.org/TR/2005/WD-css3-background-20050216/#the-border-radius
Bug Depends on:    
Bug Blocks: 87931    
Attachments:
Description Flags
Testcase
none
Rounded border rendering improvements
none
Non-uniform stroke thickness improvements
none
Non-uniform stroke thickness example (after and before)
none
An interactive test case
none
Image explaining the strategy for radius drawing
none
Re-written border code using clipping patches
eric: review-
Updated, re-factored version of Alex's patch
none
New version of the patch
none
Patch addresses some comments
none
Patch the won't break other platforms
sam: review+
On the left is border-radius (plus a few other things) in Firefox 4, the right shows border-radius not quite rendering right in Webkit 534.28 none

Description Beth Dakin 2006-05-31 14:33:21 PDT
The border arcs for double, groove, and ridge styles should be implemented differently. We should pretend like we're drawing two solid borders, so do the outer/inner clip for each line separately. These styles will probably look much nicer if they are implemented this way, particularly with smaller border widths. There are existing layout tests covering these styles in fast/borders.
Comment 1 Beth Dakin 2006-05-31 14:40:37 PDT
This is a followup bug for http://bugzilla.opendarwin.org/show_bug.cgi?id=6755
Comment 2 Joost de Valk (AlthA) 2006-05-31 22:26:40 PDT
I'll create an uber testcase for all variants of border-radius.
Comment 3 Joost de Valk (AlthA) 2006-06-01 00:48:28 PDT
Created attachment 8633 [details]
Testcase

This should help in fixing it :)
Comment 4 Alexander Kempgen 2007-02-13 09:35:33 PST
additionally the border arcs for dotted and dashed of thin borders look very different than the straight borders. 1px dotted even looks like a solid border...
Comment 5 mitz 2007-05-21 16:23:44 PDT
Created attachment 14645 [details]
Rounded border rendering improvements

I came across several pathological cases in the existing code while testing this patch, which currently adds at least one more pathology.
Comment 6 mitz 2007-05-22 03:27:29 PDT
Created attachment 14651 [details]
Non-uniform stroke thickness improvements

This version varies the stroke thickness along arcs joining borders of different widths.
Comment 7 mitz 2007-05-22 03:29:28 PDT
Created attachment 14652 [details]
Non-uniform stroke thickness example (after and before)

Rendering of arcs between borders with different widths, with attachment 14651 [details] (left) and with the current code (right).
Comment 8 mitz 2007-05-24 09:52:22 PDT
Those last two patches are actually wrong for all but the uniform stroke and equal radii case, since in other cases the outside of the curve is not exactly an ellipse. I think those cases will have to be done using a clipping path (which I tried to avoid in the patches).
Comment 9 Eric Seidel (no email) 2007-09-30 12:24:52 PDT
Looks like this was another casualty to leopard stabilization. ;)
Comment 10 mitz 2008-02-21 07:08:25 PST
See also bug 17468.
Comment 11 Alex Taylor 2008-06-19 17:19:03 PDT
(In reply to comment #8)
> Those last two patches are actually wrong for all but the uniform stroke and
> equal radii case, since in other cases the outside of the curve is not exactly
> an ellipse. I think those cases will have to be done using a clipping path
> (which I tried to avoid in the patches).
> 

That statement is a little ambiguous to me, are you saying that using this patch, the outside of the curve does not draw as an ellipse (and it should). Or are you saying that with this patch, the outside of the curve follows an ellipse (and it shouldn't).

I have a patch which is very close to this but instead I kept the Inner Clipping Rectangle in there, is that what you're hinting should be done or is there another equation for the corners that probably should be followed instead of clipping to rounded rectangles?
Comment 12 mitz 2008-06-19 17:54:30 PDT
(In reply to comment #11)
> That statement is a little ambiguous to me, are you saying that using this
> patch, the outside of the curve does not draw as an ellipse (and it should). Or
> are you saying that with this patch, the outside of the curve follows an
> ellipse (and it shouldn't).

Sorry about the ambiguity. I meant the former.

> I have a patch which is very close to this but instead I kept the Inner
> Clipping Rectangle in there, is that what you're hinting should be done or is
> there another equation for the corners that probably should be followed instead
> of clipping to rounded rectangles?

It has been a while since I worked on this bug, but I think that with Core Graphics at least, one would have to clip to a rounded rectangle (that is, the correct shape cannot be achieved by just stroking a path in the non-uniform case).
Comment 13 mitz 2008-06-20 00:47:38 PDT
Created attachment 21853 [details]
An interactive test case

A test file I used when I worked on this bug.
Comment 14 Alex Taylor 2008-06-20 01:08:43 PDT
Oh wow, awesome test case you had there. Shows some massive problems with border-radius.

When drawing rounded borders, an outer rounded clipping region is applied always.
Also currently, in cases where it needs to, an inner rounded clipping region is also applied.

You last patch removes the inner clipping region in all cases explicitly but you're saying it is required? 
Those clipping regions perform elliptically at the corners so are they what you're saying is required? 
If so, what was the reasoning for removing them in the patch? 

I'm interested in getting this all fixed up so any pointers would be appreciated.

(The double border case would probably need additional clipping regions than what is currently there to cater for the gap between the border strokes)
Comment 15 Alex Taylor 2008-06-20 16:47:48 PDT
As a correction, I can see why you removed the clipping regions. They only clip circles, not other ellipses. I got confused as the patch I had done added elliptical support on those clipping regions. 

Sorry about that.
Comment 16 Henri Sivonen 2009-01-14 00:13:44 PST
Has the patch on this bug fallen through the cracks? The latest nightly still doesn't anti-alias double and dotted curves properly and rounds the width of the gap in double border badly.
Comment 17 Simon Fraser (smfr) 2009-05-15 23:27:08 PDT
Would be nice to get these patches in.
Comment 18 mitz 2009-05-15 23:49:02 PDT
(In reply to comment #17)
> Would be nice to get these patches in.

Would be nice to fix this bug. The patches have issues, though (comment #8).
Comment 19 Simon Fraser (smfr) 2009-05-17 11:20:25 PDT
Created attachment 30423 [details]
Image explaining the strategy for radius drawing
Comment 20 Alex Taylor 2009-05-17 13:06:24 PDT
I modified the interactive test case and had a play with it in Firefox 3 yesterday. Their rendering seems to match the strategy image that Simon uploaded.

Unless one of the recent commenters is keen to fix this themselves, I'll take another look at a this bug over the next few days. By the looks, I think our code is a little too convoluted and results in far too many broken edge cases currently.

I won't assign it to me until tomorrow in case someone else has more of a chance to create a patch.
Comment 21 Alex Taylor 2009-06-28 05:09:06 PDT
Taking bug for another spin, have a patch almost done using clipping paths instead of stroking arcs.
Comment 22 Alex Taylor 2009-06-29 05:06:10 PDT
Created attachment 32006 [details]
Re-written border code using clipping patches

This patch changes all the border rendering code to use elliptical clipping masks when there is a border radius defined instead of approximating the curve by stroking an arc on the corners. The clipping masks are anti-aliased. 

This patch isn't finished at all, I need to do a few more bits and pieces and clean up the code.

Mitz or Hyatt, can I please get one of you to do some performance testing on this code (using PLT?) to see how much optimisation work is required.
Comment 23 Maciej Stachowiak 2009-07-05 04:12:46 PDT
I would expect minimal performance impact in normal browser benchmarks, since border-radius is rarely used, let alone non-uniformly. If you want to check performance, I would suggest making some artificial test cases, but you may need to pile a lot of boxes with rounded corners onto the screen to get something that takes measurable time.
Comment 24 Eric Seidel (no email) 2009-08-04 19:33:56 PDT
Comment on attachment 32006 [details]
Re-written border code using clipping patches

Wow.  We can come up with some way to abstract some of this math.  This is way too much copy/paste.

Huge sections of:
topLeftInnerRadius.setWidth(max(0, topLeftInnerRadius.width() - style->borderLeftWidth()));
 825         topLeftInnerRadius.setHeight(max(0, topLeftInnerRadius.height() - style->borderTopWidth()));


This huge section of checks is repeated at least twice too:
             && (bottomLeft.width() == 0 || bottomLeft.width() >= style->borderLeftWidth())
 895             && (bottomLeft.height() == 0 || bottomLeft.height() >= style->borderBottomWidth())
 8

Style:
 736             int innerBorderTopWidth = style->borderTopWidth()*2/3;
 737             int innerBorderRightWidth = style->borderRightWidth()*2/3;
 738             int innerBorderBottomWidth = style->borderBottomWidth()*2/3;
 739             int innerBorderLeftWidth = style->borderLeftWidth()*2/3;
 740 
Can't we use IntRects here?

Bah!  copy/paste galore:
53             if (style->borderTopWidth() % 3 == 1)
 754                 innerBorderTopWidth += 1;
 755             if (style->borderRightWidth() % 3 == 1)
 756                 innerBorderRightWidth += 1;
 757             if (style->borderBottomWidth() % 3 == 1)
 758                 innerBorderBottomWidth += 1;
 759             if (style->borderLeftWidth() % 3 == 1)
 760                 innerBorderLeftWidth += 1;

I like this change... but It needs to be about half this size.  Using some nice abstractions. :)  Way too much copy paste here to land this.
Comment 25 Alex Taylor 2009-08-06 03:24:27 PDT
(In reply to comment #24)
> (From update of attachment 32006 [details])
> Wow.  We can come up with some way to abstract some of this math.  This is way
> too much copy/paste.

Thanks for setting review- on this, I hadn't realised Maciej didn't with his comment. 
Definitely needs some refactoring and abstraction, I had submitted this and marked review? to get some performance testing but I haven't had a chance to do this myself yet.

> 
> Huge sections of:
> topLeftInnerRadius.setWidth(max(0, topLeftInnerRadius.width() -
> style->borderLeftWidth()));
>  825         topLeftInnerRadius.setHeight(max(0, topLeftInnerRadius.height() -
> style->borderTopWidth()));
> 
> 
> This huge section of checks is repeated at least twice too:
>              && (bottomLeft.width() == 0 || bottomLeft.width() >=
> style->borderLeftWidth())
>  895             && (bottomLeft.height() == 0 || bottomLeft.height() >=
> style->borderBottomWidth())
>  8
> 
> Style:
>  736             int innerBorderTopWidth = style->borderTopWidth()*2/3;
>  737             int innerBorderRightWidth = style->borderRightWidth()*2/3;
>  738             int innerBorderBottomWidth = style->borderBottomWidth()*2/3;
>  739             int innerBorderLeftWidth = style->borderLeftWidth()*2/3;
>  740 
> Can't we use IntRects here?
> 
> Bah!  copy/paste galore:
> 53             if (style->borderTopWidth() % 3 == 1)
>  754                 innerBorderTopWidth += 1;
>  755             if (style->borderRightWidth() % 3 == 1)
>  756                 innerBorderRightWidth += 1;
>  757             if (style->borderBottomWidth() % 3 == 1)
>  758                 innerBorderBottomWidth += 1;
>  759             if (style->borderLeftWidth() % 3 == 1)
>  760                 innerBorderLeftWidth += 1;
> 
> I like this change... but It needs to be about half this size.  Using some nice
> abstractions. :)  Way too much copy paste here to land this.

I'm sure most of the copy/paste can be reduced with some functions for shrinking an IntRect and rounding the sizes of an IntRect etc, I really just needed to confirm that my maths was actually working correctly before making some generic functions.

Thanks for the comments, I really need to get some more work done on this patch!
Comment 26 Jerry Seeger 2010-06-01 11:44:45 PDT
I know I'm chiming in late on this, and I'm really glad someone is tackling this problem, but I think there might be a better way to tackle this.

It seems to me that once you're calculating inner and outer paths, using fills would be a lot simpler than stroke/clip. (Except for dashes. Dashes almost justify stroke/clip, but not quite, in my mind.) You can also control bezels and the like more precisely.

Fills are also a lot more flexible as far as applying gradient colors and other effects (as far as I understand things). If I were to tackle this problem, I'd start by checking the SVG library - it's much easier to build paths with elliptical segments using SVG than with NSPath. Hopefully somewhere in there is a bridge.

Just my two cents; I don't want to diminish or discourage your efforts in any way, but ultimately I think using fills gives the code the best long-term viability.
Comment 27 Beth Dakin 2010-06-22 10:41:20 PDT
Created attachment 59390 [details]
Updated, re-factored version of Alex's patch

Here is an updated, re-factored, and ever-so-slightly behaviorally different version of Alex's patch. This is probably best reviewed in person where I can show all of the improvements and the trade-offs, but I will try to summarize here in case anyone who is not right HERE wants to review:

-Solid, inset/outset, groove/ridge border are WAY better. I am tempted to say this patch fixes all of the bugs with these styles based on Dan's interactive test case, but there might be some edge cases.
-Double border are way better too. There are a few edge-case bugs that can be found with the interactive test case, but I think these could be fine-tuned and fixed in a follow-up patch.
-Dotted and Dashed borders are not better, but I am tempted to say they are not worse either. They are just…different. This is the area that would need the most follow-up work if we decide to go with this patch. These layout tests are probably worse with this patch:

fast/borders/borderRadiusDashed01.html
fast/borders/borderRadiusDashed02.html
fast/borders/borderRadiusDashed03.html

But these ones are better:

fast/borders/borderRadiusDotted01.html
fast/borders/borderRadiusDotted02.html
fast/borders/borderRadiusDotted03.html

This one is a good example of "just different." I don't think the new rendering is better than the old or vice versa.

fast/borders/borderRadiusAllStylesAllCorners.html

Playing with the interactive test case with and without the patch make me feel like it's just trading one set of bugs for another. 

Other obvious problems with this patch are that I need to implement clipConvexPolygon() for non-CG platforms, and need to add many news tests. I will work on that while all of you reviewers out there consider taking a look ;-)
Comment 28 WebKit Review Bot 2010-06-22 10:42:47 PDT
Attachment 59390 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1000:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1000:  ignore_right is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1019:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1019:  ignore_left is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1022:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1022:  ignore_right is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1042:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1042:  ignore_top is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1045:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1045:  ignore_bottom is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1061:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1061:  ignore_top is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1062:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1065:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1065:  ignore_bottom is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/RenderBoxModelObject.cpp:1066:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 22 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Early Warning System Bot 2010-06-22 10:50:37 PDT
Attachment 59390 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3299588
Comment 30 WebKit Review Bot 2010-06-22 12:13:17 PDT
Attachment 59390 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3269600
Comment 31 WebKit Review Bot 2010-06-22 12:14:07 PDT
Attachment 59390 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3316576
Comment 32 Beth Dakin 2010-06-22 13:13:44 PDT
Comment on attachment 59390 [details]
Updated, re-factored version of Alex's patch

Taking the review flag off this for now. I fixed the style errors. The compilation errors are probably just that I haven't implemented that GraphicsContext function on non-CG platforms, and I will get to that.

Simon and I looked at this a little in person, and he convinced me to put a little more effort into getting dashed and dotted styles right. I am looking at that now, but if anyone wants to look at the patch and comment on it in any way, I still encourage that!!
Comment 33 Beth Dakin 2010-06-22 21:43:28 PDT
Created attachment 59473 [details]
New version of the patch

Here's a new version of the patch. Dashed and dotted border looks WAY better now. I also fleshed out stubs in all of the different ports of GraphicsContext. I should really fill these in because border-radius implementations on other platforms will break without this function, but it is a little scary to right them without configurations to compile and test. Hmm…any advice anyone? 

Other know problems:
-Double borders need a little tweaking. This will be easy to fix in a follow-up patch. I added a FIXME to borderWillArcInnerEdge in RenderBoxModelObject.
-Dashed and dotted borders could be even better if we tried centering the dash pattern. Added a FIXME for this too.
-The angle of border sides is a little off sometimes. Should be fix-able in a follow-up patch.
-Need to add more tests. Dan't interactive test case has been invaluable in working on this, and we need more representation in the layout tests. I would like to do this in a follow-up patch as well.
Comment 34 WebKit Review Bot 2010-06-22 21:45:54 PDT
Attachment 59473 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/RenderBoxModelObject.cpp:916:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderBoxModelObject.cpp:917:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderBoxModelObject.cpp:918:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderBoxModelObject.cpp:919:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderObject.cpp:923:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/RenderObject.cpp:924:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Early Warning System Bot 2010-06-22 21:53:06 PDT
Attachment 59473 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3329672
Comment 36 Simon Fraser (smfr) 2010-06-22 22:00:39 PDT
Comment on attachment 59473 [details]
New version of the patch

Some brief comments:
WebCore/platform/graphics/GraphicsContext.h:242
 +          void clipConvexPolygon(size_t numPoints, const FloatPoint*);
It's a bit odd to see size_t there, rather than the normal int or unsigned.

WebCore/platform/graphics/cg/GraphicsContextCG.cpp:443
 +  static void addConvexPolygonToContext(CGContextRef context, size_t npoints, const FloatPoint* points)
Use numPoints, rather than npoints.

WebCore/rendering/RenderBoxModelObject.cpp:913
 +  static bool borderWillArcInnerEdge(IntSize firstRadius, IntSize secondRadius, int firstBorderWidth, int secondBorderWidth, int middleBorderWidth)
Use const IntSize& for params.

WebCore/rendering/RenderBoxModelObject.cpp:954
 +      IntRect borderRect = IntRect(tx, ty, w, h);
IntRect borderRect(tx, ty, w, h)

WebCore/rendering/RenderBoxModelObject.cpp:1085
 +      FloatPoint quad[4];
We have FloatQuad for this.

WebCore/rendering/RenderBoxModelObject.h:118
 +      void clipBorderSidePolygon(GraphicsContext*, IntRect, IntSize topLeft, IntSize topRight, IntSize bottomLeft, IntSize bottomRight, const BoxSide side, const RenderStyle* style);
Use const reference parameters.

WebCore/rendering/RenderObject.cpp:881
 +  IntRect RenderObject::borderInnerRect(const IntRect& borderRect, unsigned short topWidth, unsigned short bottomWidth, unsigned short leftWidth, unsigned short rightWidth) const
I don't know why those are unsigned shorts.

WebCore/rendering/style/RenderStyle.h:790
 +      void getInnerBorderRadiiForRectWithBorderWidths(const IntRect&, unsigned short topWidth, 
Ditto
Comment 37 WebKit Review Bot 2010-06-22 22:23:50 PDT
Attachment 59473 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3302626
Comment 38 WebKit Review Bot 2010-06-22 22:26:16 PDT
Attachment 59473 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3293650
Comment 39 WebKit Review Bot 2010-06-23 06:42:11 PDT
Attachment 59473 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3315676
Comment 40 Beth Dakin 2010-06-23 10:14:23 PDT
(In reply to comment #36)
> (From update of attachment 59473 [details])
> Some brief comments:
> WebCore/platform/graphics/GraphicsContext.h:242
>  +          void clipConvexPolygon(size_t numPoints, const FloatPoint*);
> It's a bit odd to see size_t there, rather than the normal int or unsigned.
> 

I made it a size_t to match drawConvexPolygon()'s parameters. Not sure there is much value in that though. I didn't change this for now since I have a justification for it, however weak. I will happily change it if you think I should after reading the explanation. 

> WebCore/platform/graphics/cg/GraphicsContextCG.cpp:443
>  +  static void addConvexPolygonToContext(CGContextRef context, size_t npoints, const FloatPoint* points)
> Use numPoints, rather than npoints.
> 

Fixed.

> WebCore/rendering/RenderBoxModelObject.cpp:913
>  +  static bool borderWillArcInnerEdge(IntSize firstRadius, IntSize secondRadius, int firstBorderWidth, int secondBorderWidth, int middleBorderWidth)
> Use const IntSize& for params.
> 

Fixed.

> WebCore/rendering/RenderBoxModelObject.cpp:954
>  +      IntRect borderRect = IntRect(tx, ty, w, h);
> IntRect borderRect(tx, ty, w, h)
> 

Fixed.

> WebCore/rendering/RenderBoxModelObject.cpp:1085
>  +      FloatPoint quad[4];
> We have FloatQuad for this.
> 

I started changing this, and then I changed it back. The point of constructing this FloatPoint array is to send it to GraphicsContext::clipConvexPolygon(), which takes a FloatPoint array as a parameter and not a FloatQuad. Of course, GraphicsContext::clipConvexPolygon() is a brand new function called only from new code, so we can make it take a FloatQuad, but then it will be clipFloatQuad() instead of clipConvexPolygon() which seems less flexible for potential future use in other code. 

Anyway, I am open to changing this, but I also think there is a good justification for keeping this the way it is. What do you think?

> WebCore/rendering/RenderBoxModelObject.h:118
>  +      void clipBorderSidePolygon(GraphicsContext*, IntRect, IntSize topLeft, IntSize topRight, IntSize bottomLeft, IntSize bottomRight, const BoxSide side, const RenderStyle* style);
> Use const reference parameters.
> 

Fixed.

> WebCore/rendering/RenderObject.cpp:881
>  +  IntRect RenderObject::borderInnerRect(const IntRect& borderRect, unsigned short topWidth, unsigned short bottomWidth, unsigned short leftWidth, unsigned short rightWidth) const
> I don't know why those are unsigned shorts.
> 
> WebCore/rendering/style/RenderStyle.h:790
>  +      void getInnerBorderRadiiForRectWithBorderWidths(const IntRect&, unsigned short topWidth, 
> Ditto

They are unsigned shorts since RenderStyle stores and returns border widths as unsigned shorts. I didn't see any value in changing the type. I guess there is some value in just having a shorter type name, so I will change these to just unsigned if you want. I didn't change this for now, but I would be fine with changing it. What do you think?

I will attach a new patch with the above changes momentarily.
Comment 41 Beth Dakin 2010-06-23 10:32:49 PDT
Created attachment 59529 [details]
Patch addresses some comments
Comment 42 Beth Dakin 2010-06-23 12:42:45 PDT
Created attachment 59552 [details]
Patch the won't break other platforms

After talking with Darin and Sam, I decided to keep around the old version of RenderBoxModelObject::paintBorder() and RenderObject::drawArcForBoxSide() for non-CG platforms. After I get an r+, I will file bugs for the other platforms to implement GraphicsContext::clipConvexPolygon() and email WebKit dev about it. But in the meantime, non-CG platforms will just have our same old border-radius support, and they can switch to the new hotness once they implement that one function.

Just realized I should update the Changlog to reflect this. Will do that now, but I won't bother posting a new patch for it unless someone really wants me to.
Comment 43 Alex Taylor 2010-06-26 19:13:54 PDT
Awesome, cheers for picking this up Beth. 

There are still a few little ugly edge cases around extreme non-uniform borders but they probably need to be in a separate bug.

The only thing I can see with the outcome of this patch is a nasty 1px anti-aliasing regression on the diagonals at the corners. You can see it on the top-left corner in the interactive test case when there is an inner clipping radius on the corners. (I think it is the border-radius > border-width case?)
Comment 44 Beth Dakin 2010-06-28 12:08:19 PDT
(In reply to comment #43)
> Awesome, cheers for picking this up Beth. 
> 
> There are still a few little ugly edge cases around extreme non-uniform borders but they probably need to be in a separate bug.
> 
> The only thing I can see with the outcome of this patch is a nasty 1px anti-aliasing regression on the diagonals at the corners. You can see it on the top-left corner in the interactive test case when there is an inner clipping radius on the corners. (I think it is the border-radius > border-width case?)

Hey Alex! Thanks for your patches! My patch is basically a modernized version of yours. 

I have a list of a few known bugs that remain from this patch, and I know how to fix most of them, but I am holding out to do the work in follow-up patches. I am not sure I *totally* understand the bug you mention above…it might be one of the bugs I have found, but it might be something I didn't find. It's easy to make a stand-alone test case from the interactive one by finding out the values of the various change-able parameters in the web inspector. If you could make one or just tell me what the values are in the web inspector when you see the bug you describe, that would be very helpful.

Thanks again!
Comment 45 Beth Dakin 2010-06-28 13:54:45 PDT
Thanks Sam!! Fixed with r62035. I will be filing follow-up bugs momentarily.
Comment 46 Beth Dakin 2010-06-28 14:20:10 PDT
Filed:
https://bugs.webkit.org/show_bug.cgi?id=41301
https://bugs.webkit.org/show_bug.cgi?id=41302
https://bugs.webkit.org/show_bug.cgi?id=41303
https://bugs.webkit.org/show_bug.cgi?id=41304

I still need to file bugs for other ports to implement GraphicsContext::clipConvexPolygon().
Comment 47 WebKit Review Bot 2010-06-28 15:09:36 PDT
http://trac.webkit.org/changeset/62035 might have broken GTK Linux 32-bit Debug
Comment 49 Beth Dakin 2010-06-28 15:13:06 PDT
(In reply to comment #47)
> http://trac.webkit.org/changeset/62035 might have broken GTK Linux 32-bit Debug

Looking at the logs, I don't think so.
Comment 50 Kim 2011-04-08 11:14:58 PDT
Created attachment 88844 [details]
On the left is border-radius (plus a few other things) in Firefox 4, the right shows border-radius not quite rendering right in Webkit 534.28

It would be nice to see the curve between two borders with different widths look smooth -- I really don't want to use two elements here to have the same effect <.<;

This bug has some better examples linked:
https://bugs.webkit.org/show_bug.cgi?id=38787