[CSS Shapes][css clip-path] rounded corner calculation for box shapes is wrong

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Bem Jones-Bey]

The rounded corner calculation for boxes other than the border box needs to use the formulas in the spec (see URL).

Right now, it uses something else. This needs to be fixed in both RenderLayer::setupClipPath and ShapeInfo::computedShape.

[Radar WebKit Bug Importer]

<rdar://problem/15958752>

[Bem Jones-Bey]

Created attachment 225793 [details]
Patch

[Dirk Schulze]

Comment on attachment 225793 [details]
Patch

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

> Source/WebCore/rendering/LayoutBox.cpp:40
> +    LayoutUnit ratio = radius / margin;

No check for zero before?

> Source/WebCore/rendering/LayoutBox.cpp:42
> +        return radius + (margin * (1 + pow(ratio - 1, 3)));

The radius is clipped to max 50% already right? We do not run into problems because of pow? (Even if it is limited to cubic)

> LayoutTests/ChangeLog:7
> +
> +        [CSS Shapes][css clip-path] rounded corner calculation for box shapes is wrong
> +        https://bugs.webkit.org/show_bug.cgi?id=127982
> +
> +        Reviewed by NOBODY (OOPS!).
> +

It would be cool to have examples where the border radius is different for each corner.

> LayoutTests/css3/masking/clip-path-border-radius-border-box-000-expected.html:7
> +        border: 20px solid rgba(0, 0, 0, 0);

hehe, black would have been enough :D

[Simon Fraser (smfr)]

Comment on attachment 225793 [details]
Patch

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

>> Source/WebCore/rendering/LayoutBox.cpp:40
>> +    LayoutUnit ratio = radius / margin;
> 
> No check for zero before?

if margin is 0 this is a divide by zero.

[zalan]

Comment on attachment 225793 [details]
Patch

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

> Source/WebCore/rendering/LayoutBox.cpp:47
> +static inline LayoutSize computeMarginBoxRadius(LayoutSize radius, LayoutSize adjacentMargins)

These should be const LayoutSize&

[Bem Jones-Bey]

