Bug 12425 – border-radius not applied to fieldset with legend

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[nerkles]

border-radius not applied to fieldset

Confirmed. Changed component to Layout & Rendering.

[Sam Weinig]

I cannot reproduce this in ToT.  Could you please attach a test case demonstrating border-radius not working.

[nerkles]

Created attachment 12700 [details]
Shows the bug.

I hadn't noticed that the bug only appears if the fieldset includes a LEGEND. Sorry about that. Here's a test file.

[Alex Taylor]

I'll take this one, RenderFieldset has a separate border drawing code path if there is a legend present to make sure the border doesn't overlap the legend. It needs to be upgraded to deal with Border Radii.

[Alex Taylor]

Created attachment 21812 [details]
Implements border radius logic for a fieldset, based off RenderObject's implementation

This patch implements the border radius logic based off RenderObject's implementation but with parts that make sure the area around the legend is excluded. This just extends the current "draw the border pieces that don't overlap the legend" logic with some additional trigonometry to make the arcs shorter if the legend will overlap rather than just not rendering a corner piece.

This seems to work well but I think we should instead make the legend be an clipped region of the border rather than 'avoiding drawing the border where the legend is' as the current (and this) implementation does. If that is the case, this entire function can hopefully be removed and the RenderObject version used, that would be a separate bug though.

Firefox uses the clipped method, they clip the border around the bounding box of the legend element.

This patch is for review only, no tests have been included in the patch.

[Alex Taylor]

Created attachment 21813 [details]
Additional test case.

 This displays many different situations that the legend can be in. Probably the basis for a layout test.

[Alex Taylor]

Also, there are (double) and (int) c-style casts in there, should I be using static casts or another method, I haven't done any C++ casting in the past besides c-style so I need some pointers there.

[Alex Taylor]

Created attachment 21867 [details]
Method two: using a clipping region

This patch uses the alternate way of clipping the legend. It clips the region where they legend will be displayed. 

This keeps the rendering consistent with the current case where there are no border radii but adds support but calling the parent's "paintBoxDecorations" method to follow the default code path but with a clipping region attached.

This uses a lot less code than the current way of drawing the pieces around the legend and trying to not draw over the top of it. It also prevents code duplication between drawing borders for a fieldset and for a generic object. 

As a side note, testing showed that this is consistent with how Firefox handles the border radius + legend case on a fieldset.

Comment on attachment 21867 [details]
Method two: using a clipping region

I like this approach much better than the other patch's. However, if you are removing the only call site to paintBorderMinusLegend(), you should also remove the method. Alternatively, you can choose to keep using it in the straight-corner case, since arguably clipping (and saving and restoring the graphics state) incurs a small performance cost.

[Alex Taylor]

Created attachment 21868 [details]
Method two: using a clipping region with no border radius speed up

This patch adds a speed up to prevent unnecessary saving and restoring of the graphics context when we don't have border radii to deal with. 

Also added 3 layout tests for left align legend, right aligned legend and a really fat bordered fieldset where the clipping region is obvious.

Comment on attachment 21868 [details]
Method two: using a clipping region with no border radius speed up

Looks good! A few comments:

1) Please add the bug URL and title to the change log and add change log comments about the code changes in each function.

2) Won't this paint fill layers a second time (except behind the legend)?

+    RenderBlock::paintBoxDecorations(paintInfo, tx, ty);

Why are you not calling just paintBorder()?

3) Likewise, the change log for LayoutTests should include a reference to the bug. You should generate results (including pixel results) and include them in the patch.

4) Please add newlines:
\ No newline at end of file

5) I think a single test containing the three separate cases would be better (make sure everything fits into the 800x600 rectangle that's included in pixel test results). Maybe something like LayoutTests/fast/block/basic/fieldset-stretch-to-legend.html , only adding border-radius, to cover all interesting cases.

r- for now mostly because of 2)

Comment on attachment 21867 [details]
Method two: using a clipping region

Removing the review flag, assuming this is obsoleted by the later patch.

[Alex Taylor]

Created attachment 21869 [details]
Method two: using a clipping region

Addressed mitz concerns and changed call to paintBorderDecorations to paintBorder which was incorrect before.

Added expected results to pixel test and created composite test case based from the base fieldset test,

Comment on attachment 21869 [details]
Method two: using a clipping region

r=me

 LayoutTests/ChangeLog should include the expected results files, too, but this time the person landing the patch will have to add them.

Landed in <http://trac.webkit.org/changeset/34715>.