Bug 50072

Summary: [CSS3 Background and Borders] Element with border-radius should clip background
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, devin.chalmers, eae, hyatt, jquerypeepshow, nickshanks, philip.tellis, piersh, rhodovan.u-szeged, robertc, simon.fraser, tomhudson, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://ie.microsoft.com/testdrive/HTML5/DayNight/Default.html
Bug Depends on:    
Bug Blocks: 27569    
Attachments:
Description Flags
Patch
hyatt: review-
Teaching RenderLayer about non-rectangular clip regions :)
simon.fraser: review-
Patch
simon.fraser: review-
Patch that fixes the bug.
hyatt: review-
Patch bdakin: review+

Description Andreas Kling 2010-11-25 05:47:14 PST
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).
Comment 1 Renata Hodovan 2011-01-10 07:44:04 PST
Created attachment 78399 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-01-11 13:02:11 PST
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).
Comment 3 Andreas Kling 2011-01-11 13:10:27 PST
(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?
Comment 4 Dave Hyatt 2011-01-11 13:33:31 PST
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.
Comment 5 Simon Fraser (smfr) 2011-01-11 13:44:14 PST
I assume that "just fix that over in the RenderLayer code" you mean teach RenderLayer about non-rectangular clip regions?
Comment 6 Renata Hodovan 2011-01-12 06:28:05 PST
Created attachment 78685 [details]
Teaching RenderLayer about non-rectangular clip regions :)
Comment 7 Simon Fraser (smfr) 2011-01-12 08:53:16 PST
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.
Comment 8 Renata Hodovan 2011-01-13 05:29:56 PST
(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?
Comment 9 Renata Hodovan 2011-02-03 01:11:15 PST
Created attachment 81040 [details]
Patch
Comment 10 Simon Fraser (smfr) 2011-02-03 07:42:41 PST
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.
Comment 11 piersh 2011-04-01 23:48:24 PDT
here's another test case that shows the inconsistent clipping inside/outside the border of the outer div:

http://jsfiddle.net/N2cXJ/1/
Comment 12 Philip Tellis 2011-04-09 18:42:55 PDT
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.
Comment 13 Simon Fraser (smfr) 2011-04-10 10:03:07 PDT
*** Bug 41356 has been marked as a duplicate of this bug. ***
Comment 14 Rob Crowther 2011-06-04 14:39:30 PDT
Is this the same underlying problem, or is there another bug for border-radius not clipping the video element?:

http://jsfiddle.net/vDPW2/68/
Comment 15 jquerypeepshow 2011-06-22 15:42:24 PDT
Here's another comprehensive test case exhibiting the buggy behavior (forked from piersh@hotmail.com'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!
Comment 16 Dave Hyatt 2011-09-15 12:24:40 PDT
Created attachment 107529 [details]
Patch that fixes the bug.
Comment 17 WebKit Review Bot 2011-09-15 12:28:20 PDT
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.
Comment 18 Dave Hyatt 2011-09-15 12:29:21 PDT
Comment on attachment 107529 [details]
Patch that fixes the bug.

Didn't mean to flag. Just showing the approach.
Comment 19 Dave Hyatt 2011-09-15 13:33:00 PDT
Created attachment 107541 [details]
Patch
Comment 20 WebKit Review Bot 2011-09-15 13:35:37 PDT
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.
Comment 21 Dave Hyatt 2011-09-15 16:49:30 PDT
Fixed in r95239.
Comment 22 Mark Rowe (bdash) 2011-09-17 18:23:44 PDT
This appears to have caused the web process to get in to an infinite loop in some cases.  See bug 68314.
Comment 23 Antonio Gomes 2011-09-17 19:20:20 PDT
(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?
Comment 24 Tom Hudson 2011-09-23 13:46:53 PDT
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.
Comment 25 Dave Hyatt 2011-09-23 14:06:28 PDT
(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.)
Comment 26 Dave Hyatt 2011-09-23 14:35:32 PDT
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.
Comment 27 Dave Hyatt 2011-09-23 14:43:34 PDT
Filed 

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

to cover the regression.
Comment 28 Nicholas Shanks 2011-09-26 16:04:48 PDT
Is this not a duplicate of 9543?
Comment 29 Simon Fraser (smfr) 2011-09-26 16:09:58 PDT
*** Bug 9543 has been marked as a duplicate of this bug. ***