Bug 23882 - GraphicsContextSkia draws round rects as solid rects
Summary: GraphicsContextSkia draws round rects as solid rects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Scott Violet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-10 16:44 PST by Scott Violet
Modified: 2009-02-11 14:01 PST (History)
1 user (show)

See Also:


Attachments
Fix for 23882 (2.27 KB, patch)
2009-02-11 09:13 PST, Scott Violet
eric: review-
Details | Formatted Diff | Diff
Fix for 23882 (2.36 KB, patch)
2009-02-11 11:13 PST, Scott Violet
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Violet 2009-02-10 16:44:52 PST
This can be seen with the test LayoutTests/fast/css/shadow-multiple.html .
Comment 1 Scott Violet 2009-02-11 08:15:26 PST
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.
Comment 2 Scott Violet 2009-02-11 09:13:31 PST
Created attachment 27563 [details]
Fix for 23882
Comment 3 Brett Wilson (Google) 2009-02-11 09:45:46 PST
> 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.
Comment 4 Scott Violet 2009-02-11 09:54:04 PST
(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.
Comment 5 Eric Seidel (no email) 2009-02-11 10:37:16 PST
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.
Comment 6 Scott Violet 2009-02-11 11:06:09 PST
(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!
Comment 7 Scott Violet 2009-02-11 11:13:19 PST
Created attachment 27566 [details]
Fix for 23882

Improves comment and changes style.
Comment 8 Eric Seidel (no email) 2009-02-11 13:41:08 PST
Comment on attachment 27566 [details]
Fix for 23882

Looks good. will land.
Comment 9 Eric Seidel (no email) 2009-02-11 14:01:24 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
Committed r40869