webkit fails IETC border-radius-content-edge-001

• Component: CSS
• Status: RESOLVED DUPLICATE

[Dave Tharp]

No red should be visible on the rendered page.

[Takashi Sakamoto]

Created attachment 143196 [details]
Patch

[WebKit Review Bot]

Comment on attachment 143196 [details]
Patch

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

New failing tests:
ietestcenter/css3/bordersbackgrounds/border-radius-content-edge-001.htm

[WebKit Review Bot]

Created attachment 143223 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Comment on attachment 143196 [details]
Patch

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

I think it's worth having another test which written for an additional coverage.
And we need to add an expectation image, at least for one platform.

> Source/WebCore/rendering/RenderImage.cpp:443
> +        context->addRoundedRectClip(clipRect);

We need to push/pop the context to cancel the clipping after the rendering.

[Takashi Sakamoto]

Created attachment 144021 [details]
Patch

[Julien Chaffraix]

Comment on attachment 144021 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        care of any radii's scale. So it probably causes to shrink the radii too much.

Some of the images have some red pixels, is this the cause for that? Shouldn't you solve this issue too instead of just mentioning it here?

> Source/WebCore/rendering/RenderImage.cpp:439
> +        LayoutUnit leftWidth = style()->borderLeftWidth();
> +        LayoutUnit rightWidth = style()->borderRightWidth();
> +        LayoutUnit topWidth = style()->borderTopWidth();
> +        LayoutUnit bottomWidth = style()->borderBottomWidth();

Borders are unsigned, please avoid those unneeded conversions to LayoutUnit. Also let's avoid abbreviation, those are border width, not just width (which can be anything)

> Source/WebCore/rendering/RenderImage.cpp:441
> +        LayoutRect rectWithBorder(rect.x() - leftWidth, rect.y() - topWidth,
> +                                  rect.width() + leftWidth + rightWidth, rect.height() + topWidth + bottomWidth);

It would be neat to add a test for vertical writing mode (unless already covered). This should work as painting assumes rectangles in device coordinates and not logical ones.

> Source/WebCore/rendering/RenderImage.cpp:444
> +            rectWithBorder.move(-paddingLeft(), -paddingTop());
> +            rectWithBorder.expand(paddingLeft() + paddingRight(), paddingTop() + paddingBottom());

rectWithBorder.inflate(paddingLeft(), paddingTop());
rectWithBorder.expand(paddingRight(), paddingBottom());

> Source/WebCore/rendering/RenderImage.cpp:450
> +            clipRect.setRect(alignedRect);

As a whole, we do a lot of recurrent computation here (shrink / expanding to account for paddings or margin). I don't know of any smarter way of doing it but maybe someone else may. At least exposing a way of expanding a rect using a LayoutBoxExtend and adding the ad-hoc getter in RenderStyle should reduce the risk of error.

[Takashi Sakamoto]

Created attachment 147786 [details]
Patch

[Takashi Sakamoto]

Created attachment 147792 [details]
Patch

[Takashi Sakamoto]

Thank you for reviewing.

