Bug 129726 – [CSS Shapes] inset does not properly clamp large corner radii

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Bem Jones-Bey]

inset should clamp large corner radii as described in http://www.w3.org/TR/css3-background/#corner-overlap

[Bem Jones-Bey]

Created attachment 225882 [details]
Patch

[Bem Jones-Bey]

I did not add any tests for zero radii here because I ran into Bug 129739. I will add tests for that when I fix that.

[Dirk Schulze]

Comment on attachment 225882 [details]
Patch

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

> Source/WebCore/rendering/shapes/Shape.cpp:131
> +    float topRadiiExtent = radii.topLeft().width() + radii.topRight().width();
> +    float rightRadiiExtent = radii.topRight().height() + radii.bottomRight().height();
> +    float bottomRadiiExtent = radii.bottomLeft().width() + radii.bottomRight().width();
> +    float leftRadiiExtent = radii.topLeft().height() + radii.bottomLeft().height();
> +
> +    float topRatio = topRadiiExtent ? (rect.width() / topRadiiExtent) : 1;
> +    float rightRatio = rightRadiiExtent ? (rect.height() / rightRadiiExtent) : 1;
> +    float bottomRatio = bottomRadiiExtent ? (rect.width() / bottomRadiiExtent) : 1;
> +    float leftRatio = leftRadiiExtent ? (rect.height() / leftRadiiExtent) : 1;
> +
> +    float reductionRatio = std::min<float>(topRatio, std::min<float>(rightRatio, std::min<float>(bottomRatio, leftRatio)));
> +    if (reductionRatio < 1)
> +        roundedRect.scaleRadii(reductionRatio);

Is there anything that can be shared with border-radius?

[Bem Jones-Bey]

Created attachment 225917 [details]
Patch

[Bem Jones-Bey]

