476738 - Implement the 'inset' shadows part of the CSS3 box-shadow spec

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Michael Ventnor]

Attachment: Patch

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 2

[Michael Ventnor]

Attachment: Rediff

Sure, here you go.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[Michael Ventnor]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 6

[Michael Ventnor]

Ah, I remember why now. I'm an idiot.

I'll update the patch Monday.

Comment 7

[Michael Ventnor]

Attachment: Patch 2

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.

Comment 8

[Michael Ventnor]

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

Comment 9

[Michael Ventnor]

Attachment: Patch 2 rediff

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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 11

[Michael Ventnor]

Attachment: Patch 3

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

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 361207 [details] [diff] [review]
Patch 3

r+sr on the non-style parts.

Comment 13

[Michael Ventnor]

Comment on attachment 361207 [details] [diff] [review]
Patch 3

Now it's dbaron's turn!

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 15

[Michael Ventnor]

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

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the test did fail with the old patch?

Comment 17

[Michael Ventnor]

Yes.

Comment 18

[Michael Ventnor]

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

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[Michael Ventnor]

Attachment: Patch 4

Wonderful, thanks.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 25

[Dão Gottwald [:dao]]

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

Comment 26

[Michael Ventnor]

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 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 361415 [details] [diff] [review]
Patch 4

ok, a1.9.1=dbaron

Comment 28

[Markus Stange [:mstange]]

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.

Comment 29

[Markus Stange [:mstange]]

Attachment: 191 branch patch

(only minor merging)

Comment 30

[Markus Stange [:mstange]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cfea33167281