[CSS3 Background and Borders] Element with border-radius should clip background

[Andreas Kling]

For this page on IE9 Test Drive we don't clip the background gradient to the rounded rectangle:
http://ie.microsoft.com/testdrive/HTML5/DayNight/Default.html

A cursory exam shows that RenderBox::pushContentsClip() returns early because (!isControlClip && !isOverflowClip).

[Renata Hodovan]

Created attachment 78399 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78399 [details]
Patch

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

> ChangeLog:9
> +        CSS3 standard requires the clipping of box's background to the appropriate
> +        curve of the border: http://www.w3.org/TR/css3-background/#corner-clipping

No, that text only refers to backgrounds. It does not talk about clipping children (which happens correctly if you set overflow:hidden).

It's not clear to me that there's a bug here (though the issue has been discussed a few times on www-style).

[Andreas Kling]

(In reply to comment #2)
> No, that text only refers to backgrounds. It does not talk about clipping children (which happens correctly if you set overflow:hidden).
> 
> It's not clear to me that there's a bug here (though the issue has been discussed a few times on www-style).

Right, so the bug here is that we're not clipping the background (rather than the children) to the curved border. Or am I missing something?

[Dave Hyatt]

Comment on attachment 78399 [details]
Patch

Although this isn't technically wrong, the reason self-painting layers were excluded is that they need to be patched to account for border-radius clipping.  I kind of always assumed we'd just fix that over in the RenderLayer code.  I don't like this change because you cause the overflow clip for self-painting layers to get pushed twice.

[Simon Fraser (smfr)]

I assume that "just fix that over in the RenderLayer code" you mean teach RenderLayer about non-rectangular clip regions?

[Renata Hodovan]

Created attachment 78685 [details]
Teaching RenderLayer about non-rectangular clip regions :)

[Simon Fraser (smfr)]

Comment on attachment 78685 [details]
Teaching RenderLayer about non-rectangular clip regions :)

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

> WebCore/rendering/RenderLayer.cpp:2513
> +        if (renderer()->style()->hasBorderRadius()) {
> +            IntSize topLeft, topRight, bottomLeft, bottomRight;
> +            renderer()->style()->getBorderRadiiForRect(clipRectToApply, topLeft, topRight, bottomLeft, bottomRight);
> +            paintInfo.context->addRoundedRectClip(clipRectToApply, topLeft, topRight, bottomLeft, bottomRight);
> +        }

Sorry, this isn't enough. You need to change ClipRects to handle non-rectangular regions.

To see this in your testcase, give .inner opacity for example.

[Renata Hodovan]

