Bug 63899 – The content of replaced element 'img' is trimmed to the content edge curve of a rounded corner.

• Component: CSS
• Status: RESOLVED FIXED

[Alexandru Chiculita]

The border should display on top of the content.

[Alexandru Chiculita]

Created attachment 99619 [details]
Patch

There seem to be two issues here:
1. The test assumes that the border radius should also apply to the content rectangle. I couldn't find a reason for that in the specification and I didn't fix that in this patch.
2. The content is clipped only to the outer path of the border. The patch fixes that by using the inner path of the border, so that the border always render on top of the image.

[Alexandru Chiculita]

Created attachment 99620 [details]
Test

Extracted the test page from the previous page for easier preview.

[WebKit Review Bot]

Comment on attachment 99619 [details]
Patch

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

New failing tests:
fast/css/border-radius-clip.html

[WebKit Review Bot]

Created attachment 99621 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexandru Chiculita]

(In reply to comment #4)

My test failed because I just added expected results on Mac.

[Hajime Morrita]

(In reply to comment #5)
> (In reply to comment #4)
> 
> My test failed because I just added expected results on Mac.
Could you add an entry for test expectation file?
http://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium/test_expectations.txt
Marking it FAIL should be sufficient.

The fix itself looks obvious.

[Alexandru Chiculita]

(In reply to comment #6)

Should I try to generate the results for the Chromium platform? How does this usually work?

[Hajime Morrita]

(In reply to comment #7)
> (In reply to comment #6)
> 
> Should I try to generate the results for the Chromium platform? How does this usually work?
No, you don't need to do it.
What Chromium team want is just marking it on test_expectations.txt.
Marked tests are known to fail and bots stay green even if it failed.

[Alexandru Chiculita]

Created attachment 99816 [details]
Patch

Added expected FAIL on chrome.

[Alexandru Chiculita]

Created attachment 99817 [details]
Patch

Selected the correct patch file this time.

[Hajime Morrita]

Could you give more test coverage?
It would be like:

border size variations:
- border-radius which has 4 different sizes
- zero border-radius
- border-radius whose size is larger than half of the content size

border width variations:
- zero
- larger than half of the content size

element type variations:
- How about <canvas>?

Multiple cases can be stayed in same file
if its grouping is appropriate.

I know current lack of coverage isn't your fault.
But i think it's a good opportunity to expand it.
And higher coverage gives reviewers higher confidence ;-)

[Hajime Morrita]

Comment on attachment 99817 [details]
Patch

r- for now to sweep the pending queue.

[Alexandru Chiculita]

Created attachment 101151 [details]
Patch

Added new tests for the following:

- border-radius which has 4 different sizes
- border-radius whose size is larger than half of the content size
- zero border
- larger than half of the content size
- <canvas>

Didn't add a test for border-radius: 0px, because there's no clipping involved in that case (and I think it's already covered by other tests).

[Benjamin Poulain]

That is an interesting case, but I am wondering if the inner border radius will not give us wrong pixels values on some pixels due to:
-antialiazed clipping (the background color could blend with some pixels values)
-the "rounded" algorithm differing for clipping the inner circle, and the innermost pixels of the border. (I remember issues on X11 with the algorithm for fill not matching the algorithm for border so the 2 can never match pixel to pixel).

My first idea seeing the test case was:
-the clip should be in the middle of the border (0.5 pixels for a 1 pixel border) so that the antializing of the clip does not gives us bad value. If there is padding and the padding is supposed to clip, the clip in that case would be the inner border - padding.
-the border should be on top of the rendering and not done before (I don't remember what the spec says about that in the rendering phases...)

I see the original test include padding. I think a second test with padding is necessary here since that is an interesting variation on the rendering of this case.

[Alexandru Chiculita]

(In reply to comment #14)

I agree anti-aliasing issues may appear. I will add a new patch that will render the border on top of the content. The only spec I found about clipping the border radius is this paragraph: http://www.w3.org/TR/css3-background/#corner-clipping . It says that the content of the render replaced is always clipped by the border. I'm not sure what should happen if border color has 0.5 alpha. Should the content be totally clipped or not?

> I see the original test include padding. I think a second test with padding is necessary here since that is an interesting variation on the rendering of this case.

I will add the test, but I think the original test case is wrong. It seems like the original test case expected to apply the border-radius to the content of the element, even though the padding was big enough, so that the border didn't have to clip the content. Following that paragraph in the spec, it's not clear which one is the right behavior.

[Benjamin Poulain]

(In reply to comment #15)
> I agree anti-aliasing issues may appear. I will add a new patch that will render the border on top of the content. The only spec I found about clipping the border radius is this paragraph: http://www.w3.org/TR/css3-background/#corner-clipping . It says that the content of the render replaced is always clipped by the border. I'm not sure what should happen if border color has 0.5 alpha. Should the content be totally clipped or not?

Excellent point, a border with an alpha != 1 would make another interesting test case. :)

I add Simon Fraser and Dan Bernstein in CC. They might have insight on what the correct behavior should be.

[Simon Fraser (smfr)]

Comment on attachment 101151 [details]
Patch

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

> LayoutTests/fast/css/border-radius-clip.html:5
> +        border: 10px solid rgba(0, 255, 0, 1);

Please just use 'green' for all these.

[Alexandru Chiculita]

Created attachment 101161 [details]
Patch

I'm not a committer, changed the test case. Can you please set the commit queue flag?

What about the other issues on this bug? Should I open a different bug for them?

[Simon Fraser (smfr)]

Comment on attachment 101161 [details]
Patch

You need to update the pixel results.

[Simon Fraser (smfr)]

Comment on attachment 101161 [details]
Patch

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

> LayoutTests/fast/css/border-radius-clip.html:25
> +<p>Test passes if there is no red visible on the page.</p>

Please change this to an HTML comment to avoid text in the pixel result.

[Simon Fraser (smfr)]

Actually I think this patch will do the wrong thing if the border has transparency.

[Alexandru Chiculita]

(In reply to comment #21)
> Actually I think this patch will do the wrong thing if the border has transparency.

The correct behavior seems to be changing the order the decorations are painted:

1. Paint the background using the outer border line clipping
2. Paint the replaced content using the outer border line clipping (minus one device pixel if the border is opaque, so that we avoid bleeding outside the edge)
3. Paint the border

The spec says the rule also applies for blocks and tables that have overflow != 'visible' . So step 2 should also apply for that.

Is this right? If yes, it requires splitting the paintBoxDecorations method in two: one for backgrund and one for borders. I also need to update all the call-sites to call both methods.

[Simon Fraser (smfr)]

Where does the spec say that replaced elements paint under the border?

[Alexandru Chiculita]

(In reply to comment #23)
> Where does the spec say that replaced elements paint under the border?

There is just a small paragraph in the spec, saying that the content of the replaced elements is clipped by the curve of the border. It doesn't say which curve, the internal or the external. http://www.w3.org/TR/css3-background/#corner-clipping 

I think I misunderstood comment 21. I just thought the right behavior will be to render the border on top of the content. Can you please explain why the patch will be bad for borders with transparency? One issue is anti-aliasing of the colors on the edges. That's because of different algorithms and rounding errors. However, that seems to be general, not just for the transparency case.

[Simon Fraser (smfr)]

The real question is what your test case should look like if the border has alpha. Can you attach the test, and test other browsers?

[Alexandru Chiculita]

Created attachment 101862 [details]
Results and test

Added test and results for transparent border.

Two browsers render exactly the same as would WebKit render with the patch applied.
Win7 - IE10 - Beta2
Mac10.6 - Firefox 5.0.1

Also added screenshot for WebKit on MacOSX10.6 on Safari with the patch applied.

Opera does not apply the clipping at all.
Mac10.6 - Opera 11.5

Just for reference, also tested the following WebKit based browsers.
Mac10.6 - Chrome12
Mac10.6 - Safari 5.0.3

[Eric Seidel (no email)]

Comment on attachment 101151 [details]
Patch

Cleared Simon Fraser's review+ from obsolete attachment 101151 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 169075 [details]
Patch

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

[Simon Fraser (smfr)]

Comment on attachment 169075 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Previously we trimmed to the border edge curve.
> +        Spec: http://www.w3.org/TR/css3-background/#corner-clipping

This is slightly cryptic. A few more words here would be good.

> LayoutTests/fast/replaced/border-radius-clip-content-edge.html:36
> + <object>

The pixel results of this test would be more likely to be platform-independent if you didn't include the text.

Created attachment 169088 [details]
Patch for landing

Created attachment 169097 [details]
Patch for landing

needed new cr-linux baselines :(

[WebKit Review Bot]

Comment on attachment 169097 [details]
Patch for landing

Clearing flags on attachment: 169097

Committed r131557: <http://trac.webkit.org/changeset/131557>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Binyamin]

The issue still appears on position: absolute elements on pseudo-classes :hover, :focus, :active http://jsfiddle.net/A7MEe/show/

The issue reported also in http://code.google.com/p/chromium/issues/detail?id=71639#c47

[Mark Smits]

Another quick demo which shows the problem rises when changing properties.

http://jsfiddle.net/bhmka/1/

[Dominik Röttsches (drott)]

Created attachment 170912 [details]
Antialiasing against Red

In the baselines of this patch, this seems to antialias against red (background-color). Might be an issue. See attached screenshot.