Bug 50072 - [CSS3 Background and Borders] Element with border-radius should clip background
: [CSS3 Background and Borders] Element with border-radius should clip background
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://ie.microsoft.com/testdrive/HTM...
:
:
: 27569
  Show dependency treegraph
 
Reported: 2010-11-25 05:47 PST by
Modified: 2011-09-26 16:09 PST (History)


Attachments
Patch (28.13 KB, patch)
2011-01-10 07:44 PST, Renata Hodovan
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
Teaching RenderLayer about non-rectangular clip regions :) (28.48 KB, patch)
2011-01-12 06:28 PST, Renata Hodovan
simon.fraser: review-
Review Patch | Details | Formatted Diff | Diff
Patch (27.52 KB, patch)
2011-02-03 01:11 PST, Renata Hodovan
simon.fraser: review-
Review Patch | Details | Formatted Diff | Diff
Patch that fixes the bug. (195.56 KB, patch)
2011-09-15 12:24 PST, Dave Hyatt
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
Patch (196.78 KB, patch)
2011-09-15 13:33 PST, Dave Hyatt
bdakin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-01-10 07:44:04 PST -------
Created an attachment (id=78399) [details]
Patch
------- Comment #2 From 2011-01-11 13:02:11 PST -------
(From update of attachment 78399 [details])
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 From 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 From 2011-01-11 13:33:31 PST -------
(From update of attachment 78399 [details])
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 From 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 From 2011-01-12 06:28:05 PST -------
Created an attachment (id=78685) [details]
Teaching RenderLayer about non-rectangular clip regions :)
------- Comment #7 From 2011-01-12 08:53:16 PST -------
(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.

To see this in your testcase, give .inner opacity for example.
------- Comment #8 From 2011-01-13 05:29:56 PST -------
(In reply to comment #7)
> (From update of attachment 78685 [details] [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 From 2011-02-03 01:11:15 PST -------
Created an attachment (id=81040) [details]
Patch
------- Comment #10 From 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 From 2011-04-01 23:48:24 PST -------
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 From 2011-04-09 18:42:55 PST -------
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 From 2011-04-10 10:03:07 PST -------
*** Bug 41356 has been marked as a duplicate of this bug. ***
------- Comment #14 From 2011-06-04 14:39:30 PST -------
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 From 2011-06-22 15:42:24 PST -------
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 From 2011-09-15 12:24:40 PST -------
Created an attachment (id=107529) [details]
Patch that fixes the bug.
------- Comment #17 From 2011-09-15 12:28:20 PST -------
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 From 2011-09-15 12:29:21 PST -------
(From update of attachment 107529 [details])
Didn't mean to flag. Just showing the approach.
------- Comment #19 From 2011-09-15 13:33:00 PST -------
Created an attachment (id=107541) [details]
Patch
------- Comment #20 From 2011-09-15 13:35:37 PST -------
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 From 2011-09-15 16:49:30 PST -------
Fixed in r95239.
------- Comment #22 From 2011-09-17 18:23:44 PST -------
This appears to have caused the web process to get in to an infinite loop in some cases.  See bug 68314.
------- Comment #23 From 2011-09-17 19:20:20 PST -------
(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 From 2011-09-23 13:46:53 PST -------
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 From 2011-09-23 14:06:28 PST -------
(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 From 2011-09-23 14:35:32 PST -------
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 From 2011-09-23 14:43:34 PST -------
Filed 

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

to cover the regression.
------- Comment #28 From 2011-09-26 16:04:48 PST -------
Is this not a duplicate of 9543?
------- Comment #29 From 2011-09-26 16:09:58 PST -------
*** Bug 9543 has been marked as a duplicate of this bug. ***