458131 - test suite for css3 border-radius properties

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Zack Weinberg (:zwol)]

Now that we have basic support (with vendor prefixes) for the CSS3 'border-radius' property, we should have a test suite for it.  Bug 450652 landed a reftests/border-radius/ directory with some minimal "this does something" tests, but we need something more comprehensive. ctalbert agreed to write tests.

(The tests, when written, may expose bugs in the implementation, which should be filed as separate bugzilla bugs that block this one and are assigned to me.)

Comment 1

[Bruno Girin]

I'd be happy to help produce test cases for this. What format would you like them in?

Comment 2

[Zack Weinberg (:zwol)]

Reftests are ideal (put them in the layout/reftests/border-radius/ directory).  It was agreed that SVG and HTML should have matching elliptical curves, so you can use SVG for reference renders; however, no one has done the code audit to make sure they *do* have matching elliptical curves, so don't panic if it doesn't work, just file bugs.

Comment 3

[cmtalbert]

Hey Bruno, we'd welcome any help.  On IRC today, we were reminded that SVG border radius is measured to the center of the stroke line whereas the CSS border radius is measured to the outer edge of the border line.  There were two suggestions for getting around this.  One was to change the border radius to be half-width in order to force SVG to draw in the proper place.  The other one was to put an shape inside a shape and use it to simulate the border radius curves.

So, I've been working on tweaking two tests that attempt to use these two ideas. The tests are at http://people.mozilla.org/~ctalbert/reftest/ If you look in there, test1d* try to use the width principle.  Test1e* tries to use the image in an image method to address this.

I'm not entirely sure that I've done these correctly, but they are reporting very close in reftests.  I'm wondering if I am getting into rounding errors with these.  Any advice you have is completely welcome.

Comment 4

[Alfred Kayser]

Adding bug 448193 as there is a regression on border-radius with outset/inset/ridge/groove borders which makes us fail any standards compliancy testing on this combination.

Comment 5

[cmtalbert]

Attachment: First pass at test cases for border radius

First pass for the border radius tests.  Note that most of these don't have references, for which I'm tracking that issue in bug 459148.  The only case that fails here is the "border-tinyradius-*" files, which is what I'm using for the follow on bug about creating references for this.

Most things look good.  There are a couple of issues though:
* border-padding-scrolling has some issues, filed bug 459144 for that
* border-intersection-double.html -- should the double lines of a border always be parallel?  Here, you can see that in the bottom right corner, the lines move toward each other.  These borders do not change thickness, so I think this is also a bug. Zach, what do you think?

Comment 6

[Zack Weinberg (:zwol)]

