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

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: Michael Ventnor, Assigned: Michael Ventnor)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {fixed1.9.1})

unspecified
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

8 years ago
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.
Attachment #360399 - Flags: review?(dbaron)
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 .)
(Assignee)

Comment 2

8 years ago
Created attachment 360408 [details] [diff] [review]
Rediff

Sure, here you go.
Attachment #360399 - Attachment is obsolete: true
Attachment #360408 - Flags: review?(dbaron)
Attachment #360399 - Flags: review?(dbaron)
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.
Attachment #360408 - Flags: review?(dbaron) → review-
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 6

8 years ago
Ah, I remember why now. I'm an idiot.

I'll update the patch Monday.
(Assignee)

Comment 7

8 years ago
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.
Attachment #361185 - Flags: superreview?(roc)
Attachment #361185 - Flags: review?(roc)
(Assignee)

Comment 8

8 years ago
Oh, hm, why am I getting less context again...
I'm still getting git diffs and function names, though.
(Assignee)

Comment 9

8 years ago
Created attachment 361192 [details] [diff] [review]
Patch 2 rediff

Was using a different hg command, thats why...
Attachment #360408 - Attachment is obsolete: true
Attachment #361185 - Attachment is obsolete: true
Attachment #361192 - Flags: superreview?(roc)
Attachment #361192 - Flags: review?(roc)
Attachment #361185 - Flags: superreview?(roc)
Attachment #361185 - Flags: review?(roc)
+  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.
(Assignee)

Comment 11

8 years ago
Created attachment 361207 [details] [diff] [review]
Patch 3

Fix comments, plus fixed the issue with outer shadows on buttons.
Attachment #361192 - Attachment is obsolete: true
Attachment #361207 - Flags: superreview?(roc)
Attachment #361207 - Flags: review?(roc)
Attachment #361192 - Flags: superreview?(roc)
Attachment #361192 - Flags: review?(roc)
Comment on attachment 361207 [details] [diff] [review]
Patch 3

r+sr on the non-style parts.
Attachment #361207 - Flags: superreview?(roc)
Attachment #361207 - Flags: superreview+
Attachment #361207 - Flags: review?(roc)
Attachment #361207 - Flags: review+
(Assignee)

Comment 13

8 years ago
Comment on attachment 361207 [details] [diff] [review]
Patch 3

Now it's dbaron's turn!
Attachment #361207 - Flags: review?(dbaron)
(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?
(Assignee)

Comment 15

8 years ago
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?
(Assignee)

Comment 17

8 years ago
Yes.
(Assignee)

Comment 18

8 years ago
(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.
Attachment #361207 - Flags: review?(dbaron) → review+
(Assignee)

Comment 20

8 years ago
Created attachment 361415 [details] [diff] [review]
Patch 4

Wonderful, thanks.
Attachment #361207 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/ba9ad7164a84
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Attachment #361415 - Flags: approval1.9.1?
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.
Attachment #361415 - Flags: approval1.9.1? → approval1.9.1-

Updated

8 years ago
Blocks: 479677
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).
Attachment #361415 - Flags: approval1.9.1- → approval1.9.1?
(Assignee)

Comment 26

8 years ago
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.

Updated

8 years ago
Blocks: 481847

Updated

8 years ago
Blocks: 482086

Updated

8 years ago
Blocks: 482105
Comment on attachment 361415 [details] [diff] [review]
Patch 4

ok, a1.9.1=dbaron
Attachment #361415 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
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.
Keywords: checkin-needed
Whiteboard: [needs 191 landing] → [needs bug 456219]

Updated

8 years ago
Depends on: 456219
Whiteboard: [needs bug 456219] → [needs bug 456219][needs 191 landing]

Updated

8 years ago
Blocks: 422203
Created attachment 371318 [details] [diff] [review]
191 branch patch

(only minor merging)
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs bug 456219][needs 191 landing] → [needs branch patch in bug 456219, is also needs 191 landing][needs 191 landing]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cfea33167281
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [needs branch patch in bug 456219, is also needs 191 landing][needs 191 landing]
Blocks: 631373

Updated

4 years ago
Depends on: 829712
You need to log in before you can comment on or make changes to this bug.