(In reply to comment #3)
> (From update of attachment 225793 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225793&action=review
> > Source/WebCore/rendering/LayoutBox.cpp:42
> > +        return radius + (margin * (1 + pow(ratio - 1, 3)));
> 
> The radius is clipped to max 50% already right? We do not run into problems because of pow? (Even if it is limited to cubic)

You're right, I should make sure they get scaled properly if they go over 50%. When looking into this, I realized that inset wasn't scaling at all for 50%, so I'll post a new patch here after my fix for Bug 129726 lands.

> > LayoutTests/ChangeLog:7
> > +
> > +        [CSS Shapes][css clip-path] rounded corner calculation for box shapes is wrong
> > +        https://bugs.webkit.org/show_bug.cgi?id=127982
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> It would be cool to have examples where the border radius is different for each corner.

I'll add some tests like that.

> 
> > LayoutTests/css3/masking/clip-path-border-radius-border-box-000-expected.html:7
> > +        border: 20px solid rgba(0, 0, 0, 0);
> 
> hehe, black would have been enough :D

I'll change them to blue, so they match the background.

(In reply to comment #4)
> (From update of attachment 225793 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225793&action=review
> 
> >> Source/WebCore/rendering/LayoutBox.cpp:40
> >> +    LayoutUnit ratio = radius / margin;
> > 
> > No check for zero before?
> 
> if margin is 0 this is a divide by zero.

Yup. My bad. Added a check for zero margins.

(In reply to comment #5)
> (From update of attachment 225793 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225793&action=review
> 
> > Source/WebCore/rendering/LayoutBox.cpp:47
> > +static inline LayoutSize computeMarginBoxRadius(LayoutSize radius, LayoutSize adjacentMargins)
> 
> These should be const LayoutSize&

Ok, I've changed this.

[Bem Jones-Bey]

(In reply to comment #6)
> > 
> > > LayoutTests/css3/masking/clip-path-border-radius-border-box-000-expected.html:7
> > > +        border: 20px solid rgba(0, 0, 0, 0);
> > 
> > hehe, black would have been enough :D
> 
> I'll change them to blue, so they match the background.

Looks like if I do that, then when I use background-clip to clip to the padding box or content-box, the border is still drawn, and it looks really weird. I'm going to stick with the transparent border for now.

[Bem Jones-Bey]

Created attachment 226506 [details]
Patch

Update patch for review comments. I also noticed that I wasn't properly using logical coordinates, and fixed a test failure for a shape-inside test that I somehow didn't notice before.

[Simon Fraser (smfr)]

Comment on attachment 226506 [details]
Patch

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

> Source/WebCore/rendering/LayoutBox.cpp:48
> +static inline LayoutUnit adjustRadiusForMargin(LayoutUnit radius, LayoutUnit margin)
> +{
> +    // This algorithm is defined in the CSS Shapes specifcation
> +    if (!margin)
> +        return radius;
> +
> +    LayoutUnit ratio = radius / margin;
> +    if (ratio < 1)
> +        return radius + (margin * (1 + pow(ratio - 1, 3)));
> +
> +    return radius + margin;

If this is just about shapes (and not border-radius) then the function name should reflect that.

> Source/WebCore/rendering/LayoutBox.cpp:97
> +static inline LayoutSize computeMarginBoxRadius(const LayoutSize& radius, const LayoutSize& adjacentMargins)
> +{
> +    return LayoutSize(adjustRadiusForMargin(radius.width(), adjacentMargins.width()),
> +        adjustRadiusForMargin(radius.height(), adjacentMargins.height()));
> +}
> +
> +static inline RoundedRect::Radii computeMarginBoxRadii(const RoundedRect::Radii& radii, const RenderBox& renderer)
> +{
> +    return RoundedRect::Radii(computeMarginBoxRadius(radii.topLeft(), LayoutSize(renderer.marginLeft(), renderer.marginTop())),
> +        computeMarginBoxRadius(radii.topRight(), LayoutSize(renderer.marginRight(), renderer.marginTop())),
> +        computeMarginBoxRadius(radii.bottomLeft(), LayoutSize(renderer.marginLeft(), renderer.marginBottom())),
> +        computeMarginBoxRadius(radii.bottomRight(), LayoutSize(renderer.marginRight(), renderer.marginBottom())));
> +}
> +
> +RoundedRect computeRoundedRectForLayoutBox(const LayoutBox& box, const RenderBox& renderer)
> +{
> +    return computeRoundedRectForLayoutBox(box, renderer, renderer.borderBoxRect());
> +}
> +
> +RoundedRect computeRoundedRectForLayoutBox(const LayoutBox& box, const RenderBox& renderer, const LayoutRect& borderBoxRect)
> +{
> +    const RenderStyle& style = renderer.style();
> +    switch (box) {
> +    case MarginBox: {
> +        if (!style.hasBorderRadius())
> +            return RoundedRect(renderer.marginBoxRect(), RoundedRect::Radii());
> +
> +        LayoutRect marginBox = renderer.marginBoxRect();
> +        RoundedRect::Radii radii = computeMarginBoxRadii(style.getRoundedBorderFor(borderBoxRect, &(renderer.view())).radii(), renderer);
> +        radii.scale(calcBorderRadiiConstraintScaleFor(marginBox, radii));
> +        return RoundedRect(marginBox, radii);
> +    }
> +    case PaddingBox:
> +        return style.getRoundedInnerBorderFor(borderBoxRect);
> +    case ContentBox:
> +        return style.getRoundedInnerBorderFor(borderBoxRect,
> +            renderer.paddingTop() + renderer.borderTop(), renderer.paddingBottom() + renderer.borderBottom(),
> +            renderer.paddingLeft() + renderer.borderLeft(), renderer.paddingRight() + renderer.borderRight());
> +        // fill, stroke, view-box compute to border-box for HTML elements.
> +    case BorderBox:
> +    case Fill:
> +    case Stroke:
> +    case ViewBox:
> +    case BoxMissing:
> +        return style.getRoundedBorderFor(borderBoxRect, &(renderer.view()));
> +    }
> +}

All this stuff also seems very shape-specific, but the function names make it sound like it covers border-radius too.

> Source/WebCore/rendering/LayoutBox.h:1
> +/*

It's not clear why you need a new file for this.

[zalan]

Comment on attachment 226506 [details]
Patch

What I'd do is to get rid of the RenderStyle::getRounded*BorderFor() functions, move their code to computeRoundedRectFor() and make it a RenderBoxModelObject method. -unless I missed some layer violation here (RenderBox would be more correct though, but according to a quick grep, it is not possible atm) I am not sure why RenderStyle has the getRoundedBox* calculations, but to me it sounds more of a layout(paint) than a style concept.

[Bem Jones-Bey]

Created attachment 226956 [details]
Patch

How about this for now, and I do follow ups for the LayoutBox name and to attempt to unify the radii calculation into one function?

[Bem Jones-Bey]

Created attachment 227112 [details]
Patch

Updated for LayoutBox rename to CSSBoxType. WDYT?

[Bem Jones-Bey]

Created attachment 227134 [details]
Patch

Fix EFL/Gtk build breakage.

[zalan]

Comment on attachment 227134 [details]
Patch

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

> Source/WebCore/rendering/shapes/BoxShape.h:39
> +RoundedRect computeRoundedRectForBoxShape(const CSSBoxType&, const RenderBox&, const LayoutRect&);

const CSSBoxType& -> CSSBoxType?

[zalan]

(In reply to comment #14)
> (From update of attachment 227134 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227134&action=review
> 
> > Source/WebCore/rendering/shapes/BoxShape.h:39
> > +RoundedRect computeRoundedRectForBoxShape(const CSSBoxType&, const RenderBox&, const LayoutRect&);
> 
> const CSSBoxType& -> CSSBoxType?
for both functions.

[Bem Jones-Bey]

Created attachment 227186 [details]
Patch

const CSSBoxType& -> CSSBoxType

[Bem Jones-Bey]

Comment on attachment 227134 [details]
Patch

Something must have gone wrong with my upload this morning, don't know why this patch didn't automatically get marked as obsolete. Fixing that now.

[Simon Fraser (smfr)]

Comment on attachment 227186 [details]
Patch

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

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:150
> +        // We can't just use the borderBoxRect on m_renderer because in the case of shape-inside,
> +        // it doesn't yet have a height at the time this is called. So we're left with this unfortunate mess.

Why not? When?

[Bem Jones-Bey]

(In reply to comment #18)
> (From update of attachment 227186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227186&action=review
> 
> > Source/WebCore/rendering/shapes/ShapeInfo.cpp:150
> > +        // We can't just use the borderBoxRect on m_renderer because in the case of shape-inside,
> > +        // it doesn't yet have a height at the time this is called. So we're left with this unfortunate mess.
> 
> Why not? When?

Zoltan is working on a patch to remove shape-inside. Once that patch lands, I'll fix this here to be the simpler version I wanted in the first place.

[Bem Jones-Bey]

Created attachment 227880 [details]
Patch

Remove shape-inside hack now that shape-inside is gone.

[WebKit Commit Bot]

Comment on attachment 227880 [details]
Patch

Rejecting attachment 227880 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 227880, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ile LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-border-radius-padding-box-002.html
patching file LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-border-radius-padding-box-003-expected.html
patching file LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-border-radius-padding-box-003.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Simon Fraser']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/4568716264079360

[Bem Jones-Bey]

Created attachment 228002 [details]
Rebased Patch

[WebKit Commit Bot]

Comment on attachment 228002 [details]
Rebased Patch

Clearing flags on attachment: 228002

Committed r166383: <http://trac.webkit.org/changeset/166383>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.