Bug 6755 – CSS3: Borders rounded with border-radius don't draw the roundings

• Component: CSS
• Status: RESOLVED FIXED

[Joost de Valk (AlthA)]

According to the specs:
"All border styles ('solid', 'dotted', 'inset', etc.) follow the curve of the border."

see the spec:
http://www.w3.org/TR/2005/WD-css3-background-20050216/#the-border-radius

As forthcoming testcase shows, this doesn't work at this point, instead, the rounding is drawn in the background-color of the element with the rounded corners.

[Joost de Valk (AlthA)]

Created attachment 5906 [details]
Testcase

[Rachel Blackman]

I'm relatively new to WebCore/WebKit, but this bug has been annoying me so I started to poke at the source.  It seems to me that the problem is both a fairly simple one, and a fairly hairy one.  

The simple part is that border-radius is handled by drawing onto a surface clipped to a rounded rectangle; thus the corners of the still-rectangular border draw outside the clip, and are omitted.  Obviously, this (and probably border-image, though I've not poked extensively into that) needs to draw the arced bits as well.  Simple enough...

The hairy part is that the way drawBorder is laid out right now, either all the different logic for drawing styles needs to be duplicated in a drawBorderArcs, or more state information (such as the border radius stuff) needs to be passed down into drawBorder and it needs to draw the arcs.  Neither seems ideal, though duplicating the logic in a drawBorderArcs strikes me as more annoying, but probably less intrusive on the codebase than trying to change the calling format of drawBorder.

I'm willing to look at tackling that, though I'd kind of not mind a sanity-check from someone more familiar with WebCore, before I go off trying to duplicate the Giant Border-Style Switch Statement of Doom or anything. :)

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> I'm willing to look at tackling that, though I'd kind of not mind a
> sanity-check from someone more familiar with WebCore, before I go off trying to
> duplicate the Giant Border-Style Switch Statement of Doom or anything. :)

I suggest you head over to IRC.  It's usually easier to ask questions there than via bug comments unless the right person/people are on the CC list.

http://webkit.opendarwin.org/contact.html

A lot of the Mac users on IRC use Colloquy for an IRC client.

http://colloquy.info/

[Dave Hyatt]

Yeah, basically you need a function to draw the rounded borders for every possible style.  I can't even begin to guess how joins of different styles should work also.

[Beth Dakin]

Created attachment 8115 [details]
Test case that uses -webkit prefix

Here is the same testcase with the -webkit prefix attached to border-radius so that it works with current TOT.

[Beth Dakin]

Created attachment 8484 [details]
First shot at border-radius....(incomplete)

Here is a patch for border-radius. Several things about this patch:

1. It is incomplete. It does not yet draw the inset, groove, ridge, or outset styles.
2. My drawBorderArc() function is based largely on drawBorder() but right now, it ignores all cases where adjbw1 and adjbw2 are nonzero. This is mostly because I don't understand what adjbw1 and adjbw2 are just yet...need to talk to Hyatt or someone about that.
3. I took over the drawArc() function in GraphicsContext to make it specific to what I needed. I think this is fine because drawArc() is never actually called from anywhere in WebCore outside of this patch.

Okay, I think that's mostly it. Mainly I am just looking for feedback on what I have right now.

[Beth Dakin]

Comment on attachment 8484 [details]
First shot at border-radius....(incomplete)

wait wait! ignore this patch! i caused a big problem with it accidentally before i posted it. will re-post very soon.

[Beth Dakin]

Created attachment 8487 [details]
Fixed first shot at border-radius (still incomplete)

Okay, so all of my previous comments about the last patch still apply to this one, except that I added inset and outset while I fixed the previous bug because I realized they were easy to implement. So definite remaining problems with this patch are: 

-ridge and groove are still unimplemented
-still ignoring adjbw1 and adjbw2

[Eric Seidel (no email)]

Comment on attachment 8487 [details]
Fixed first shot at border-radius (still incomplete)

I think this needs some cleanup.
drawArc shoudl have lables on all of its "int" parameters in the header.  The general rule is that we omit parameter names when they don't add to readabilty of the code (for example ExecState* exec), but we keep them when they do.  In this case, these definitely need lables.

That said, drawArc should also be converted to use the more modern IntRect or possibly FloatRect.  If you do convert to IntRect, you might want to have local float x; float y; float w, etc. variables since pretty much all uses of x, y, w, h, you're casting directly to floats.

int a and int alen need better names.

Note that you should probalby also look at the cairo implementation of drawArc and see what code can/should be shared.

render_l, render_r render_radii , ignore_left, ignore_right, don't obey modern style guidelines.

We can talk more in person.

[Beth Dakin]

Created attachment 8527 [details]
Next round...still not ready to land

Here is a new patch. This patch takes care of all of the different border styles. I do not want to land this until I can sit down with Hyatt or someone and talk about some of the edge cases that may or may not require hackery...but I would still like someone to look at this because it is almost ready.

[Beth Dakin]

Created attachment 8563 [details]
Clips inside arc as well

Okay here is my latest version of the patch. I feel much better about this one than either of the previous two. This patch also adds a clip inside the arc so that some edge cases (like 1 px borders) will now look correct. There are two things I would like to discuss about this patch before it is landed (in addition to any comments people have, of course). First, antialiasing yes or no? This patch that i am posting now leaves antialiasing on, but I have been switching it on and off throughout working on this bug, and I can't decide which is better. When there is not background color, the borders themselves look significantly better with antialiasing on, but some background colors mix with the antialiased border arcs to make the corners looks a little smudgy. In the end, i may just have to sit down with Hyatt or someone and show both versions and decide which we prefer. The other thing is dotted and dashed. they aren't perfect but I am not sure how else to do them. It is also possible that we want to just file that as a follow-up bug since getting those styles just right isn't necessarily our priority right now.

[Timothy Hatcher]

Beth, can you attach screenshots of samples with anti-aliasing on and off? I really think anti-aliasing should be on to give a nice clean feel, but I would like to see a side-by-side comparison.

[Beth Dakin]

Created attachment 8602 [details]
No antialising, no background color

[Beth Dakin]

Created attachment 8603 [details]
Antialiasing on, no background color

[Beth Dakin]

Created attachment 8604 [details]
No antialising, with background color

[Beth Dakin]

Created attachment 8605 [details]
antialiasing ON, with background color

[Beth Dakin]

I attached just a couple of screenshots with and without antialiasing. The problems are most obvious in the double border style (I can barely notice a difference with solid styles), so I only attached photos of double lines.

[Joost de Valk (AlthA)]

Don't know if my vote counts here, but i'm voting in favor of the anti-aliasing!

[Nicholas Shanks]

I vote in favour of anti-aliasing too, it's to be left up to the website designer not to use ugly colours ;-)

[Dave Hyatt]

Comment on attachment 8563 [details]
Clips inside arc as well

(1) Turn antialiasing off.
(2) Stub out the Cairo methods and make sure Windows at least compiles.
(3) File a followup bug about double borders.

For double borders I think you want to pretend like you're drawing two solid borders, so do the outer/inner clip for each line separately.

[Beth Dakin]

I committed this fix and filed:

http://bugzilla.opendarwin.org/show_bug.cgi?id=9197 CSS3: Borders with border-radius and double, groove, or ridge styles should look better

http://bugzilla.opendarwin.org/show_bug.cgi?id=9198 border-radius logic in GraphicsContext needs to be written in Cairo