Bug 38787 – Incorrect rendering with one-sided thick border and border-radius

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[James Robinson]

According to http://www.w3.org/TR/css3-background/#border-radius, an element with multiple border-radii must transition smoothly between them "Thus when two adjoining borders are of different thicknesses the corner will show a smooth transition between the thicker and thinner borders."

[Simon Fraser (smfr)]

Maybe dup to bug 9197.

[Simon Fraser (smfr)]

Created attachment 89988 [details]
One possible rendering

On possible rendering for this is shown in the attachment. The code for that looks something like:


diff --git a/Source/WebCore/rendering/style/RenderStyle.cpp b/Source/WebCore/rendering/style/RenderStyle.cpp
index b3ea499..8f14716 100644
--- a/Source/WebCore/rendering/style/RenderStyle.cpp
+++ b/Source/WebCore/rendering/style/RenderStyle.cpp
@@ -803,15 +803,30 @@ RoundedIntRect RenderStyle::getRoundedBorderFor(const IntRect& rect) const
 {
     RoundedIntRect::Radii radii = calcRadiiFor(surround->border, rect.width(), rect.height());
     radii.scale(calcConstraintScaleFor(rect, radii));
+    
+    // Further constrain the radii
+    int leftWidth = borderLeftWidth();
+    int rightWidth = borderRightWidth();
+    int topWidth = borderTopWidth();
+    int bottomWidth = borderBottomWidth();
+    
+    int maxLeftRadius = rect.width() - rightWidth;
+    int maxRightRadius = rect.width() - leftWidth;
+    int maxTopRadius = rect.width() - bottomWidth;
+    int maxBottomRadius = rect.width() - topWidth;
+    
+    radii.setTopLeft(IntSize(min(radii.topLeft().width(), maxLeftRadius), min(radii.topLeft().height(), maxTopRadius)));
+    radii.setTopRight(IntSize(min(radii.topRight().width(), maxRightRadius), min(radii.topRight().height(), maxTopRadius)));
+    radii.setBottomLeft(IntSize(min(radii.bottomLeft().width(), maxLeftRadius), min(radii.bottomLeft().height(), maxBottomRadius)));
+    radii.setBottomRight(IntSize(min(radii.bottomRight().width(), maxRightRadius), min(radii.bottomRight().height(), maxBottomRadius)));
+    
     return RoundedIntRect(rect, radii);
 }

[Simon Fraser (smfr)]

The smooth transition issue is fixed. Repurposing this bug to address the remaining issue with the testcase.

[Simon Fraser (smfr)]

Created attachment 89989 [details]
Testcase

[Simon Fraser (smfr)]

To get this rendering right, we'd have to not consider the border inner edge as a rounded rect.

[Ben Wells]

Created attachment 110436 [details]
A simpler testcase with the same problem

This is a reduced test case from http://crbug.com/98893

[Ben Wells]

Created attachment 110457 [details]
Patch

[Ben Wells]

This fix isn't perfect, in particular:
- it doesn't handle cases where the four borders are different styles or colors as the side polygons are calculated in a conservative way to ensure bits don't get chopped off
- it won't help fix other problems with inner border not being a rounded rect (e.g. border shadow which you can see in chrome bug 98893)

A better fix would be to work out the path of the inner border and use that, perhaps that can come subsequently and this fix stand as a progression.

[Simon Fraser (smfr)]

Comment on attachment 110457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110457&action=review

> LayoutTests/fast/borders/border-radius-complex-inner.html:10
> +    display: inline-block;
> +    border: 5px #00F solid;
> +    width: 20px;
> +    height: 20px;
> +    padding: 0px;
> +    margin: 10px;

Please make everything bigger, more like the mixed-border-styles-radius tests, so that failures are more obvious in pixel tests.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2258
> +    // Expand the inner border as necessary to make it a rounded rect (i.e. radii contained within each
> +    // edge).

Unfortunate wrapping.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2337
> +void RenderBoxModelObject::clipBorderSideForComplexInnerPath(GraphicsContext* graphicsContext, const RoundedRect& outerBorder, const RoundedRect& innerBorder,
> +                                                             BoxSide side, const class BorderEdge edges[])

In new code we don't try to align stuff like this. Just indent the second line by 4 spaces, or unwrap it.

[Ben Wells]

Created attachment 110468 [details]
Patch

[Mike Lawther]

Comment on attachment 110468 [details]
Patch

setting cq+ after previous r+ from smfr

[WebKit Review Bot]

Comment on attachment 110468 [details]
Patch

Rejecting attachment 110468 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
video-playing-and-pause.html = MISSING PASS


Regressions: Unexpected text diff mismatch : (1)
  fast/borders/border-radius-complex-inner.html = TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/10030461

[WebKit Review Bot]

Comment on attachment 110468 [details]
Patch

Attachment 110468 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10030467

New failing tests:
fast/borders/border-radius-complex-inner.html

[Ben Wells]

Created attachment 110636 [details]
Patch with test text results updated and moved.

[Ben Wells]

Comment on attachment 110636 [details]
Patch with test text results updated and moved.

The previous patch had the wrong test text result, this patch updates it and moves it into platform/mac/fast/borders.

[WebKit Review Bot]

Comment on attachment 110636 [details]
Patch with test text results updated and moved.

Rejecting attachment 110636 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 110636 [details]
Patch with test text results updated and moved.

Clearing flags on attachment: 110636

Committed r97318: <http://trac.webkit.org/changeset/97318>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vlad Grecescu]

Created attachment 133665 [details]
work-around for border-radius background clipping

I just noticed (from the w3c link and the EWS tests that failed previously) that need to support non "full quarter ellipses" too, - I initially tried to reduce all the radii to keep the borders and paddings untouched..

So I modified the patch to just work for the common cases, and fall back to the existing code if the clipping border is not 'renderable'.

[Vlad Grecescu]

Ah, sorry, posting in the wrong bug. The patch is meant for https://bugs.webkit.org/show_bug.cgi?id=23166