Bug 127982

<rdar://problem/15958752>

Created attachment 225793 [details] Patch

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

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.

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&

(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.

(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.

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.

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.

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.

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?

Created attachment 227112 [details] Patch Updated for LayoutBox rename to CSSBoxType. WDYT?

Created attachment 227134 [details] Patch Fix EFL/Gtk build breakage.

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?

(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.

Created attachment 227186 [details] Patch const CSSBoxType& -> CSSBoxType

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.

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?

(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.

Created attachment 227880 [details] Patch Remove shape-inside hack now that shape-inside is gone.

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

Created attachment 228002 [details] Rebased Patch

Comment on attachment 228002 [details] Rebased Patch Clearing flags on attachment: 228002 Committed r166383: <http://trac.webkit.org/changeset/166383>

All reviewed patches have been landed. Closing bug.