Closed Bug 382613 (spirograph) Opened 15 years ago Closed 15 years ago

-moz-border-radius of background color isn't clamped to 50% (regression from 368247)

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: netrolller.3d, Assigned: vlad)

References

()

Details

Attachments

(4 files, 1 obsolete file)

If a large object has a -moz-border-radius larger than 50% or its equivalent in other units, the border's radius gets clamped to 50%, but not the background, resulting in a serious mess. On some of the "circles", scrolling has an effect on the appearance.

Reproducible: Always

Steps to reproduce:
Open the attached testcase.

Actual results:
The -moz-border-radii are interpreted for the background as they are, resulting in strange errors on the page.

Expected results:
The -moz-border-radii are interpreted as 50% both for the border and the background, resulting in all 4 tests being rendered as circles. Scrolling doesn't change the appearance of the page.
Flags: blocking1.9?
Whoops, I didn't do the same kind of clamping to the border radii for backgrounds as I do for borders themselves.  I have a fix, but I need to investigate how this interacts with inline elements that get split... I've put a testcase in the URL.

Firefox 2 ends up drawing a rounded border around each segment; if you remove the -moz-border-radius, there is no yellow border on the right side of the first ABC or the left side of the third, and not on either side of the middle -- as it should be.

Fx3 currently does the same, but the background of each of the 3 chunks is rounded; the problem is that PaintBackground doesn't get the same information about which sides to skip as the border painting methods do.
(The patch that I have is worth taking anyway though -- as much as I think that the testcase rendering is pretty, especially the 100% case, I don't think we can call it a feature :)
That screenshot confirms this on Mac, so All/All.
OS: Windows Vista → All
Hardware: PC → All
Attached file animated testcase
Tests the range 0% to 180% for a square.
Attached patch clamp background border radius (obsolete) — Splinter Review
Here's the fix.  Do the right clamping in reused code, and also fudge things so that backgrounds without a border get rendered right.

There's still one edge case:

data:text/html,<div style="-moz-border-radius: 50px; background: red; border: 30px solid blue; border-left: none; width: 200px; height: 200px;"></div>

Basically, it's hard to decide when to round the background and when not to.  We only have one -moz-border-radius property, and it's technically the border radius.  So if we don't have a border on one side, what do we do?  The code in there forces adjacent corners to have no radius, which I think is visually correct.

But that obviously breaks down when -moz-border-radius is used without a border, to just round a background.  So this patch here forces all borders to be at least 1, for the purpose of computing radii for backgrounds.  However, we then get into trouble when you actually have an element like the edge case above, where one side of the border is explicitly left off.  This is ok though, because the Fx2 behaviour was even more broken (we drew the background with a radius, but it stuck out into the border area, and the border was oddly cut off, etc.).
Attachment #267349 - Flags: review?(roc)
static void
+ComputePixelRadii(const nscoord *aTwipsRadii,
+                  const nsRect& outerRect,
+                  const nsMargin& borderMargin,
+                  PRIntn skipSides,
+                  nscoord twipsPerPixel,
+                  gfxFloat *oBorderRadii)

What naming convention(s) are you using here? :-)

This needs a comment.

I don't understand your approach to missing borders. Why introduce a discontinuity at border size zero? A 1-appunit border shouldn't render very differently to a 0 appunit border. I think in the style system we can tell the difference between a border of width 0 and 'none'. IMHO the latter should stop adjacent corners from rounding, but the former should not.

> But that obviously breaks down when -moz-border-radius is used without a
> border, to just round a background.

Should we allow this? Can we require people to apply border: 0 in that case?
W3c says this on 'border-radius':
The border radius also causes the element's background to be rounded, even if the border is 'none'. 

So, when -moz-border-radius is defined for a corner, even if the corr. border is none or 0, the background of that corner needs to be rounded.
Then it seems we shouldn't check the border width at all, just the passed-in aSkipSides parameter. This should just be telling us when an element was broken and it really doesn't have a left/right/top/bottom border.
Alias: spirograph
Attached patch clamp, v2.0Splinter Review
Forgot I had this patch still sitting around... this does better clamping, and doesn't special-case a border width of 0 for border-radius.  The only downside is that when an element with a border is split (and we get skipsides passed in), we'll round the background of each chunk seperately... this is what 2.0 did though, so it's not so bad.
Attachment #267349 - Attachment is obsolete: true
Attachment #268961 - Flags: review?(roc)
Attachment #267349 - Flags: review?(roc)
Comment on attachment 268961 [details] [diff] [review]
clamp, v2.0

ComputePixelRadii needs documentation.
Attachment #268961 - Flags: superreview+
Attachment #268961 - Flags: review?(roc)
Attachment #268961 - Flags: review+
Whiteboard: [checkin needed]
Whiteboard: [checkin needed]
When will this be finally checked in? Or is there still some other work going on about it?
Flags: blocking1.9? → blocking1.9+
Checked in (with documentation).
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
v.fixed

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.