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