(In reply to comment #6)
> (From update of attachment 144021 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144021&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        care of any radii's scale. So it probably causes to shrink the radii too much.
> 
> Some of the images have some red pixels, is this the cause for that? Shouldn't you solve this issue too instead of just mentioning it here?

Talking about the test: fast/borders/border-radius-image.html, the left box shows how img works with no border-radius. So it should have red pixels.
This border-radius-image is for testing three cases which are not checked by ietestcenter's test:
(1) check this patch doesn't affect img rendering if no border-radius.
(2) check this patch correctly treats img with rounded border. The border's left width, right width, top width and bottom width are not the same.
(3) check this patch correctly treats img with no padding and with rounded border.

Firstly I think, I need only border-radius-content-edge-001.htm. However, I found that I created new bugs while creating this patch. So I think, I need these three test cases.

Talking about ietestcenter/css3/borderbackgrounds/border-radius-content-edge-001.htm, there are still small red pixels left on rounded corners. However, I compared the result with Firefox's one and found that Firefox also shows small red pixels.

> > Source/WebCore/rendering/RenderImage.cpp:439
> > +        LayoutUnit leftWidth = style()->borderLeftWidth();
> > +        LayoutUnit rightWidth = style()->borderRightWidth();
> > +        LayoutUnit topWidth = style()->borderTopWidth();
> > +        LayoutUnit bottomWidth = style()->borderBottomWidth();
> 
> Borders are unsigned, please avoid those unneeded conversions to LayoutUnit. Also let's avoid abbreviation, those are border width, not just width (which can be anything)
> 
> > Source/WebCore/rendering/RenderImage.cpp:441
> > +        LayoutRect rectWithBorder(rect.x() - leftWidth, rect.y() - topWidth,
> > +                                  rect.width() + leftWidth + rightWidth, rect.height() + topWidth + bottomWidth);
> 
> It would be neat to add a test for vertical writing mode (unless already covered). This should work as painting assumes rectangles in device coordinates and not logical ones.
> 
> > Source/WebCore/rendering/RenderImage.cpp:444
> > +            rectWithBorder.move(-paddingLeft(), -paddingTop());
> > +            rectWithBorder.expand(paddingLeft() + paddingRight(), paddingTop() + paddingBottom());
> 
> rectWithBorder.inflate(paddingLeft(), paddingTop());
> rectWithBorder.expand(paddingRight(), paddingBottom());

I think, inflate does't work as I expected. I mean,

inflateX(dx) increases size by dx * 2:
  dx <----|---- original size ---| ----> dx
        
What I want to do is
  paddingLeft <----|---- original size ----|-----> paddingRight


> > Source/WebCore/rendering/RenderImage.cpp:450
> > +            clipRect.setRect(alignedRect);
> 
> As a whole, we do a lot of recurrent computation here (shrink / expanding to account for paddings or margin). I don't know of any smarter way of doing it but maybe someone else may. At least exposing a way of expanding a rect using a LayoutBoxExtend and adding the ad-hoc getter in RenderStyle should reduce the risk of error.

I see. I modified to use LayoutBoxExtent.

Best regards,
Takashi Sakamoto

[Takashi Sakamoto]

Created attachment 155778 [details]
Patch

[WebKit Review Bot]

Comment on attachment 155778 [details]
Patch

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

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

[WebKit Review Bot]

Created attachment 155784 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Julien Chaffraix]

Comment on attachment 155778 [details]
Patch

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

r- mostly for tripping between logical and physical coordinate. Could you explain why you can't extend the existing helper functions (getRounderInnerBorderFor and al.) for your need?

> Source/WebCore/ChangeLog:10
> +        clipping if an image has border-radius. I compared the below test's
> +        result with a result obtained by using Firefox.

And what was the result of the comparison?

> Source/WebCore/ChangeLog:20
> +        doesn't include any padding), the radii's scale of the calculrated

typo: calculated.

> Source/WebCore/ChangeLog:24
> +        Talking about the above test, the scale is 0.96 < 1.0. However
> +        radii.shrink() doesn't take care of any radii's scale. So it probably
> +        causes to shrink the radii too much.

This probably should go in the LayoutTest ChangeLog as it describes why the image is failing. Please file a bug about that before landing and mention that it is covered by this bug in the ChangeLog.

> Source/WebCore/rendering/RenderImage.cpp:450
> +    GraphicsContextStateSaver clipToBorderStateSaver(*context, hasRoundedBorder);

This should be moved inside the if below and you can remove the |hasRoundedBorder| check.

> Source/WebCore/rendering/RenderImage.cpp:454
> +        rectWithBorder.move(-borderLeft(), -borderTop());
> +        rectWithBorder.expand(borderLeft() + borderRight(), borderTop() + borderBottom());

I feel like you are fighting the API here. getRoundedInnerBorderFor will re-add the borders back in the case where no padding was set on your image. My bet is that you can just pass the args as |false| but nothing prevents you from actually making a variant that works in your case, instead of implementing everything inline here.

> Source/WebCore/rendering/RenderImage.cpp:461
> +            radii.shrink(paddingBorderBox.before(style()), paddingBorderBox.after(style()), paddingBorderBox.start(style()), paddingBorderBox.end(style()));

This line is wrong. You need to use the physical coordinates (top / left / ...) not the logical one (before / after / ...). You would see that if you had a test case involving some vertical writing-mode, which I already advised you to add but you missed the cue.

The easiest way is to add a new class to your test similar to #border_radius_padding but which has: -webkit-writing-mode: vertical-rl;

> LayoutTests/ChangeLog:16
> +        * platform/chromium-win/ietestcenter/css3/bordersbackgrounds/border-radius-content-edge-001-expected.png: Removed.

Note that Qt also has a pixel baseline for this test which we will need to update. If possible, it's better before if not you will have to coordinate with some people to avoid breaking Qt.

> LayoutTests/fast/borders/border-radius-image.html:27
> +<body>

A test should explain what you are trying to achieve. Here it's extremely difficult to do so.

Some questions that should be answered:
* What is expected here?
* What are you even testing? 

As you are using a pixel test, you don't want that to be dumped as text in the image. There are several tricks on http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Pixeltesttips on how to do that.

[Takashi Sakamoto]

Created attachment 156019 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 155778 [details]
Patch

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

Thank you for reviewing. I added a new method by extending an existing getRoundedBorderFor.

>> Source/WebCore/ChangeLog:10
>> +        result with a result obtained by using Firefox.
> 
> And what was the result of the comparison?

I see. I added "the result was the same".

>> Source/WebCore/ChangeLog:20
>> +        doesn't include any padding), the radii's scale of the calculrated
> 
> typo: calculated.

Done.

>> Source/WebCore/ChangeLog:24
>> +        causes to shrink the radii too much.
> 
> This probably should go in the LayoutTest ChangeLog as it describes why the image is failing. Please file a bug about that before landing and mention that it is covered by this bug in the ChangeLog.

This is caused by fighting APIs out of RenderStyle.cpp. I removed inline fighting codes and added a new API for getting rounded rect for content. So I removed this comment.

>> Source/WebCore/rendering/RenderImage.cpp:450
>> +    GraphicsContextStateSaver clipToBorderStateSaver(*context, hasRoundedBorder);
> 
> This should be moved inside the if below and you can remove the |hasRoundedBorder| check.

I think, GraphicsContextStateSaver rollbacks the rounded clipping in its destructor. So I have to place the GraphicsContext in the same level as  drawImage.

>> Source/WebCore/rendering/RenderImage.cpp:454
>> +        rectWithBorder.expand(borderLeft() + borderRight(), borderTop() + borderBottom());
> 
> I feel like you are fighting the API here. getRoundedInnerBorderFor will re-add the borders back in the case where no padding was set on your image. My bet is that you can just pass the args as |false| but nothing prevents you from actually making a variant that works in your case, instead of implementing everything inline here.

I added a new method, getRoundedContentFor.

>> Source/WebCore/rendering/RenderImage.cpp:461
>> +            radii.shrink(paddingBorderBox.before(style()), paddingBorderBox.after(style()), paddingBorderBox.start(style()), paddingBorderBox.end(style()));
> 
> This line is wrong. You need to use the physical coordinates (top / left / ...) not the logical one (before / after / ...). You would see that if you had a test case involving some vertical writing-mode, which I already advised you to add but you missed the cue.
> 
> The easiest way is to add a new class to your test similar to #border_radius_padding but which has: -webkit-writing-mode: vertical-rl;

I see. I added new tests for checking vertical writing-mode.

>> LayoutTests/fast/borders/border-radius-image.html:27
>> +<body>
> 
> A test should explain what you are trying to achieve. Here it's extremely difficult to do so.
> 
> Some questions that should be answered:
> * What is expected here?
> * What are you even testing? 
> 
> As you are using a pixel test, you don't want that to be dumped as text in the image. There are several tricks on http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree#Pixeltesttips on how to do that.

I see. I added descriptions about these tests. As I would like to use only pixel test, I added the descriptions as comments and added dumpAsText(true).

[WebKit Review Bot]

Comment on attachment 156019 [details]
Patch

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

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

[WebKit Review Bot]

Created attachment 156029 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Eric Seidel (no email)]

I believe Beth is the last person to do serious work in the rounded border code.

*** This bug has been marked as a duplicate of bug 63899 ***