Bug 51374 - Cairo's ContextShadow may mis-render some box shadows
Summary: Cairo's ContextShadow may mis-render some box shadows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-20 21:02 PST by Simon Fraser (smfr)
Modified: 2011-01-07 02:27 PST (History)
4 users (show)

See Also:


Attachments
Testcase (2.07 KB, text/html)
2010-12-20 21:03 PST, Simon Fraser (smfr)
no flags Details
Proposed patch (3.65 KB, patch)
2010-12-21 03:54 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (36.03 KB, patch)
2010-12-23 04:45 PST, Alejandro G. Castro
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-12-20 21:02:56 PST
After porting ContextShadow to Mac, I sees some combinations of border-radius and shadow blur that it mis-renders. Example coming.
Comment 1 Simon Fraser (smfr) 2010-12-20 21:03:40 PST
Created attachment 77081 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2010-12-20 21:22:24 PST
I think the top of ContextShadow::drawRectShadow() should read:


    float radiusTwice = m_blurDistance * 2;

    // Size of the tiling side.
    int sideTileWidth = 1;

    // Find the extra space needed from the curve of the corners.
    int extraWidthFromCornerRadii = radiusTwice + max(topLeftRadius.width(), bottomLeftRadius.width()) +
        radiusTwice + max(topRightRadius.width(), bottomRightRadius.width());
    int extraHeightFromCornerRadii = radiusTwice + max(topLeftRadius.height(), topRightRadius.height()) +
        radiusTwice + max(bottomLeftRadius.height(), bottomRightRadius.height());

    // The length of a side of the buffer is the enough space for four blur radii,
    // the radii of the corners, and then 1 pixel to draw the side tiles.
    IntSize shadowTemplateSize = IntSize(sideTileWidth + extraWidthFromCornerRadii,
                                         sideTileWidth + extraHeightFromCornerRadii);

    // drawShadowedRect still does not work with rotations.
    // https://bugs.webkit.org/show_bug.cgi?id=45042
    CGContextRef cgContext = context->platformContext();
    if ((!context->getCTM().isIdentityOrTranslationOrFlipped()) || (shadowTemplateSize.width() > rect.width())
        || (shadowTemplateSize.height() > rect.height()) || (m_type != BlurShadow)) {
        drawRectShadowWithoutTiling(cgContext, rect, topLeftRadius, topRightRadius, bottomLeftRadius, bottomRightRadius, 1);
        return;
    }
Comment 3 Alejandro G. Castro 2010-12-21 00:44:41 PST
(In reply to comment #1)
> Created an attachment (id=77081) [details]
> Testcase

Yep, it fails for me also in webkitgtk, we had solved this issues I guess in some of the refactorings before applying the patch we did not check it correctly.
Comment 4 Alejandro G. Castro 2010-12-21 03:54:28 PST
Created attachment 77099 [details]
Proposed patch
Comment 5 Alejandro G. Castro 2010-12-21 03:55:49 PST
(In reply to comment #2)
> I think the top of ContextShadow::drawRectShadow() should read:
> 
> [...]
>
>     if ((!context->getCTM().isIdentityOrTranslationOrFlipped()) || (shadowTemplateSize.width() > rect.width())
>         || (shadowTemplateSize.height() > rect.height()) || (m_type != BlurShadow)) {
>         drawRectShadowWithoutTiling(cgContext, rect, topLeftRadius, topRightRadius, bottomLeftRadius, bottomRightRadius, 1);
>         return;
>     }

You are right, the problem is the calculation of the internal space required for the corner shadows, I've refactored the code with your proposal in the patch. Thanks.
Comment 6 Martin Robinson 2010-12-21 07:50:48 PST
Comment on attachment 77099 [details]
Proposed patch

Great, though we should definitely include the test case with pixel results to avoid this in the future.
Comment 7 Alejandro G. Castro 2010-12-23 04:45:27 PST
Created attachment 77327 [details]
Patch
Comment 8 Martin Robinson 2010-12-23 07:14:36 PST
Comment on attachment 77327 [details]
Patch

Thank you!
Comment 9 Alejandro G. Castro 2011-01-07 02:27:09 PST
Landed http://trac.webkit.org/changeset/75237