User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/534.55.3 (KHTML, like Gecko) Version/5.1.5 Safari/534.55.3 Steps to reproduce: Consider the following CSS code div { width: 300px; height: 300px; margin: 100px; background-color: silver; border-radius: 0 10px 0 10px; box-shadow: 0 0 0 20px rgba(0,0,0,0.5); } A sample can be found at: http://tinkerbin.com/pwo285n2 Actual results: Gecko appears to round the corners of the shadow of the top-left and bottom-right corners. Expected results: In section 7.2 of Borders and Backgrounds Module 3 (http://www.w3.org/TR/css3-background/#box-shadow), it states that: "For corners with a zero border-radius, however, the corner must remain sharp—the operation is equivalent to scaling the shadow shape." The corners should have remained square, as no border-radius was added to the top-left and bottom-right sides.

> A sample can be found at: > http://tinkerbin.com/pwo285n2 This is a bad way to post testcases: it doesn't even let me load the testcase in its own tab -- it redirects back to the editing view -- which basically means that it's impossible to use any debugging tool without rebuilding the testcase. It's also best to actually attach a reduced testcase to the bug, so that it's archived. I'll do that for this one.

For some reason, this only seems to happen when two opposite corners have a border-radius; if just one corner, or if two corners that have a side in common have a border-radius (or something like that -- I didn't test all the possibilities), we do keep corners sharp.

Perhaps there's something wrong with the logic here: http://hg.mozilla.org/mozilla-central/file/babbc38b7f52/layout/base/nsCSSRendering.cpp#l1247

Yeah, I think, basically, that code is fundamentally broken and the fix for bug 514670 was done wrong, since it doesn't work correctly for a corner with no rounding between two corners that do have rounding. I think the proper solution would be to set all the borders before the call, and zero out the radii after the call where necessary.

And by "the call", I meant the call to nsCSSBorderRenderer::ComputeInnerRadii. (It also might make sense to encapsulate that logic in a ComputeOuterRadii function, rather than passing negative borders to ComputeInnerRadii.)

I'd like to work on this bug. I did a quick modification of the nsCSSRendering.cpp code based on the suggested changes, and so far, so good. I also noticed that the case with 3 round corners and 1 sharp corner also fails.

Some concerns that I'd like feedback on: Is ComputeOuterRadii in an appropriate place? Does ComputeOuterRadii do enough of its own work or should some of the borderSizes/spreadDistance logic be pulled in.

Comment on attachment 618826 [details] [diff] [review] Proposed patch for this bug, creates ComputeOuterRadii and calls it. ># HG changeset patch ># Parent b952bb042f129b3ac3083209ecc447b080c3ed6d ># User Thomas Powell <twilliampowell@gmail.com> >Bug 742736 - Change box shadows to match rounded or sharp corners of shadowed >object when round/sharp are on alternating corners; r=dbaron You actually want this all on one line -- there's a semantic difference between the first line of a commit message and the rest; the first line is a complete summary, and if a more detailed explanation is needed that should be properly-wrapped text on the lines following. >diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp >--- a/layout/base/nsCSSRendering.cpp >+++ b/layout/base/nsCSSRendering.cpp >@@ -1265,17 +1265,19 @@ nsCSSRendering::PaintBoxShadowOuter(nsPr > if (borderRadii[C_TR].width > 0 || borderRadii[C_BR].width > 0) { > borderSizes[NS_SIDE_RIGHT] = spreadDistance; > } > > if (borderRadii[C_BL].height > 0 || borderRadii[C_BR].height > 0) { > borderSizes[NS_SIDE_BOTTOM] = spreadDistance; > } You should remove all the conditions for setting borderSizes here (and continuing a little above what's quoted) and just set the borderSizes unconditionally. Your new code doesn't need these conditions (and they didn't get things right). You should also change spreadDistance to be a positive number (remove the negative sign when setting it), since it doesn't make sense to pass the negative of the borders to a function that serves the purpose of what you're doing (rather than the trick of passing the negative number to a function that does the opposite thing). >- nsCSSBorderRenderer::ComputeInnerRadii(borderRadii, borderSizes, >+ // bug 742736, created separate function for shadow to keep >+ // corners sharp if shadowed corner is sharp. >+ nsCSSBorderRenderer::ComputeOuterRadii(borderRadii, borderSizes, > &clipRectRadii); Don't add a comment here. The version control system (in our case, Mercurial) shows the history of the code; comments describing all the history would end up making it unreadable after a time. >diff --git a/layout/base/nsCSSRenderingBorders.h b/layout/base/nsCSSRenderingBorders.h ... >+ // utility function used for computing shadow borders, added with fix for >+ // bug 742736 >+ static void ComputeOuterRadii(const gfxCornerSizes& aRadii, >+ const gfxFloat *aBorderSizes, >+ gfxCornerSizes *aOuterRadiiRet); I think a comment is useful here, but it should say what the function does rather than when it was added. Maybe something like: // Given aRadii as the border radii for a rectangle, compute the // appropriate radii for another rectangle *outside* that rectangle // by increasing the radii, except keeping sharp corners sharp. // Used for spread box-shadows >diff --git a/layout/base/nsCSSRenderingBorders.cpp b/layout/base/nsCSSRenderingBorders.cpp >+/* static */ void >+nsCSSBorderRenderer::ComputeOuterRadii(const gfxCornerSizes& aRadii, >+ const gfxFloat *aBorderSizes, >+ gfxCornerSizes *aOuterRadiiRet) >+{ >+ gfxCornerSizes& oRadii = *aOuterRadiiRet; >+ >+ // default all corners to sharp corners >+ oRadii = gfxCornerSizes(0.0); >+ >+ // round the edges that have radii > 0.0 to start with >+ if(aRadii[C_TL].width > 0.0 && aRadii[C_TL].height > 0.0) Hmmm. I think the spec is actually a little unclear here, but I think what you've done (using &&) makes sense, though one could read the spec as requiring ||. >+ { Local style puts the { on the same line as the if (), and also has a space between "if" and "(". >+ oRadii[C_TL].width = NS_MAX(0.0, aRadii[C_TL].width - aBorderSizes[NS_SIDE_LEFT]); >+ oRadii[C_TL].height = NS_MAX(0.0, aRadii[C_TL].height - aBorderSizes[NS_SIDE_TOP]); >+ } You should change all these - signs to + signs (see above). >+ if(aRadii[C_TR].width > 0.0 && aRadii[C_TR].height > 0.0) >+ { >+ oRadii[C_TR].width = NS_MAX(0.0, aRadii[C_TR].width - aBorderSizes[NS_SIDE_RIGHT]); >+ oRadii[C_TR].height = NS_MAX(0.0, aRadii[C_TR].height - aBorderSizes[NS_SIDE_TOP]); >+ } Probably best to put blank lines between all of these pairs of if {} blocks rather than just some pairs. With those things fixed, this looks good. But I'm marking as review- because I'd like to take a quick look at the revisions. Thanks for taking this one.

(In reply to Thomas Powell from comment #9) > Is ComputeOuterRadii in an appropriate place? yes > Does ComputeOuterRadii do enough of its own work or should some of the > borderSizes/spreadDistance logic be pulled in. looks great

(In reply to David Baron [:dbaron] from comment #10) > >+ if(aRadii[C_TL].width > 0.0 && aRadii[C_TL].height > 0.0) > > Hmmm. I think the spec is actually a little unclear here, but I think what > you've done (using &&) makes sense, though one could read the spec as > requiring ||. Then again, that's pretty much unrelated to this bug, since you're not changing our current behavior. But I did post http://lists.w3.org/Archives/Public/www-style/2012Apr/0767.html

(In reply to David Baron [:dbaron] from comment #10) > You should remove all the conditions for setting borderSizes here (and > continuing a little above what's quoted) and just set the borderSizes > unconditionally. Your new code doesn't need these conditions (and they > didn't get things right). > Should ComputeOuterRadii just receive spreadDistance, then? I started to do that, but then thought I might be missing a scenario somehow.

(In reply to Thomas Powell from comment #13) > (In reply to David Baron [:dbaron] from comment #10) > > > You should remove all the conditions for setting borderSizes here (and > > continuing a little above what's quoted) and just set the borderSizes > > unconditionally. Your new code doesn't need these conditions (and they > > didn't get things right). > > > Should ComputeOuterRadii just receive spreadDistance, then? I started to do > that, but then thought I might be missing a scenario somehow. I think it's probably better to leave the two functions taking the same sort of arguments rather than introduce a difference that might need to be undone later in case we want it for something else.

Comment on attachment 619208 [details] [diff] [review] Replacement patch with updates as suggested in review. ># HG changeset patch ># Parent b952bb042f129b3ac3083209ecc447b080c3ed6d ># User Thomas Powell <twilliampowell@gmail.com> >Bug 742736 - Fix box shadow corner calc for round/sharp corners; r=dbaron Oh, I didn't mean to shorten the comment -- having a longer amount of text all on one line for the first line is normal. The previous comment was better. >diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp >--- a/layout/base/nsCSSRendering.cpp >+++ b/layout/base/nsCSSRendering.cpp >@@ -1242,40 +1242,28 @@ nsCSSRendering::PaintBoxShadowOuter(nsPr > } > > renderContext->SetFillRule(gfxContext::FILL_RULE_EVEN_ODD); > renderContext->Clip(); > > shadowContext->NewPath(); > if (hasBorderRadius) { > gfxCornerSizes clipRectRadii; >- gfxFloat spreadDistance = -shadowItem->mSpread / twipsPerPixel; >+ gfxFloat spreadDistance = shadowItem->mSpread / twipsPerPixel; > gfxFloat borderSizes[4] = { 0, 0, 0, 0 }; Oh, and the = {0, 0, 0, 0} isn't needed any more -- or the assignment of spreadDistance could actually just go there instead instead of: >+ borderSizes[NS_SIDE_LEFT] = spreadDistance; >+ borderSizes[NS_SIDE_TOP] = spreadDistance; >+ borderSizes[NS_SIDE_RIGHT] = spreadDistance; >+ borderSizes[NS_SIDE_BOTTOM] = spreadDistance; > // We only give the spread radius to corners with a radius on them, otherwise we'll > // give a rounded shadow corner to a frame corner with 0 border radius, should > // the author use non-uniform border radii sizes (border-top-left-radius etc) This comment should go; if it's needed it belongs inside ComputeOuterRadii. >+ // round the edges that have radii > 0.0 to start with >+ if(aRadii[C_TL].width > 0.0 && aRadii[C_TL].height > 0.0) { space between "if" and "(" [4 times]. r=dbaron with those things fixed

One other thing that I should have asked for earlier: this patch could use an automated test (or perhaps more than one). The best way to write such tests is a reftests; the best way to check the behavior is probably to compare a shadow + background to a larger background and assert they're equal. (There might be very drawing glitches where the shadow and the background meet that require you to cover up small pieces of the test with another element.) For more on reftests, see https://developer.mozilla.org/en/Mozilla_automated_testing and http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt If you post a patch with those revisions, I or someone else can push it for you. (If you add the checkin-needed keyword to the bug, that will increase the chances it will happen quickly.)

> Oh, and the = {0, 0, 0, 0} isn't needed any more -- or the assignment of > spreadDistance could actually just go there instead instead of: > > >+ borderSizes[NS_SIDE_LEFT] = spreadDistance; > >+ borderSizes[NS_SIDE_TOP] = spreadDistance; > >+ borderSizes[NS_SIDE_RIGHT] = spreadDistance; > >+ borderSizes[NS_SIDE_BOTTOM] = spreadDistance; Would a {0} initializer work? I wouldn't want to leave the auto variable uninitialized, but the [NS_SIDE_...] element assignment seems to help explain better what's going on, as opposed to {spreadDistance, spreadDistance, ...} initializer.

Just leave it uninitialized (and then make the assignments right below).

Comment on attachment 619470 [details] [diff] [review] Patch with further suggestions implemented + reftests added for 2 and 3 corner scenarios. r=dbaron Although in future, it's a bad idea to have red show up in a passing test because red is often used to indicate failure and green to indicate success. Generally if you can't tell the status from color alone then it's best to use other colors. In this case, though, you're copying that pattern from an existing test.

Pushed to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c7c21102690 mozilla-inbound gets merged to mozilla-central daily or so. Once this gets in mozilla-central, it'll appear in the next nightly. Thanks for fixing this, and good work.