The latest editor's draft of the box-shadow specification mentions a new type of shadow 'inset' whereby the shadow is painted within the frame just above the background, to give the appearance of the frame being sunk underneath the rest of the canvas. Will ask dbaron to review the parser changes, and roc to review the painting implementation.

Could you put: [diff] git = 1 showfunc = 1 unified = 8 in your ~/.hgrc and rediff? (I'm assuming your hg version is >= 1.0.1 .)

Sure, here you go.

Comment on attachment 360408 [details] [diff] [review] Rediff ParseCSSShadowList looks like it currently accepts: "inset inherit" and treats it as "inherit". (Likewise for "none" and "-moz-initial".) This is incorrect. You should fix this and add a test for it in the invalid_values section for the property in property_database.js. Probably the easiest way to fix it is to adjust the "(cur == list)" test for whether to accept inherit/initial/none right below your first call to ParseVariant, so it also considers whether you parsed 'inset'. I think element.style.boxShadow isn't actually going to produce the 'inset' keyword when it's reserialized. (In other words, if you set and get, inset will be dropped.) I think this is the case because I don't see how nsCSSDeclaration::AppendCSSValueToString would know what keyword table to use to interpret the value with enumerated type. Adding the kBoxShadowTypeKTable to nsCSSPropList.h for the BoxShadow property may well fix this (since it would get picked up by nsCSSDeclaration::AppendCSSValueToString, which is called by itself (for the array case), which is called by AppendValueToString, called by GetValue. You should fix this and add a test for it. Actually, it should have caused a test failure in test_value_storage.html. In particular, it should have failed: if (test_computed && info.type != CSS_TYPE_TRUE_SHORTHAND) { is(gComputedStyle.getPropertyValue(property), step1comp, "serialize+parse should be identity transform for '" + property + ": " + value + "'"); } Was this test failing? If not, could you explain why not? (If so, why didn't you run the tests before requesting review?) Other than that, the layout/style code looks fine.

No, the test didn't fail when I ran it. Could it be because I implemented computed style parsing of the inset keyword but not the actual getter? I'll have another look in a few days.

(In reply to comment #4) > No, the test didn't fail when I ran it. Could it be because I implemented > computed style parsing of the inset keyword but not the actual getter? I'll > have another look in a few days. No... if you hadn't implemented computed style parsing I wouldn't have expected it to fail. What it's testing is that if you set a value, then serialize it and set the serialized value, the computed value doesn't change. But since you implemented the serialization for computed values, it should have.

Ah, I remember why now. I'm an idiot. I'll update the patch Monday.

Fixes dbaron's comments. All tests pass. Asking for review on the rendering parts from roc first this time since it's Sunday in the States.

Oh, hm, why am I getting less context again... I'm still getting git diffs and function names, though.

Was using a different hg command, thats why...

+ nsRect frameRect = aFrameArea; + nsMargin border = aForFrame->GetUsedBorder(); + aForFrame->ApplySkipSides(border); + frameRect.Deflate(border); Call frameRect "paddingRect" to be clear which rect it is. + if (hasBorderRadius) { + gfxCornerSizes outerRadii = borderRadii; + gfxFloat borderSizes[4] = { + border.top / twipsPerPixel, border.right / twipsPerPixel, + border.bottom / twipsPerPixel, border.left / twipsPerPixel + }; + nsCSSBorderRenderer::ComputeInnerRadii(outerRadii, borderSizes, + &borderRadii); I think you should leave borderRadii as the border-box radii and store the inner radii in a new variable paddingRadii. Storing the inner radii in borderRadii is confusing. + if (GetStyleBorder()->mBoxShadow) { Everywhere you're doing this, cache this value in a local "PRBool haveBoxShadow" so you don't need to get it twice. Seems to me that if buttons need a special area for the inner box-shadow, they should also use a special area for the outer box-shadow.

Fix comments, plus fixed the issue with outer shadows on buttons.

Comment on attachment 361207 [details] [diff] [review] Patch 3 r+sr on the non-style parts.

Comment on attachment 361207 [details] [diff] [review] Patch 3 Now it's dbaron's turn!

(In reply to comment #6) > Ah, I remember why now. I'm an idiot. So could you explain what happened with regard to the questions in comment 3 / comment 5? Were the tests actually failing? If not, is there now an added test that fails with the old patch but passes with the new one?

No, it was just something embarrassingly brain-dead. I popped this patch off my hg queue to do some other (semi-related) work, but then forgot I had done so but still compiled and ran the tests. So it didn't actually test it... :(

So the test did fail with the old patch?

Yes.

(I just applied and ran the old patch now to check)

Comment on attachment 361207 [details] [diff] [review] Patch 3 ok, r=dbaron on the layout/style parts if you also add "inset none" to the invalid_values line.

Wonderful, thanks.

Pushed http://hg.mozilla.org/mozilla-central/rev/ba9ad7164a84

Comment on attachment 361415 [details] [diff] [review] Patch 4 I think we should land this on 1.9.1, but I wonder if dbaron agrees.

I think it's a little late for new features.

I was thinking of it as more completing support for a existing new feature, but I guess it doesn't matter.

Comment on attachment 361415 [details] [diff] [review] Patch 4 Another beta, another chance? This inset thing is quite useful (e.g. bug 477827, bug 479677).

Wait, a 4th beta? So much for a quick follow-up release :( I do agree with taking this for 1.9.1, if it were up to me I would consider this an extension of a feature coming in 1.9.1 rather than something new. There haven't been any regressions or bug reports for this on trunk from what I can tell.

Comment on attachment 361415 [details] [diff] [review] Patch 4 ok, a1.9.1=dbaron

I wanted to land this today, but the patch uses nsCSSBorderRenderer::ComputeInnerRadii, which was introduced by bug 456219, which can't land on 1.9.1 yet because bug 472769 hasn't got approval and bug 475535 isn't fixed yet.

(only minor merging)