Bug 42605

Summary: New border-radius path-based drawing code has some issues with corner-joins
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, jamesr
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
New patch
mitz: review+
Pixie screenshot none

Description Beth Dakin 2010-07-19 16:37:15 PDT
The new path-based border-radius drawing code in CG has resulted in a corner-join regression. This is a good example of the regression being bad:

http://ie.microsoft.com/testdrive/Graphics/IE%20Logo/Default.html

This is because GraphicsContext::clipConvexPolygon() is currently written to be an anti-aliased clip. (That clipping sets up all the corner joins.) When we anti-alias that clip, we sometimes get these bad corner joints. In the long term, the solution to the problem is to be able to re-write the code so that it is capable of drawing multiple sides of the border at once, because this would not only prevent bad joins in the ie-logo case, but it would also fix our badness with alpha borders (see fast/borders/border-radius-wide-border-03.html). In fact, we have this ialpha-border issue with straight borders too! So that is not specific to border-radius. Right now, doing that seems like a major project, and it also seems messy in it's own way because I can't think of a way to write the code that doesn't involve branching for every possible combination of possibly-same-colored edges. Anyway, that would be good to figure out some day because it would make our corners look much better on borders with alpha. 

But since it is such a large project (and the alpha issue is not a regression nor is it unique to border-radius), I think it is beyond the scope of fixing this regression. I have a simple patch to fix just this issue that I will upload shortly.
Comment 1 Beth Dakin 2010-07-19 17:08:11 PDT
Created attachment 62015 [details]
Patch

Here is a patch that disables anti-aliasing when the border is all one color. I chose not to disable anti-aliasing all the time because I felt like there were some examples in the fast/border directory that looked worse when anti-aliasing was disabled in all cases. It always looks better when the border is all one color though.
Comment 2 Beth Dakin 2010-07-20 18:54:34 PDT
Comment on attachment 62015 [details]
Patch

Building a better patch…
Comment 3 Beth Dakin 2010-07-21 16:31:40 PDT
Created attachment 62246 [details]
New patch

Here is a smarter patch that can handle anti-aliasing some corners and not others. Dan also suggested changing the boolean parameter to GraphicsContext::clipConvexPolygon() into an enum. I didn't do this only because I couldn't think of a good name for the enum. Any ideas?
Comment 4 mitz 2010-07-21 16:41:03 PDT
Comment on attachment 62246 [details]
New patch

r=me with some polish suggested in person
Comment 5 Beth Dakin 2010-07-21 16:54:05 PDT
Thanks Dan! Fixed with http://trac.webkit.org/changeset/63864
Comment 6 James Robinson 2010-07-22 00:12:43 PDT
There are visible seams in many of the new expected results - for example the top left and bottom right of http://trac.webkit.org/export/63877/trunk/LayoutTests/platform/mac/fast/borders/borderRadiusOutset01-expected.png.  It does look better than the previous behavior (which was http://trac.webkit.org/export/63877/trunk/LayoutTests/platform/chromium-mac/fast/borders/borderRadiusOutset01-expected.png) but still not very good.  Is there another bug to track this?
Comment 7 Beth Dakin 2010-07-22 10:21:32 PDT
(In reply to comment #6)
> There are visible seams in many of the new expected results - for example the top left and bottom right of http://trac.webkit.org/export/63877/trunk/LayoutTests/platform/mac/fast/borders/borderRadiusOutset01-expected.png.  It does look better than the previous behavior (which was http://trac.webkit.org/export/63877/trunk/LayoutTests/platform/chromium-mac/fast/borders/borderRadiusOutset01-expected.png) but still not very good.  Is there another bug to track this?

Actually I don't seem any visible seems in the results for that test. Could you provide a screenshot demonstrating the problem that you see?
Comment 8 Beth Dakin 2010-07-22 10:22:51 PDT
Created attachment 62312 [details]
Pixie screenshot

Here's a pixie zoom-in of the lower right corner which you said has a visible seam. I don't see one.
Comment 9 James Robinson 2010-07-22 12:55:05 PDT
You're right, that looks perfectly smooth.  I must have been seeing things after staring at too many pixel results.  Sorry for the noise!
Comment 10 Beth Dakin 2010-07-22 12:57:45 PDT
No worries. I totally know the feeling :-)