(In reply to comment #7)
> (From update of attachment 78685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78685&action=review
> 
> > WebCore/rendering/RenderLayer.cpp:2513
> > +        if (renderer()->style()->hasBorderRadius()) {
> > +            IntSize topLeft, topRight, bottomLeft, bottomRight;
> > +            renderer()->style()->getBorderRadiiForRect(clipRectToApply, topLeft, topRight, bottomLeft, bottomRight);
> > +            paintInfo.context->addRoundedRectClip(clipRectToApply, topLeft, topRight, bottomLeft, bottomRight);
> > +        }
> 
> Sorry, this isn't enough. You need to change ClipRects to handle non-rectangular regions.
Could you give me some advice how to do it?

[Renata Hodovan]

Created attachment 81040 [details]
Patch

[Simon Fraser (smfr)]

You need a testcase with overflow:hidden on the outer, and something like position:relative on the inner.

When I say that RenderLayer clipRects need to be radius-aware, I mean the code in RenderLayer::calculateClipRects() and the entire ClipRects class.

here's another test case that shows the inconsistent clipping inside/outside the border of the outer div:

http://jsfiddle.net/N2cXJ/1/

[Philip Tellis]

I'm not sure if this is the same bug or something different, but it seems to have the same symptoms, except with just a single div.  Here's my test case:
http://jsfiddle.net/5dGAt/2/

Description:

if position is static, then the contents of a container element clip to its border.
If position is anything else, then the contents clip to its outline.

Note that in either case, the contents wrap at the outline.

[Simon Fraser (smfr)]

*** Bug 41356 has been marked as a duplicate of this bug. ***

[Rob Crowther]

Is this the same underlying problem, or is there another bug for border-radius not clipping the video element?:

http://jsfiddle.net/vDPW2/68/

Here's another comprehensive test case exhibiting the buggy behavior (forked from [e-mail]'s example):

http://jsfiddle.net/timbowhite/wnHaU/7/

The 1st example (static > static) correctly clips the corners of the inner div to the inner border of the outer div.

The 2nd example (static > absolute) correctly busts out the inner div over the outer div.

Apparently, since the rest of the examples do not have both divs positioned "static", they fail to render correctly.  Poop.

The expected behavior for the remaining examples is that the inner divs' corners should be clipped to the inner border of the outer div. See http://www.w3.org/TR/css3-background/#corner-clipping.  The spec doesn't mention anything about relative position changing this behavior.

This bug is kinda a bummer for doing fancy stuff like images with round corners.  Apologies for name dropping, but Firefox 4+ and IE9 got this one licked.

Thanks!

[Dave Hyatt]

Created attachment 107529 [details]
Patch that fixes the bug.

[WebKit Review Bot]

Attachment 107529 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderLayer.cpp:2585:  This { should be at the end of the previous line  [whitespace/braces] [4]
LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/rendering/RenderLayer.h:77:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Comment on attachment 107529 [details]
Patch that fixes the bug.

Didn't mean to flag. Just showing the approach.

[Dave Hyatt]

Created attachment 107541 [details]
Patch

[WebKit Review Bot]

Attachment 107541 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderLayer.cpp:2585:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Fixed in r95239.

[Mark Rowe (bdash)]

This appears to have caused the web process to get in to an infinite loop in some cases.  See bug 68314.

[Antonio Gomes]

(In reply to comment #22)
> This appears to have caused the web process to get in to an infinite loop in some cases.  See bug 68314.

is not it what bug 68040 fixed?

[Tom Hudson]

This changeset fixed rounded rectangle clipping, but utterly destroyed the performance of Chromium on twitter.com (http://code.google.com/p/chromium/issues/detail?id=97716). I'm posting this as a heads-up that we'll be working in the area, and would welcome any insight from the people who have been here recently.

[Dave Hyatt]

(In reply to comment #24)
> This changeset fixed rounded rectangle clipping, but utterly destroyed the performance of Chromium on twitter.com (http://code.google.com/p/chromium/issues/detail?id=97716). I'm posting this as a heads-up that we'll be working in the area, and would welcome any insight from the people who have been here recently.

What makes clipping hard is that it doesn't necessarily follow the RenderLayer painting hierarchy. I think what's needed to improve clipping performance on pages with large numbers of layers (that themselves all cllp) like Twitter is to try to come up with a scheme where the parent layer pushes the clip, and the children don't have to pop or re-push if they end up wanting to respect that clip.

In other words, right now the whole system is designed around the exceptions rather than the common case.

This will probably be extremely difficult for anyone not very familiar with this code to fix.

(Also note Twitter performance is bad already before this patch, and it's basically the same issue. The clips just got more expensive, so they're showing off the already-existing problem even more.)

[Dave Hyatt]

The most promising improvement would probably be to find a way to treat positioned layers more like transform layers and go ahead and translate/push their clips/transparency layers aggressively. You can't do this is you're tainted with any fixed positioned children, however (well, not without popping all of the clips and repushing them again somehow).

I tried a simple experiment doing this and Twitter performance seemed good with that change.

[Dave Hyatt]

Filed 

https://bugs.webkit.org/show_bug.cgi?id=68733

to cover the regression.

[Nicholas Shanks]

Is this not a duplicate of 9543?

[Simon Fraser (smfr)]

*** Bug 9543 has been marked as a duplicate of this bug. ***