Bug 23882 – GraphicsContextSkia draws round rects as solid rects

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Scott Violet]

This can be seen with the test LayoutTests/fast/css/shadow-multiple.html .

[Scott Violet]

Once this line is fixed, the layout test LayoutTests/fast/box-shadow/basic-shadows.html starts failing. This is because we need to check if the width/height of the rounded rect is greater than the width/height of the rounded rect. If it is, then we should just draw a rect.

[Scott Violet]

Created attachment 27563 [details]
Fix for 23882

[Brett Wilson (Google)]

> if the total radius along a given axis is greater than the size of
> the axis to draw, a solid rect should be drawn.

I assume this is the behavior of CG? It seems weird that it would do this for the entire rect if just a single corner is "too smooth." What does the Skia output look like in this case without your change? It's possible this is an aspect of CG we don't want to emulate.

[Scott Violet]

(In reply to comment #3)
> > if the total radius along a given axis is greater than the size of
> > the axis to draw, a solid rect should be drawn.
> 
> I assume this is the behavior of CG?

Yes. This behavior can be seen with the layout test LayoutTests/fast/box-shadow/border-radius-big.html .

> It seems weird that it would do this for
> the entire rect if just a single corner is "too smooth." What does the Skia
> output look like in this case without your change?

Experimentation shows it does this for the whole rect, not just the corner(s).

> It's possible this is an aspect of CG we don't want to emulate.

I don't have a strong feeling here. This is an edge case that I can't imagine comes up much. But I do tend to think Skia should match CG as close as possible.

[Eric Seidel (no email)]

Comment on attachment 27563 [details]
Fix for 23882

This block still doesn't make sense to me, even after reading the bug:

+    if (topLeft.width() + topRight.width() > rect.width() ||
+        bottomLeft.width() + bottomRight.width() > rect.width() ||
+        topLeft.height() + bottomLeft.height() > rect.height() ||
+        topRight.height() + bottomRight.height() > rect.height()) {
+        // If the width/height along an axis is > than the width/height of the
+        // rectangle, just draw a rect.
+        fillRect(rect, color);
+        return;
+    }

i.e. What behavior does that change?  Why are we changing it (to match CG's behavior it seems).  What is "width/height long an axis"
Seems we need a better comment there to make more sense to the casual reader (who knows less about our graphics code than even I do).

Otherwise this change looks great.

[Scott Violet]

(In reply to comment #5)
> (From update of attachment 27563 [details] [review])
> This block still doesn't make sense to me, even after reading the bug:
> 
> +    if (topLeft.width() + topRight.width() > rect.width() ||
> +        bottomLeft.width() + bottomRight.width() > rect.width() ||
> +        topLeft.height() + bottomLeft.height() > rect.height() ||
> +        topRight.height() + bottomRight.height() > rect.height()) {
> +        // If the width/height along an axis is > than the width/height of the
> +        // rectangle, just draw a rect.
> +        fillRect(rect, color);
> +        return;
> +    }
> 
> i.e. What behavior does that change?

This effectively gives us the old behavior (of always filling a rect) if the sum of the radii along an axis are bigger than the size of the rect. This method (fillRoundedRect) is invoked to draw the shadow. The method that draws the background uses Path::createRoundedRectangle to create the path for the background. Path::createRoundedRectangle creates a rectangle under the same conditions:

    if (width < topLeftRadius.width() + topRightRadius.width()
            || width < bottomLeftRadius.width() + bottomRightRadius.width()
            || height < topLeftRadius.height() + bottomLeftRadius.height()
            || height < topRightRadius.height() + bottomRightRadius.height())
        // If all the radii cannot be accommodated, return a rect.
        return createRectangle(rectangle);

If fillRoundedRect isn't updated like this it means we draw a round shadow for a solid background.

> Why are we changing it (to match CG's
> behavior it seems).  What is "width/height long an axis"
> Seems we need a better comment there to make more sense to the casual reader
> (who knows less about our graphics code than even I do).

My comment is wrong. I'll update it (along with a style change) and attach a new patch.

Thanks!

[Scott Violet]

Created attachment 27566 [details]
Fix for 23882

Improves comment and changes style.

[Eric Seidel (no email)]

Comment on attachment 27566 [details]
Fix for 23882

Looks good. will land.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
Committed r40869