Draw rectangular box shadow for elements with border-radius if no corners are visible

[Ian Henderson]

Drawing shadows of rectangular shapes is often much easier for the underlying graphics framework to optimize.  To this end, we can use the unroundClippedCorners function from 64584 to remove invisible corners before drawing an element's shadow.

[Ian Henderson]

Created attachment 101692 [details]
proposed patch

[Ian Henderson]

Created attachment 101693 [details]
test case

Attached a test case.

[WebKit Review Bot]

Comment on attachment 101692 [details]
proposed patch

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

[Simon Fraser (smfr)]

Comment on attachment 101692 [details]
proposed patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2174
> +    unroundClippedCorners(border, info.rect);

I don't think this is right. The effect of the corner radius can propagate further for the shadow than the corner radius itself; you have to add the blur radius (or maybe twice the blur radius).

[Simon Fraser (smfr)]

In ShadowBlur, computeSliceSizesFromRadii(), we use corner radius + 2 * blur radius as the "area of influence" of the corner in the shadow.

[Ian Henderson]

Created attachment 101803 [details]
proposed patch

[Ian Henderson]

Created attachment 101804 [details]
proposed patch

svn up!

[WebKit Review Bot]

Comment on attachment 101804 [details]
proposed patch

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

[Ian Henderson]

Created attachment 101925 [details]
proposed patch

Slightly updated patch -- add 2 * blur radius to the influence rect's corners, which moves them further towards the middle of the element's border.  Before, the influence rect only extended outwards by the blur radius, but the corners' presence can affect the shadows near the straight section of the border as well.

[WebKit Review Bot]

Comment on attachment 101925 [details]
proposed patch

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

[Ian Henderson]

I'm a bit confused about why cr-linux doesn't like these patches.  The error is:

/WebCore/rendering/RenderBoxModelObject.cpp: In member function 'void WebCore::RenderBoxModelObject::paintBoxShadow(const WebCore::PaintInfo&, const WebCore::LayoutRect&, const WebCore::RenderStyle*, WebCore::ShadowStyle, bool, bool)':
Source/WebCore/rendering/RenderBoxModelObject.cpp:2249: error: 'cornersIntersect' was not declared in this scope

but that function is defined in the patch, and the other ews bots don't seem to have any problem seeing it…

[Ian Henderson]

Created attachment 102557 [details]
proposed patch

This is the same patch as the last one, but updated to ToT.  I'd like to make sure the cr-linux issue was not a transient one.

[WebKit Review Bot]

Comment on attachment 102557 [details]
proposed patch

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

[Ian Henderson]

Created attachment 102703 [details]
proposed patch

Move cornersIntersect() outside the #if HAVE(PATH_BASED_BORDER_RADIUS_DRAWING) block, since paintBoxShadow calls the function outside the #if block.

[Simon Fraser (smfr)]

Comment on attachment 102703 [details]
proposed patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1132
> +static bool cornersIntersect(const RoundedRect& border, const LayoutRect& clipRect)

I don't think the name of this function is quite right. It's asking whether any of the rounded corners fall inside the clipRect. You could call it isAnyCornerVisible() or something. Or you could flip the logic around and call it allCornersClippedOut().

> LayoutTests/platform/mac/fast/box-shadow/box-shadow-clipped-slices-expected.txt:4
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600

You should make this a dumpAsText(true). Where are the pixel results? If the pixel result includes a scrollbar, you should refactor the test to avoid this.

[Ian Henderson]

Created attachment 102729 [details]
proposed patch

New patch to address Simon's comments.

[WebKit Review Bot]

Comment on attachment 102729 [details]
proposed patch

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

New failing tests:
fast/box-shadow/box-shadow-clipped-slices.html

[Simon Fraser (smfr)]

Comment on attachment 102729 [details]
proposed patch

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

> LayoutTests/fast/box-shadow/box-shadow-clipped-slices.html:9
> +</style></head><body>

No need to put all the properties on one line here.

I don't quite get the point of this test. If not all of the test divs are visible in the pixel result, there's no point having them all.

[Ian Henderson]

Created attachment 102751 [details]
illustration of the layout test

(In reply to comment #18)
> (From update of attachment 102729 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102729&action=review
> 
> > LayoutTests/fast/box-shadow/box-shadow-clipped-slices.html:9
> > +</style></head><body>
> 
> No need to put all the properties on one line here.
> 
> I don't quite get the point of this test. If not all of the test divs are visible in the pixel result, there's no point having them all.

All of the test divs are visible in the pixel result.  The idea is to clip the shadow to many small regions to ensure the drawing matches up correctly.  I attached an image of the test with a red border around each div to illustrate what's going on.

[Joseph Pecoraro]

Landed in r92340 <http://trac.webkit.org/changeset/92340>

[Adam Barth]

Is this change in image results a regression?

http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win/results/layout-test-results/fast/box-shadow/box-shadow-clipped-slices-expected.png
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win/results/layout-test-results/fast/box-shadow/box-shadow-clipped-slices-actual.png
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win/results/layout-test-results/fast/box-shadow/box-shadow-clipped-slices-diff.png

It certainly looks uglier...

[Ian Henderson]

(In reply to comment #21)
> Is this change in image results a regression?
> 
> http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win/results/layout-test-results/fast/box-shadow/box-shadow-clipped-slices-expected.png
> http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win/results/layout-test-results/fast/box-shadow/box-shadow-clipped-slices-actual.png
> http://build.chromium.org/f/chromium/layout_test_results/Webkit_Win/results/layout-test-results/fast/box-shadow/box-shadow-clipped-slices-diff.png
> 
> It certainly looks uglier...

I added that test in this commit with a mac baseline for the pixel results.  It looks like the diff is between the mac baseline and the Chromium pixel results.

[Adam Barth]

> I added that test in this commit with a mac baseline for the pixel results.  It looks like the diff is between the mac baseline and the Chromium pixel results.

Ok, so you're saying that the Chromium pixel result is ugly but not wrong.

[Ian Henderson]

(In reply to comment #23)
> > I added that test in this commit with a mac baseline for the pixel results.  It looks like the diff is between the mac baseline and the Chromium pixel results.
> 
> Ok, so you're saying that the Chromium pixel result is ugly but not wrong.

I don't know if it's wrong or not -- I'm just saying it's not a regression.

[Simon Fraser (smfr)]

Maybe it's a Skia bug.