Last Comment Bug 742736 - [css3-background] spread box-shadow + border-radius on two opposite corners should keep sharp corners on other corners
: [css3-background] spread box-shadow + border-radius on two opposite corners s...
Status: RESOLVED FIXED
[good first bug][mentor=dbaron]
:
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla15
Assigned To: Thomas Powell
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 514670
  Show dependency treegraph
 
Reported: 2012-04-05 08:26 PDT by paul
Modified: 2012-04-30 08:02 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
4-sided-rounding.png (7.96 KB, image/png)
2012-04-05 08:26 PDT, paul
no flags Details
reporter's testcase (272 bytes, text/html)
2012-04-07 13:14 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details
3 rounded corners causes fourth corner's shadow to be rounded (294 bytes, text/html)
2012-04-24 20:21 PDT, Thomas Powell
no flags Details
Proposed patch for this bug, creates ComputeOuterRadii and calls it. (4.52 KB, patch)
2012-04-26 15:35 PDT, Thomas Powell
dbaron: review-
Details | Diff | Splinter Review
Replacement patch with updates as suggested in review. (5.64 KB, patch)
2012-04-27 16:27 PDT, Thomas Powell
dbaron: review+
Details | Diff | Splinter Review
Patch with further suggestions implemented + reftests added for 2 and 3 corner scenarios. (9.27 KB, patch)
2012-04-29 19:14 PDT, Thomas Powell
dbaron: review+
Details | Diff | Splinter Review

Description paul 2012-04-05 08:26:36 PDT
Created attachment 612553 [details]
4-sided-rounding.png

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.
Comment 1 David Baron :dbaron: ⌚️UTC-10 2012-04-07 13:13:37 PDT
> 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.
Comment 2 David Baron :dbaron: ⌚️UTC-10 2012-04-07 13:14:42 PDT
Created attachment 613133 [details]
reporter's testcase
Comment 3 David Baron :dbaron: ⌚️UTC-10 2012-04-07 13:18:48 PDT
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.
Comment 4 David Baron :dbaron: ⌚️UTC-10 2012-04-07 13:22:37 PDT
Perhaps there's something wrong with the logic here:
http://hg.mozilla.org/mozilla-central/file/babbc38b7f52/layout/base/nsCSSRendering.cpp#l1247
Comment 5 David Baron :dbaron: ⌚️UTC-10 2012-04-07 13:24:37 PDT
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.
Comment 6 David Baron :dbaron: ⌚️UTC-10 2012-04-07 13:29:08 PDT
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.)
Comment 7 Thomas Powell 2012-04-24 20:03:30 PDT
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.
Comment 8 Thomas Powell 2012-04-24 20:21:56 PDT
Created attachment 618153 [details]
3 rounded corners causes fourth corner's shadow to be rounded
Comment 9 Thomas Powell 2012-04-26 15:35:40 PDT
Created attachment 618826 [details] [diff] [review]
Proposed patch for this bug, creates ComputeOuterRadii and calls it.

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 10 David Baron :dbaron: ⌚️UTC-10 2012-04-26 16:51:30 PDT
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.
Comment 11 David Baron :dbaron: ⌚️UTC-10 2012-04-26 16:53:52 PDT
(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
Comment 12 David Baron :dbaron: ⌚️UTC-10 2012-04-26 17:06:02 PDT
(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
Comment 13 Thomas Powell 2012-04-26 18:17:27 PDT
(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.
Comment 14 David Baron :dbaron: ⌚️UTC-10 2012-04-26 21:10:25 PDT
(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 15 Thomas Powell 2012-04-27 16:27:23 PDT
Created attachment 619208 [details] [diff] [review]
Replacement patch with updates as suggested in review.
Comment 16 David Baron :dbaron: ⌚️UTC-10 2012-04-27 16:56:40 PDT
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
Comment 17 David Baron :dbaron: ⌚️UTC-10 2012-04-28 10:44:18 PDT
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.)
Comment 18 Thomas Powell 2012-04-29 10:29:04 PDT
> 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.
Comment 19 David Baron :dbaron: ⌚️UTC-10 2012-04-29 11:08:32 PDT
Just leave it uninitialized (and then make the assignments right below).
Comment 20 Thomas Powell 2012-04-29 19:14:22 PDT
Created attachment 619470 [details] [diff] [review]
Patch with further suggestions implemented + reftests added for 2 and 3 corner scenarios.
Comment 21 David Baron :dbaron: ⌚️UTC-10 2012-04-29 19:34:40 PDT
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.
Comment 22 David Baron :dbaron: ⌚️UTC-10 2012-04-29 19:39:05 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.