Last Comment Bug 476738 - Implement the 'inset' shadows part of the CSS3 box-shadow spec
: Implement the 'inset' shadows part of the CSS3 box-shadow spec
Status: RESOLVED FIXED
: fixed1.9.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
-- normal with 1 vote (vote)
: ---
Assigned To: Michael Ventnor
:
: Jet Villegas (:jet)
Mentors:
Depends on: 829712 456219
Blocks: css3-background 422203 479677 481847 482086 482105
  Show dependency treegraph
 
Reported: 2009-02-03 15:55 PST by Michael Ventnor
Modified: 2013-01-11 18:29 PST (History)
8 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (30.88 KB, patch)
2009-02-03 15:55 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Rediff (43.56 KB, patch)
2009-02-03 16:32 PST, Michael Ventnor
dbaron: review-
Details | Diff | Splinter Review
Patch 2 (32.94 KB, patch)
2009-02-08 15:07 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 2 rediff (46.28 KB, patch)
2009-02-08 16:06 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 3 (48.96 KB, patch)
2009-02-08 17:54 PST, Michael Ventnor
roc: review+
dbaron: review+
roc: superreview+
Details | Diff | Splinter Review
Patch 4 (48.97 KB, patch)
2009-02-09 16:47 PST, Michael Ventnor
dbaron: approval1.9.1+
Details | Diff | Splinter Review
191 branch patch (35.20 KB, patch)
2009-04-06 15:14 PDT, Markus Stange [:mstange] (away until Feb 22)
no flags Details | Diff | Splinter Review

Description User image Michael Ventnor 2009-02-03 15:55:10 PST
Created attachment 360399 [details] [diff] [review]
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 User image David Baron :dbaron: ⌚️UTC-8 2009-02-03 16:24:26 PST
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 User image Michael Ventnor 2009-02-03 16:32:18 PST
Created attachment 360408 [details] [diff] [review]
Rediff

Sure, here you go.
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2009-02-05 10:23:37 PST
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 User image Michael Ventnor 2009-02-05 11:04:49 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2009-02-05 11:13:10 PST
(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 User image Michael Ventnor 2009-02-05 12:55:45 PST
Ah, I remember why now. I'm an idiot.

I'll update the patch Monday.
Comment 7 User image Michael Ventnor 2009-02-08 15:07:22 PST
Created attachment 361185 [details] [diff] [review]
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 User image Michael Ventnor 2009-02-08 15:09:21 PST
Oh, hm, why am I getting less context again...
I'm still getting git diffs and function names, though.
Comment 9 User image Michael Ventnor 2009-02-08 16:06:49 PST
Created attachment 361192 [details] [diff] [review]
Patch 2 rediff

Was using a different hg command, thats why...
Comment 10 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-08 16:57:24 PST
+  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 User image Michael Ventnor 2009-02-08 17:54:26 PST
Created attachment 361207 [details] [diff] [review]
Patch 3

Fix comments, plus fixed the issue with outer shadows on buttons.
Comment 12 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-08 18:25:49 PST
Comment on attachment 361207 [details] [diff] [review]
Patch 3

r+sr on the non-style parts.
Comment 13 User image Michael Ventnor 2009-02-08 18:29:54 PST
Comment on attachment 361207 [details] [diff] [review]
Patch 3

Now it's dbaron's turn!
Comment 14 User image David Baron :dbaron: ⌚️UTC-8 2009-02-09 15:01:20 PST
(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 User image Michael Ventnor 2009-02-09 15:36:15 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2009-02-09 15:46:23 PST
So the test did fail with the old patch?
Comment 17 User image Michael Ventnor 2009-02-09 16:15:19 PST
Yes.
Comment 18 User image Michael Ventnor 2009-02-09 16:15:40 PST
(I just applied and ran the old patch now to check)
Comment 19 User image David Baron :dbaron: ⌚️UTC-8 2009-02-09 16:28:07 PST
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 User image Michael Ventnor 2009-02-09 16:47:37 PST
Created attachment 361415 [details] [diff] [review]
Patch 4

Wonderful, thanks.
Comment 21 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-10 00:49:41 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/ba9ad7164a84
Comment 22 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-10 00:50:48 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2009-02-10 13:37:39 PST
I think it's a little late for new features.
Comment 24 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-02-10 14:10:08 PST
I was thinking of it as more completing support for a existing new feature, but I guess it doesn't matter.
Comment 25 User image Dão Gottwald [:dao] 2009-02-26 17:51:34 PST
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 User image Michael Ventnor 2009-02-26 20:29:23 PST
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 User image David Baron :dbaron: ⌚️UTC-8 2009-03-10 15:24:46 PDT
Comment on attachment 361415 [details] [diff] [review]
Patch 4

ok, a1.9.1=dbaron
Comment 28 User image Markus Stange [:mstange] (away until Feb 22) 2009-03-11 03:40:34 PDT
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 User image Markus Stange [:mstange] (away until Feb 22) 2009-04-06 15:14:32 PDT
Created attachment 371318 [details] [diff] [review]
191 branch patch

(only minor merging)
Comment 30 User image Markus Stange [:mstange] (away until Feb 22) 2009-04-07 04:45:18 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cfea33167281

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