(In reply to comment #5)
> Created an attachment (id=342343) [details]
> First pass at test cases for border radius

You should maybe hold off on dashed and dotted tests until I can get back to bug 19963, because that's going to change the rendering for those and invalidate all the references.

> * border-intersection-double.html -- should the double lines of a border always
> be parallel?  Here, you can see that in the bottom right corner, the lines move
> toward each other.  These borders do not change thickness, so I think this is
> also a bug. Zach, what do you think?

Staring at that and border-intersection-groove makes me think the problem is we're not doing a proper inner radius computation for the outer ring(s).  It's more obvious in -groove because there's only two rings to look at; the curvature of the middle boundary should not be exactly the same as the curvature of the outer boundary.

It's not possible to make the lines all be perfectly parallel here, I think, but doing that ought to make it look better anyway.

Comment 7

[Zack Weinberg (:zwol)]

In general, the tests look good, but should they be so tiny?  It's hard to tell, visually, what's going on.

Comment 8

[Zack Weinberg (:zwol)]

I said in bug 459148 comment 2 that after the SVG ellipse approximation is brought in line with the Thebes one, we still have a rendering failure on the tinyradius test case.  I think this is because CSS allows a box border to be of different thickness on each side, and so we draw all elliptical borders by tracing the outside and inside of the border as separate rounded rectangle paths and then filling the space between.  Your SVG, by contrast, traces one rounded rectangle and strokes it.  I am considering an optimization to one-trace-and-stroke when all four borders are the same thickness, but that's all tangled up with the other work in bug 19963, which gets messier every time I look at it. :-(

Anyway, I imagine there is a way to do the same thing with SVG but I don't know how.  You may also run into trouble because SVG rounded rectangles have to have the same corner dimensions on all four corners, whereas CSS lets you have them all different.

Something that's not tested in your current set but should be, and that I think you can test with nothing but HTML, is the CSS-specified algorithm for reducing corner radii when they're too big for the box: see http://dev.w3.org/csswg/css3-background/#the-border-radius (scroll down to just below figure 8).

Comment 9

[cmtalbert]

(In reply to comment #6, #7, and #8)

> Anyway, I imagine there is a way to do the same thing with SVG but I don't know
> how.  You may also run into trouble because SVG rounded rectangles have to have
> the same corner dimensions on all four corners, whereas CSS lets you have them
> all different.
> 
Right, all the tests with SVG references that should match use the same corner dimensions on all four corners.  Let me experiment with some more ways of drawing in SVG plus your patch over in bug 459148 and I'll see if I can come up with another way to do this.

> Something that's not tested in your current set but should be, and that I think
> you can test with nothing but HTML, is the CSS-specified algorithm for reducing
> corner radii when they're too big for the box: see
> http://dev.w3.org/csswg/css3-background/#the-border-radius (scroll down to just
> below figure 8).
Good catch.  I read that section and then forgot to write a test for it :/

I'll hold off on further dashed-dotted stuff for now - they don't have references anyway.  I just wanted to include them so that we could see visually that they work ok.

Should I file a bug on the inner radius computation issue you mention up in comment 6?

And in the next version of the patch, I'll make the tests bigger. :-)

Thanks for the review.

Comment 10

[Zack Weinberg (:zwol)]

(In reply to comment #9)

> Should I file a bug on the inner radius computation issue you mention up in
> comment 6?

Yes please.  Assign to me, and make it block this and bug 19963.

Comment 11

[cmtalbert]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > Should I file a bug on the inner radius computation issue you mention up in
> > comment 6?
> 
> Yes please.  Assign to me, and make it block this and bug 19963.

ok, it's bug 459945.  

Also marking this as blocking 1.9.1 since I think we want these tests (and all the fixes they require) to land before final.

Comment 12

[cmtalbert]

Attachment: Another pass at border-radius testing

Another round with border radius testing. There are a couple of noteworthy additions to this set that I'd like to call out:

= Processing.js =
Last week, Ryan suggested to me online to try using processing as the reference image in these tests.  I included the infrastructure to do that in this patch in case anyone else wants to try it.  From my (incredibly hackified) first take at trying to do this, it doesn't look like it will work because the fundamental drawing arc of the ellipse is different between the two technologies.  Zoom in on the image difference, you'll see what I mean.

I think that processing might be useful if we had the raw point by point output of the bezier curves and the lines of the HTML + CSS output.  In that case, you could probably draw an adequate representation of the shape and it would likely pass.

= Hiding the Curves =
Another attempt I have in this set of tests is a less ambitious but also less exact way to match these.  Take a look at test4.html/test4.xhtml.  This test places small 10px x 10px squares over the border-radius curves allowing the reftest to pass.  This type of test will detect if the border radius calculation makes an error that causes greater curvature (for instance try changing the test to use a radius of 11px).  However, it won't catch any error that causes less curvature (i.e. a radius of 0). You can say that our other tests in the suite will catch the errors that are on the side of less curvature, so perhaps that is not too big of an issue.  Zack, what do you think?

= Possible Bug =
Zack, to start out with the border radius reduction test cases, I started with the test that is illustrated in the specification (example XXII & figure 9). If the radius reduction is truly taking place, then with a height of 2em the radius of 0.5em 2em 0.5em 2em should be reduced by a factor of 0.8 to a radius of 0.4em 1.6em 0.4em 1.6e.  So I expected the shapes to match between these two radius measurements, however that was not the case, and the difference is rather significant, far more than the usual rounding error that we're accustomed to seeing.  Should I file a bug on this?  You can check this out in border-reduce-height.html and border-reduce-height-ref.html

And finally, the patch addresses review comments and makes the shapes larger so that they can more easily be examined by the naked eye.

Comment 13

[cmtalbert]

Attachment: Patch v3.1

This patch now uses the div in a div method Zack figured out on bug 459148. However, the patch does have a couple of issues that I need your help in solving, Zack.

* Reduced Radii - I used the test case from the spec, but the used value numbers are not being reduced.  In border-reduce-height.html, the radii are 8px 32px 8px 32px (0.5em 2em 0.5em 2em), and not 6.4px 25.6px 6.4px 25.6px (.4em 1.6em .4em 1.6em) as I'd expect.  Is this a bug, or am I misinterpreting that part of the spec?

* The specification clearly states that percentage values are not applicable to border-radius properties.  However, when I added those to my "invalid values" tests, I realized that percentage values are used and do calculate a border radius.  I'm going to file a bug on this.

* The div-in-a-div method seems to only work for small values of radii.  Specifically, it seems that the radius value must be 10% or less of the width and height of the square.  Is this a known limitation until bug 456219 is fixed?

Comment 14

[cmtalbert]

Zack, can I get a review on this patch?  I'll test again now that 456219 is completed to see if that 10% rule still holds, and if so I'll make a test with radius > 10% of the width and height of the square. 

Thanks.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 354923 [details] [diff] [review]
Patch v3.1

Clint: it looks like the patch is missing a chunk of the tests that it adds to reftest.list.

These test files are included in "Patch v3.1":
>+== border-value-interpret.html border-value-interpret-ref.html
>+== invalid-units.html invalid-units-ref.html
>+== invalid-units-outline.html invalid-units-outline-ref.html
[snip]
>+== border-reduce-height.html border-reduce-height-ref.html

But these files aren't included:

>+== inherited-values-1.html inherited-values-1-ref.xhtml
>+== child-1a.html child-1-ref.xhtml
>+# Test that you can use border radius as a clipping edge
>+== clipped-1.html clipped-1-ref.xhtml
>+# Can't create reference for corners, so we compare it only to itself
>+== corners-1.html corners-1.html
>+!= corners-1.html about:blank
[snip]
>+# Test that border radius nullified when table { border-collapse: collapse }
>+== table-elements-1a.html table-elements-1-ref.html
>+!= table-elements-1b.html table-elements-1-ref.html
>+

Comment 16

[Zack Weinberg (:zwol)]

(In reply to comment #13)
> 
> This patch now uses the div in a div method Zack figured out on bug 459148.
> However, the patch does have a couple of issues that I need your help in
> solving, Zack.

Sorry for not getting back to you for months on this.

> * Reduced Radii - I used the test case from the spec, but the used value
> numbers are not being reduced.  In border-reduce-height.html, the radii are 8px
> 32px 8px 32px (0.5em 2em 0.5em 2em), and not 6.4px 25.6px 6.4px 25.6px (.4em
> 1.6em .4em 1.6em) as I'd expect.  Is this a bug, or am I misinterpreting that
> part of the spec?

There is no 'box-width' property.  If I change that to '-moz-box-sizing' in both border-reduce-height.html and border-reduce-height-ref.html then they're both rendered identically.  

> * The specification clearly states that percentage values are not applicable to
> border-radius properties.  However, when I added those to my "invalid values"
> tests, I realized that percentage values are used and do calculate a border
> radius.  I'm going to file a bug on this.

(Did you file that bug?) Percentage units are deliberately accepted (the code was there before I started messing with this) and documented on mozdev.  We ought to take this up with the CSS WG, I'll send mail.

> * The div-in-a-div method seems to only work for small values of radii. 
> Specifically, it seems that the radius value must be 10% or less of the width
> and height of the square.  Is this a known limitation until bug 456219 is
> fixed?

Without knowing what problem you had, I'm only guessing here, but as far as I know it ought to work.

Comment 17

[Jonathan Griffin (:jgriffin)]

Attachment: Revtests v4

This patch includes the tests from Clint's previous patch with a few fixes, plus some additional tests that were lost, plus some additional misc. tests.  There are also a few reftests attached to other border-radius bugs, for bugs that are still unresolved.

Comment 18

[Zack Weinberg (:zwol)]

Comment on attachment 385854 [details] [diff] [review]
Revtests v4

Sorry (again) for taking forever to review this.  I tend to forget that anyone's asked me to review anything, it happens so seldom.

You added two "skip" tests to reftest.list - those should be "fails" instead.  Please take out icon-logo.png (which appears to be unused) and remove the unused .div1 rules from table-collapse-1.html and its reference.  Is there any particular reason corner-[12]-ref.xhtml are XHTML instead of bare SVG?

Other than that, looks good to me, although my review may not be good enough to check things in on.

Comment 19

[cmtalbert]

It had been so long, I thought that these had actually been checked in.  Jgriffin, can you work through the last comments in 18 and get these checked in when you get a chance?

Reassigning to Jgriffin, to correctly show the state of the bug.

Comment 20

[Jonathan Griffin (:jgriffin)]

I addressed Zack's comments and pushed as http://hg.mozilla.org/mozilla-central/rev/f0f48d5f535a