(In reply to comment #3)
> (From update of attachment 225882 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225882&action=review
> 
> > Source/WebCore/rendering/shapes/Shape.cpp:131
> > +    float topRadiiExtent = radii.topLeft().width() + radii.topRight().width();
> > +    float rightRadiiExtent = radii.topRight().height() + radii.bottomRight().height();
> > +    float bottomRadiiExtent = radii.bottomLeft().width() + radii.bottomRight().width();
> > +    float leftRadiiExtent = radii.topLeft().height() + radii.bottomLeft().height();
> > +
> > +    float topRatio = topRadiiExtent ? (rect.width() / topRadiiExtent) : 1;
> > +    float rightRatio = rightRadiiExtent ? (rect.height() / rightRadiiExtent) : 1;
> > +    float bottomRatio = bottomRadiiExtent ? (rect.width() / bottomRadiiExtent) : 1;
> > +    float leftRatio = leftRadiiExtent ? (rect.height() / leftRadiiExtent) : 1;
> > +
> > +    float reductionRatio = std::min<float>(topRatio, std::min<float>(rightRatio, std::min<float>(bottomRatio, leftRatio)));
> > +    if (reductionRatio < 1)
> > +        roundedRect.scaleRadii(reductionRatio);
> 
> Is there anything that can be shared with border-radius?

Yeah, I should have done that in the first place. The new patch reuses the same function as border-radius.

[Dirk Schulze]

Comment on attachment 225917 [details]
Patch

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

> Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:29
> +template <class Rect, class Radii>
> +inline float calcBorderRadiiConstraintScaleFor(const Rect& rect, const Radii& radii)

Cause I hate to debugs templates, I am not a huge fan of templates and would like to avoid them as much as possible. For this particular case it seems you just created a template to be able to use the classes Rect and Radii. Isn't there a better way? Also, Can't you just add this to FloatRoundedRect or RoundedRect instead of creating another header?

[Bem Jones-Bey]

(In reply to comment #6)
> (From update of attachment 225917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225917&action=review
> 
> > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:29
> > +template <class Rect, class Radii>
> > +inline float calcBorderRadiiConstraintScaleFor(const Rect& rect, const Radii& radii)
> 
> Cause I hate to debugs templates, I am not a huge fan of templates and would like to avoid them as much as possible. For this particular case it seems you just created a template to be able to use the classes Rect and Radii. Isn't there a better way? Also, Can't you just add this to FloatRoundedRect or RoundedRect instead of creating another header?

I would unfortunately have to add it to both of them. The shapes geometry code uses floats, and the style and layout code uses LayoutUnits. Converting in the middle of the geometry code to LayoutUnits and then back to floats has caused problems in the past due to the imprecise nature of that conversion. And I don't think we want to be converting to FloatRoundedRect and back every time we need to do this calculation in RenderStyle (or in my fix for Bug 127982, for that matter).

I did notice that there is an adjustRadii() call in RoundedRect, but it doesn't do exactly the same calculation as the border radii case, and it looks like it is only used for box shadow (RenderBoxModelObject::paintBoxShadow).

[Dirk Schulze]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 225917 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=225917&action=review
> > 
> > > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:29
> > > +template <class Rect, class Radii>
> > > +inline float calcBorderRadiiConstraintScaleFor(const Rect& rect, const Radii& radii)
> > 
> > Cause I hate to debugs templates, I am not a huge fan of templates and would like to avoid them as much as possible. For this particular case it seems you just created a template to be able to use the classes Rect and Radii. Isn't there a better way? Also, Can't you just add this to FloatRoundedRect or RoundedRect instead of creating another header?
> 
> I would unfortunately have to add it to both of them. The shapes geometry code uses floats, and the style and layout code uses LayoutUnits. Converting in the middle of the geometry code to LayoutUnits and then back to floats has caused problems in the past due to the imprecise nature of that conversion. And I don't think we want to be converting to FloatRoundedRect and back every time we need to do this calculation in RenderStyle (or in my fix for Bug 127982, for that matter).
> 
> I did notice that there is an adjustRadii() call in RoundedRect, but it doesn't do exactly the same calculation as the border radii case, and it looks like it is only used for box shadow (RenderBoxModelObject::paintBoxShadow).

Is it possible to create two inline functions where one calls the other but does the conversion first?

[Bem Jones-Bey]

Created attachment 226004 [details]
Patch

[Bem Jones-Bey]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 225917 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=225917&action=review
> > > 
> > > > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:29
> > > > +template <class Rect, class Radii>
> > > > +inline float calcBorderRadiiConstraintScaleFor(const Rect& rect, const Radii& radii)
> > > 
> > > Cause I hate to debugs templates, I am not a huge fan of templates and would like to avoid them as much as possible. For this particular case it seems you just created a template to be able to use the classes Rect and Radii. Isn't there a better way? Also, Can't you just add this to FloatRoundedRect or RoundedRect instead of creating another header?
> > 
> > I would unfortunately have to add it to both of them. The shapes geometry code uses floats, and the style and layout code uses LayoutUnits. Converting in the middle of the geometry code to LayoutUnits and then back to floats has caused problems in the past due to the imprecise nature of that conversion. And I don't think we want to be converting to FloatRoundedRect and back every time we need to do this calculation in RenderStyle (or in my fix for Bug 127982, for that matter).
> > 
> > I did notice that there is an adjustRadii() call in RoundedRect, but it doesn't do exactly the same calculation as the border radii case, and it looks like it is only used for box shadow (RenderBoxModelObject::paintBoxShadow).
> 
> Is it possible to create two inline functions where one calls the other but does the conversion first?

I looked into this and discovered that there are some implicit conversions from LayoutRect/RoundedRect -> FloatRect/FloatRoundedRect. So I just changed the signature to take the float varieties, and not have a template. WDYT?

[Bem Jones-Bey]

Created attachment 226011 [details]
Patch

[Dirk Schulze]

Comment on attachment 226011 [details]
Patch

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

> Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:31
> +inline float calcBorderRadiiConstraintScaleFor(const FloatRect& rect, const FloatRoundedRect::Radii& radii)

looks nicer already. Do you think we can put that on FloatRoundedRect.h instead of creating a new header file?

[Bem Jones-Bey]

Created attachment 226019 [details]
Patch

[Bem Jones-Bey]

(In reply to comment #12)
> (From update of attachment 226011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226011&action=review
> 
> > Source/WebCore/rendering/style/BorderRadiiConstraintScale.h:31
> > +inline float calcBorderRadiiConstraintScaleFor(const FloatRect& rect, const FloatRoundedRect::Radii& radii)
> 
> looks nicer already. Do you think we can put that on FloatRoundedRect.h instead of creating a new header file?

Ok, WDYT?

(I wish I could make it just take a FloatRoundedRect, but that would require some serious surgery to how RenderStyle does it's thing.)

[Dirk Schulze]

Comment on attachment 226019 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 226019 [details]
Patch

Clearing flags on attachment: 226019

Committed r165261: <http://trac.webkit.org/changeset/165261>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michal Pakula vel Rutka]

Committed r165262: <http://trac.webkit.org/changeset/165262>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 129887

[Bem Jones-Bey]

The reopen was spurious; a different commit got tangled up with this one, and that's the one that got reverted.