Bug 87859 – Bug: Negative SVG rect rx,ry corner radii values aren't handled correctly

• Component: SVG
• Status: RESOLVED FIXED

[Hans Muller]

Created attachment 144834 [details]
HTML file contains failing SVG rect negative rx,ry test cases

The SVG specification says that negative values for rx,ry "rect" corner radii aren't allowed and that if "if neither rx nor ry are properly specified, then set both rx and ry to 0. (This will result in square corners.)" - http://www.w3.org/TR/SVG/shapes.html#RectElement. Similarly, if only one of rx,ry is negative then the other attribute (default 0) should be used for both.

The current implementation fails in all three cases:

<rect x="100" y="100" width="200" height="100" rx="-90" ry="-40"/>
<rect x="100" y="100" width="200" height="100" rx="-90" ry="40"/>
<rect x="100" y="100" width="200" height="100" rx="90" ry="-40"/>

[Hans Muller]

Created attachment 144836 [details]
Screenshot of the failing test cases.

[Rob Buis]

Created attachment 145110 [details]
Patch

[Hans Muller]

I'd assigned the bug to myself because I had a fix for it.   I'd put the changes in Path::addRoundedRect(), where the other checks for the corner constraints from the SVG spec are already.

[Hans Muller]

Not to put too fine a point on it, but here's the updated version of addRoundedRect().   I'd merged the original comments about the SVG spec with a new one.


void Path::addRoundedRect(const FloatRect& rect, const FloatSize& roundingRadii, RoundedRectStrategy strategy)
{
    if (rect.isEmpty())
        return;

    FloatSize radius(roundingRadii);
    FloatSize halfSize(rect.width() / 2, rect.height() / 2);

    // Apply the SVG corner radius constraints, per the rect section of the SVG shapes spec: if
    // one of rx,ry is negative, then the other corner radius value is used.  If both values are 
    // negative then rx = ry = 0.  If rx is greater than half of the width of the rectangle
    // then set rx to half of the width; ry is handled similarly.

    if (radius.width() < 0)
        radius.setWidth((radius.height() < 0) ? 0 : radius.height());

    if (radius.height() < 0)
        radius.setHeight(radius.width());

    if (radius.width() > halfSize.width())
        radius.setWidth(halfSize.width());

    if (radius.height() > halfSize.height())
        radius.setHeight(halfSize.height());

    addPathForRoundedRect(rect, radius, radius, radius, radius, strategy);
}

[Rob Buis]

Hi,

(In reply to comment #3)
> I'd assigned the bug to myself because I had a fix for it.   I'd put the changes in Path::addRoundedRect(), where the other checks for the corner constraints from the SVG spec are already.

I am sorry, totally missed the assigned! We are still discussing on IRC what is the best place to fix this.

[Hans Muller]

Created attachment 145163 [details]
Patch

[Hans Muller]

I've uploaded a revised patch per the krit,rbuis IRC discussion.  The reftest is essentially the same as the one Rob provided, modulo a little gratuitous reformatting.

[Dirk Schulze]

Comment on attachment 145163 [details]
Patch

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

Looks good to me. Just snippets to get r+.

> LayoutTests/svg/custom/rect-negative-corner-radii-expected.svg:2
> +<svg xmlns:svg="http://www.w3.org/2000/svg" xmlns="http://www.w3.org/2000/svg" version="1.0" width="500" height="500">

xmlns:svg="http://www.w3.org/2000/svg" Is unnecessary.

> LayoutTests/svg/custom/rect-negative-corner-radii.svg:2
> +<svg xmlns:svg="http://www.w3.org/2000/svg" xmlns="http://www.w3.org/2000/svg" version="1.0" width="500" height="500">

Ditto.

[Hans Muller]

Created attachment 145170 [details]
Patch

[Hans Muller]

Removed redundant namespace attributes from the reftest.

[WebKit Review Bot]

Comment on attachment 145170 [details]
Patch

Clearing flags on attachment: 145170

Committed r119182: <http://trac.webkit.org/changeset/119182>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.