Last Comment Bug 212633 - Add support for CSS3 box-shadow
: Add support for CSS3 box-shadow
Status: VERIFIED FIXED
: css3, dev-doc-complete, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement with 20 votes (vote)
: mozilla1.9.1a1
Assigned To: Michael Ventnor
:
Mentors:
http://www.w3.org/TR/2005/WD-css3-bac...
Depends on: 448182 906379 444237 445741 453641 458031 466353 475197 475723 485149 514670 514917 527695
Blocks: css3-background
  Show dependency treegraph
 
Reported: 2003-07-14 02:28 PDT by Erik Arvidsson
Modified: 2013-08-18 12:35 PDT (History)
45 users (show)
hskupin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple test case (289 bytes, application/xhtml+xml)
2003-10-15 04:00 PDT, Anne (:annevk)
no flags Details
Parser patch (25.21 KB, patch)
2008-06-18 15:46 PDT, Michael Ventnor
dbaron: review-
dbaron: superreview-
Details | Diff | Review
Parser patch 1.1 (27.06 KB, patch)
2008-06-18 16:41 PDT, Michael Ventnor
no flags Details | Diff | Review
Parser patch 2 (35.42 KB, patch)
2008-06-18 23:20 PDT, Michael Ventnor
dbaron: review-
dbaron: superreview-
Details | Diff | Review
Rendering patch (10.48 KB, patch)
2008-06-19 18:17 PDT, Michael Ventnor
no flags Details | Diff | Review
Rendering patch 2 (10.93 KB, patch)
2008-06-30 21:32 PDT, Michael Ventnor
no flags Details | Diff | Review
Rendering patch 2.01 (10.89 KB, patch)
2008-06-30 21:39 PDT, Michael Ventnor
no flags Details | Diff | Review
Rendering patch 3 (17.55 KB, patch)
2008-07-01 20:16 PDT, Michael Ventnor
no flags Details | Diff | Review
Rendering patch 4 (18.44 KB, patch)
2008-07-01 21:29 PDT, Michael Ventnor
roc: review+
roc: superreview+
Details | Diff | Review
Reftests (4.99 KB, patch)
2008-07-02 02:31 PDT, Michael Ventnor
no flags Details | Diff | Review
Parser patch 3 (35.11 KB, patch)
2008-07-02 19:07 PDT, Michael Ventnor
dbaron: review+
dbaron: superreview+
Details | Diff | Review
Parser patch 4 (38.25 KB, patch)
2008-07-03 17:06 PDT, Michael Ventnor
dbaron: review+
dbaron: superreview+
Details | Diff | Review
Parser patch 4.1 (38.38 KB, patch)
2008-07-06 19:50 PDT, Michael Ventnor
no flags Details | Diff | Review
Screenshot of Selection bug (13.43 KB, image/png)
2008-07-26 16:11 PDT, Hans Schmucker
no flags Details
-moz-box-shadow overlay (430 bytes, text/html)
2010-02-26 03:02 PST, Darren Kalck [:D-Kalck]
no flags Details

Description Erik Arvidsson 2003-07-14 02:28:09 PDT
Although only a working draft, the CSS3 border module defines a property called
box-shadow that allows boxes to drop a shadow.

http://www.w3.org/TR/css3-border/#the-box-shadow

Combine this with RGBA and some very neat visual effects can beachieved
Comment 1 Anne (:annevk) 2003-10-15 04:00:29 PDT
Created attachment 133322 [details]
simple test case

Test case taken from the specification (slightly modified):
http://www.w3.org/TR/css3-border/#the-box-shadow
Comment 2 Greg K. 2005-09-30 06:11:42 PDT
Is any of the work in bug 10713 applicable to this one?
Comment 3 Sierk Bornemann 2007-05-06 23:06:40 PDT
Apple Safari nightly has it, Opera 10 might have it. Firefox should not stay far behind. So, please implement this upcoming CSS3 feature and feed it into the trunk, if a working patch is available. Please sooner than too late.
Comment 4 Michael Ventnor 2008-06-18 06:27:57 PDT
Taking, I've been working on this all day today and have a (AFAIK) perfectly working implementation. Will attach patches tomorrow.
Comment 5 Michael Ventnor 2008-06-18 15:46:07 PDT
Created attachment 325646 [details] [diff] [review]
Parser patch

This adds support for parsing -moz-box-shadow in the style system. Once this gets checked in I'll attach the actual rendering patch.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-18 15:50:06 PDT
Comment on attachment 325646 [details] [diff] [review]
Parser patch

No changes to property_database.js == review-.

Please change property_database.js, run the mochitests in layout/style, and request review with a report that they still all pass.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-18 15:50:35 PDT
And you also need to bump the IID of nsIDOMNSCSS2Properties.
Comment 8 Michael Ventnor 2008-06-18 16:41:11 PDT
Created attachment 325656 [details] [diff] [review]
Parser patch 1.1

I always forget that.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-18 16:49:32 PDT
Did the mochitests pass?
Comment 10 Michael Ventnor 2008-06-18 16:50:19 PDT
Yes.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-18 16:50:53 PDT
Also, what spec have you been using?  Probably http://dev.w3.org/csswg/css3-background/#the-box-shadow is best.
Comment 12 Michael Ventnor 2008-06-18 16:56:22 PDT
Argh, didn't notice that. OK, will update the patch.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-18 16:57:58 PDT
Then again, you're probably better off not implementing spread radius yet, since that was just added in the latest editor's draft, and is still pretty unclear (see my questions in http://lists.w3.org/Archives/Public/www-style/2008May/0218.html ).  So you probably want to follow the featureset of http://www.w3.org/TR/2005/WD-css3-background-20050216/#the-box-shadow using any clarifications in http://dev.w3.org/csswg/css3-background/#the-box-shadow .
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-18 17:08:15 PDT
Comment on attachment 325656 [details] [diff] [review]
Parser patch 1.1

>+
>+  PRBool ParseCSSShadowList(nsresult& aErrorCode, nsCSSValueList** aValueList);

Why doesn't it just return nsCSSValueList* and skip the boolean?

>diff -r 7b6aaf10b6db layout/style/nsCSSStruct.cpp
>+  : mBoxShadow (nsnull)

No space between w and (.

>diff -r 7b6aaf10b6db layout/style/nsCSSStruct.h
>+  nsCSSValueList* mBoxShadow; // NEW

Drop the "// NEW".  It doesn't really help, and it will last until it's not new anymore.

>+nsComputedDOMStyle::ParseCSSShadowArray(nsCSSShadowArray* aArray,

This isn't doing parsing.  I'd call it GetCSSShadowArray.

>@@ -3683,6 +3685,53 @@ nsRuleNode::ComputeBorderData(void* aSta
> {
>   COMPUTE_START_RESET(Border, (mPresContext), border, parentBorder,
>                       Margin, marginData)
>+
>+  // -moz-box-shadow: none, list, inherit, initial
>+  nsCSSValueList* list = marginData.mBoxShadow;

Any chance of sharing code here?

>+    // Decide what to do with regards to box-shadow
>+    if ((!mBoxShadowArray && !aOther.mBoxShadowArray) ||
>+        mBoxShadowArray == aOther.mBoxShadowArray)
>+      return NS_STYLE_HINT_NONE;
>+
>+    if (!mBoxShadowArray || !aOther.mBoxShadowArray ||
>+        (mBoxShadowArray->Length() != aOther.mBoxShadowArray->Length()))
>+      return NS_STYLE_HINT_REFLOW;
>+
>+    for (PRUint32 i = 0; i < mBoxShadowArray->Length(); ++i) {
>+      if (*mBoxShadowArray->ShadowAt(i) != *aOther.mBoxShadowArray->ShadowAt(i))
>+        return NS_STYLE_HINT_REFLOW;
>+    }

And here?

>diff -r 7b6aaf10b6db layout/style/nsStyleStruct.h
>+  nsRefPtr<nsCSSShadowArray> mBoxShadowArray; // [reset] NULL in case of a zero-length

I'd call it mBoxShadow, not mBoxShadowArray.

"NULL in case of zero-length" should probably be "null for 'none'".

And nsStyleText::mShadowArray should probably be mTextShadow.
Comment 15 Michael Ventnor 2008-06-18 19:24:36 PDT
The spread radius seems pretty clear to me. I'm just using Outset() on the shadow's gfxRect.

Even if it isn't clear, this is -moz-box-shadow, not box-shadow. I think the W3 wants some reference implementations so that both they and the web design community know what parts of the spec can be implemented and are practical.
Comment 16 Michael Ventnor 2008-06-18 23:20:02 PDT
Created attachment 325708 [details] [diff] [review]
Parser patch 2

Passes Mochitests here.
Comment 17 Michael Ventnor 2008-06-19 18:17:51 PDT
Created attachment 325886 [details] [diff] [review]
Rendering patch

This renders the box shadows.

I'll make reftests once both the rendering and parsing patches are reviewed (but before checkin).
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-19 18:55:48 PDT
(In reply to comment #17)
> I'll make reftests once both the rendering and parsing patches are reviewed
> (but before checkin).

You should really write tests before requesting review.  You should be finding bugs in your patch when you write tests.  If you're not, it probably means the tests aren't good enough.
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-06-19 18:56:51 PDT
(In reply to comment #18)
> You should really write tests before requesting review.  You should be finding

But note that "before requesting review" doesn't mean "before soliciting feedback on the general approach".
Comment 20 Michael Ventnor 2008-06-30 21:32:08 PDT
Created attachment 327536 [details] [diff] [review]
Rendering patch 2

This is much more efficient since it only makes one temporary surface max under any conditions.
Comment 21 Michael Ventnor 2008-06-30 21:39:50 PDT
Created attachment 327537 [details] [diff] [review]
Rendering patch 2.01

Forgot to nuke a now-unused variable.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-30 21:51:55 PDT
For large box-shadows you should be adding to the frames' overflow area somehow. I don't see you doing that. You may need to extend nsFrame::FinishAndStoreOverflow.
Comment 23 Michael Ventnor 2008-06-30 22:29:08 PDT
(In reply to comment #22)
> For large box-shadows you should be adding to the frames' overflow area
> somehow. I don't see you doing that. You may need to extend
> nsFrame::FinishAndStoreOverflow.
> 

What do you mean? outlineRect becomes the overflow rect in FinishAndStoreOverflow, as I see it.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-01 11:56:58 PDT
It does. You need to change that to ensure that the overflow rect includes the box-shadow. Right now you have repaint bugs if the shadow sticks far outside the element's normal box.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-01 16:07:00 PDT
Sorry, I totally missed your changes to FinishAndStoreOverflow.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-01 16:36:53 PDT
The new code in FinishAndStoreOverflow should be moved to a helper function, FinishAndStoreOverflow is going to get too complicated otherwise.

+    nsRect originalRect = outlineRect;

Shouldn't this be the border-box for the frame (relative to the frame origin, so nsRect(nsPoint(0,0), aNewSize)? We don't draw the shadow for overflowing children or the outline.

Surely the "extract border radius and convert to pixels" code can be shared.

+  gfxRect frameGfxRect = gfxRect(RectToGfxRect(frameRect, twipsPerPixel));
+    gfxRect shadowRect = gfxRect(RectToGfxRect(frameRect, 1));

Why the extra gfxRect copy constructor here?

I think you can simplify the code quite a bit by, instead of using OPERATOR_CLEAR to punch the frame border-area out of the shadow, clipping that area out before you do the Mask operation. That just means drawing the path for that area *counter-clockwise* (e.g. using DoRoundedRectCCWSubPath) and then Clip(). That should eliminate the need for changes to nsContextBoxBlur and special handling for the no-blur case.
Comment 27 Michael Ventnor 2008-07-01 20:16:57 PDT
Created attachment 327732 [details] [diff] [review]
Rendering patch 3

Fixes comments, hopefully.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-01 20:41:53 PDT
+nsCSSRendering::GetBorderRadiusTwips(const nsStyleSides& aBorderRadius,
+                                     const nscoord& aFrameWidth,
+                                     PRInt32 aTwipsRadii[4])

aTwipsRadii should be nscoords.

+  PRBool hasAbsPosClip;
+  nsRect absPosClipRect;
+  hasAbsPosClip = GetAbsPosClipRect(GetStyleDisplay(), &absPosClipRect, aNewSize);
+  if (hasAbsPosClip) {
+    overflowRect.IntersectRect(overflowRect, absPosClipRect);
+  }

You should apply this after box-shadow since it will clip the box-shadow.

+  nsRect GetAdditionalOverflow(nsRect& aOverflowArea, nsSize& aNewSize);

const references for the parameters

+  gfxRect frameGfxRect = gfxRect(RectToGfxRect(frameRect, twipsPerPixel));

You're still doing this unnecessary copy construction.

+    nsRefPtr<gfxContext> shadowContext = nsnull;

Don't need "= nsnull"

+    if (!aIsBackgroundOpaque) {
+      // Clip out the area of the actual frame so the shadow is not shown within
+      // the frame
+      renderContext->NewPath();
+      renderContext->Rectangle(shadowRectPlusBlur);
+      if (hasBorderRadius)
+        DoRoundedRectCWSubPath(renderContext, frameGfxRect, borderRadii);
+      else
+        renderContext->Rectangle(frameGfxRect);
+      renderContext->SetFillRule(gfxContext::FILL_RULE_EVEN_ODD);
+      renderContext->Clip();
+    }

It's probably not worth doing this optimization. Just drop the conditional and then you can remove the isbackgroundopaque parameter.

+    // Draw the shape of the frame so it can be blurred
+    shadowContext->NewPath();
+    if (hasBorderRadius)
+      DoRoundedRectCWSubPath(shadowContext, shadowRect, borderRadii);
+    else
+      shadowContext->Rectangle(shadowRect);
+    shadowContext->Fill();

This is tricky. In the no-blur case we're filling with gfxRGBA(shadowColor), but in the blur case we're filling with the default color (solid black). It turns out that that's what you want in each case, but you really should document in nsContextBoxBlur and mention in a comment here that the fill pattern must be set up on renderContext before you draw into shadowContext and the right fill --- which is possibly different --- will be used to Fill shadowContext.

Looks good otherwise.
Comment 29 Michael Ventnor 2008-07-01 21:29:48 PDT
Created attachment 327741 [details] [diff] [review]
Rendering patch 4

Fixes additional comments.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-01 21:36:11 PDT
Comment on attachment 327741 [details] [diff] [review]
Rendering patch 4

But of course, you need tests.
Comment 31 Michael Ventnor 2008-07-02 02:31:04 PDT
Created attachment 327769 [details] [diff] [review]
Reftests
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-02 14:44:05 PDT
Comment on attachment 325708 [details] [diff] [review]
Parser patch 2

>diff -r 7b6aaf10b6db dom/public/idl/css/nsIDOMCSS2Properties.idl
>@@ -470,6 +470,9 @@ interface nsIDOMNSCSS2Properties : nsIDO
>                                         // raises(DOMException) on setting
> 
>            attribute DOMString        MozBoxPack;
>+                                        // raises(DOMException) on setting
>+
>+           attribute DOMString        MozBoxShadow;
>                                         // raises(DOMException) on setting
> 
>            attribute DOMString        MozBoxSizing;

Please add the new attribute to the end rather than the middle.

>diff -r 7b6aaf10b6db layout/style/nsCSSParser.cpp
>+  nsCSSValueList* ParseCSSShadowList(nsresult& aErrorCode,
>+                                     PRBool aUsesSpread = PR_FALSE);

Don't use default parameters here.  You should avoid them unless you have a really good reason to want them.  With two callers (one that wants each choice), they should just both have to specify the parameter explicitly.

>@@ -6449,6 +6457,7 @@ PRBool CSSParserImpl::ParseTextShadow(ns
>     IndexX,
>     IndexY,
>     IndexRadius,
>+    IndexSpread,
>     IndexColor
>   };
> 
>@@ -6474,7 +6483,7 @@ PRBool CSSParserImpl::ParseTextShadow(ns
>     nsCSSUnit unit = cur->mValue.GetUnit();
>     if (unit != eCSSUnit_None && unit != eCSSUnit_Inherit &&
>         unit != eCSSUnit_Initial) {
>-      nsRefPtr<nsCSSValue::Array> val = nsCSSValue::Array::Create(4);
>+      nsRefPtr<nsCSSValue::Array> val = nsCSSValue::Array::Create(5);
>       if (!val) {
>         aErrorCode = NS_ERROR_OUT_OF_MEMORY;
>         break;

As a followup it might be worth using a 4-element array for text-shadow and only using a 5-element array for box-shadow.  (IndexSpread would need to be last.)  Please DO NOT do that as part of a revision of this patch, though.

>@@ -6528,6 +6537,24 @@ PRBool CSSParserImpl::ParseTextShadow(ns
>         continue;
>       }
> 
>+      // Now look for the optional spread
>+      PRBool haveSpread = PR_FALSE;
>+      if (GetToken(aErrorCode, PR_TRUE)) {
>+        haveSpread = mToken.IsDimension();
>+        UngetToken();
>+      }

This doesn't work because it will cause a spread of unitless zero to be a parse error.  You need to just try the ParseVariant call.

>+
>+      if (aUsesSpread && haveSpread) {
>+        if (!ParseVariant(aErrorCode, val->Item(IndexSpread), VARIANT_LENGTH,
>+                          nsnull)) {
>+          break;
>+        }
>+        if (ExpectSymbol(aErrorCode, ',', PR_TRUE)) {
>+          // Go to next value
>+          continue;
>+        }
>+      }


>+PRBool CSSParserImpl::ParseTextShadow(nsresult& aErrorCode)
>+{
>+  nsCSSValueList* list = ParseCSSShadowList(aErrorCode);
>+  if (list) {
>     mTempData.SetPropertyBit(eCSSProperty_text_shadow);
>     mTempData.mText.mTextShadow = list;
>+    return PR_TRUE;
>+  }
>+  return PR_FALSE;
>+}

For both of these functions (ParseTextShadow and ParseBoxShadow), I'd reverse things so that the normal flow of the function is non-indented:

  if (!list) {
    return PR_FALSE;
  }
  mTempData.SetPropertyBit(eCSSProperty_text_shadow);
  mTempData.mText.mTextShadow = list;
  return PR_TRUE;
  
>diff -r 7b6aaf10b6db layout/style/nsComputedDOMStyle.cpp
>+nsresult
>+nsComputedDOMStyle::GetBoxShadow(nsIDOMCSSValue** aValue)
>+{
>+  const nsStyleBorder* border = GetStyleBorder();
>+
>+  if (!border->mBoxShadow) {
>+    nsROCSSPrimitiveValue *val = GetROCSSPrimitiveValue();
>+    val->SetIdent(nsGkAtoms::none);
>+    return CallQueryInterface(val, aValue);
>+  }
>+
>+  nsDOMCSSValueList *valueList = GetCSSShadowArray(border->mBoxShadow,
>+                                                   GetStyleColor()->mColor,
>+                                                   PR_TRUE);
>+  NS_ENSURE_TRUE(valueList, NS_ERROR_OUT_OF_MEMORY);
>+
>+  return CallQueryInterface(valueList, aValue);
>+}

I don't see why the code to output 'none' and the final CallQueryInterface can't be part of GetCSSShadowArray too.  Couldn't it just return nsresult and have an out parameter just like the functions it's a helper for?  (That should also reduce the diffs and make it easier to review the changes between old and new, which I haven't done.)

>diff -r 7b6aaf10b6db layout/style/nsComputedDOMStyle.h
>+  nsDOMCSSValueList* GetCSSShadowArray(nsCSSShadowArray* aArray,
>+                                       const nscolor& aDefaultColor,
>+                                       PRBool aUsesSpread = PR_FALSE);

Get rid of the default parameter here too.

>diff -r 7b6aaf10b6db layout/style/nsRuleNode.cpp
>+  // -moz-box-shadow: none, list, inherit, initial
>+  nsCSSValueList* list = marginData.mBoxShadow;
>+  if (list) {
>+    // This handles 'none' and 'initial'
>+    border->mBoxShadow = nsnull;
>+
>+    if (eCSSUnit_Inherit == list->mValue.GetUnit()) {
>+      inherited = PR_TRUE;
>+      border->mBoxShadow = parentBorder->mBoxShadow;
>+    } else if (eCSSUnit_Array == list->mValue.GetUnit()) {

Seems like everything from here...

>+      // List of arrays
>+      PRUint32 arrayLength = 0;
>+      for (nsCSSValueList *list2 = list; list2; list2 = list2->mNext)
>+        ++arrayLength;
>+
>+      NS_ASSERTION(arrayLength > 0, "Non-null -moz-box-shadow list, yet we counted 0 items.");
>+      border->mBoxShadow = new(arrayLength) nsCSSShadowArray(arrayLength);
>+      if (border->mBoxShadow) {
>+        GetShadowData(list, aContext, border->mBoxShadow, PR_TRUE, inherited);
>+      }

... to here could be inside of GetShadowData.

>diff -r 7b6aaf10b6db layout/style/nsStyleStruct.cpp
>@@ -438,8 +449,11 @@ nsChangeHint nsStyleBorder::CalcDifferen
>       }
>     }
> 
>+    // Decide what to do with regards to box-shadow
>+    if (!mBoxShadow)
>+      return NS_STYLE_HINT_NONE;

This is wrong.  aOther.mBoxShadow could be non-null.  You probably meant:

if (!mBoxShadow) {
  return aOther.mBoxShadow ? NS_STYLE_HINT_REFLOW
                           : NS_STYLE_HINT_NONE;
}

although it would probably be better to make CalcShadowDifference a static method that handles either pointer being null.

>+    return mBoxShadow->CalcShadowDifference(aOther.mBoxShadow);

(Same for mTextShadow.)
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-02 14:49:43 PDT
(In reply to comment #32)
> This doesn't work because it will cause a spread of unitless zero to be a parse
> error.  You need to just try the ParseVariant call.

And please add a test for this to property_database.js.
Comment 34 Michael Ventnor 2008-07-02 19:07:11 PDT
Created attachment 327903 [details] [diff] [review]
Parser patch 3

Fix comments.
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-03 14:22:33 PDT
Comment on attachment 327903 [details] [diff] [review]
Parser patch 3

>@@ -6526,6 +6535,21 @@ PRBool CSSParserImpl::ParseTextShadow(ns
>       if (ExpectSymbol(aErrorCode, ',', PR_TRUE)) {
>         // Go to next value
>         continue;
>+      }
>+
>+      // Now look for the optional spread
>+      if (aUsesSpread && GetToken(aErrorCode, PR_TRUE)) {
>+        UngetToken();

Drop both the GetToken and the UngetToken; ParseVariant (and ExpectSymbol) will do the GetToken that you need.

>+        if (!ParseVariant(aErrorCode, val->Item(IndexSpread), VARIANT_LENGTH,
>+                          nsnull)) {
>+          // This is an optional property, so we ignore our error here.
>+          // The color parser will decide what punishment the rule will take.
>+          CLEAR_ERROR();
>+        }
>+        if (ExpectSymbol(aErrorCode, ',', PR_TRUE)) {
>+          // Go to next value
>+          continue;
>+        }


>+nsComputedDOMStyle::GetCSSShadowArray(nsCSSShadowArray* aArray,
>+                                      const nscolor& aDefaultColor,
>+                                      PRBool aUsesSpread,
>+                                      nsIDOMCSSValue** aValue)
>+{
>+  if (!aArray) {
>+    nsROCSSPrimitiveValue *val = GetROCSSPrimitiveValue();
>+    val->SetIdent(nsGkAtoms::none);
>+    return CallQueryInterface(val, aValue);
>+  }
>+
>+  static const nsStyleCoord nsCSSShadowItem::*shadowValues[] = {
>+    &nsCSSShadowItem::mXOffset,
>+    &nsCSSShadowItem::mYOffset,
>+    &nsCSSShadowItem::mRadius
>+  };

What you can do here is this:

  static nsStyleCoord nsCSSShadowItem::* const shadowValuesNoSpread[] = {
    &nsCSSShadowItem::mXOffset,
    &nsCSSShadowItem::mYOffset,
    &nsCSSShadowItem::mRadius
  };

  static nsStyleCoord nsCSSShadowItem::* const shadowValuesWithSpread[] = {
    &nsCSSShadowItem::mXOffset,
    &nsCSSShadowItem::mYOffset,
    &nsCSSShadowItem::mRadius,
    &nsCSSShadowItem::mSpread
  };

  nsStyleCoord nsCSSShadowItem::* const * shadowValues;
  PRUint32 shadowValuesLength;
  if (aUsesSpread) {
    shadowValues = shadowValuesWithSpread;
    shadowValuesLength = NS_ARRAY_LENGTH(shadowValuesWithSpread);
  } else {
    shadowValues = shadowValuesNoSpread;
    shadowValuesLength = NS_ARRAY_LENGTH(shadowValuesNoSpread);
  }

And then remove this piece:

>+    // The spread used in box-shadow
>+    if (aUsesSpread) {
>+      val = GetROCSSPrimitiveValue();
>+      if (!val || !itemList->AppendCSSValue(val)) {
>+        delete val;
>+        delete valueList;
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+      SetValueToCoord(val, item->mSpread);
>+    }

>+nsresult
>+nsComputedDOMStyle::GetBoxShadow(nsIDOMCSSValue** aValue)
>+{
>+  const nsStyleBorder* border = GetStyleBorder();
>+
>+  return GetCSSShadowArray(border->mBoxShadow,
>+                           GetStyleColor()->mColor,
>+                           PR_TRUE, aValue);

Put the GetStyleBorder() call inside the function parameter just like the GetStyleColor() call.

(twice)
Comment 36 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-03 14:40:32 PDT
Comment on attachment 327903 [details] [diff] [review]
Parser patch 3

It looks like you dropped nsHTMLContainerFrame.cpp changes that shouldn't have been dropped.

I realize you can simplify the parser code even more.  To handle the optional radius and optional spread, you can replace the existing code:

      // Peek the next token to determine whether it's the radius or the color
      // EOF is fine too (properties can end in EOF)
      PRBool haveRadius = PR_FALSE;
      if (GetToken(aErrorCode, PR_TRUE)) {
        // The radius is a length, and all lengths are dimensions
        haveRadius = mToken.IsDimension();
        // Now unget the token, we didn't consume it
        UngetToken();
      }

      if (haveRadius) {
        // Optional radius
        if (!ParseVariant(aErrorCode, val->Item(IndexRadius), VARIANT_LENGTH,
                          nsnull)) {
          break;
        }
      }

      // Might be at a comma now
      if (ExpectSymbol(aErrorCode, ',', PR_TRUE)) {
        // Go to next value
        continue;
      }

      if (!haveColor) {
        // Now, we are either at the end of the property, or have a color (or
        // have an error)

        if (ExpectEndProperty(aErrorCode)) {
          atEOP = PR_TRUE;
        } else {
          // Clear the error from ExpectEndProperty - not a real error (if we
          // have a color here)
          CLEAR_ERROR();

          // Since we're not at the end of the property, we must have a color,
          // or report an error.
          if (!ParseVariant(aErrorCode, val->Item(IndexColor), VARIANT_COLOR,
                            nsnull)) {
            break;
          }

          if (ExpectSymbol(aErrorCode, ',', PR_TRUE)) {
            // Parse next value
            continue;
          }
        }
      }


with the following replacement which also handles the spread:

      // Optional radius (ignore errors)
      ParseVariant(aErrorCode, val->Item(IndexRadius), VARIANT_LENGTH,
                   nsnull);

      if (aHaveSpread) {
        // Optional spread (ignore errors)
        ParseVariant(aErrorCode, val->Item(IndexSpread), VARIANT_LENGTH,
                     nsnull);
      }

      if (!haveColor) {
        // Optional color (ignore errors)
        ParseVariant(aErrorCode, val->Item(IndexColor), VARIANT_COLOR,
                     nsnull);
      }

      // Might be at a comma now
      if (ExpectSymbol(aErrorCode, ',', PR_TRUE)) {
        // Go to next value
        continue;
      }

and that also lets you remove the "atEOP" variable.

With the changes in the previous comment and the ones in this one (assuming the above makes sense), r+sr=dbaron
Comment 37 Michael Ventnor 2008-07-03 17:06:23 PDT
Created attachment 328071 [details] [diff] [review]
Parser patch 4

Fix final comments.
Comment 38 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-07-06 16:57:42 PDT
Comment on attachment 328071 [details] [diff] [review]
Parser patch 4

>+    if (!ExpectEndProperty(aErrorCode)) {
>+      // We expect no more values
>       break;
>     }

I'm not sure what "We expect no more values" means here.  I think it would make more sense to say "If we don't have a comma to delimit the next value, we better be at the end of the property.  Otherwise we've hit something else, which is an error."
Comment 39 Michael Ventnor 2008-07-06 19:50:04 PDT
Created attachment 328288 [details] [diff] [review]
Parser patch 4.1
Comment 40 Reed Loden [:reed] (use needinfo?) 2008-07-07 18:02:01 PDT
Pushed to mozilla-central:

Changesets:
2ac5fa6f9090 (parsing support)
b332a386f59e (rendering support)
7b54ff2319f6 (reftests)
Comment 41 Henrik Skupin (:whimboo) 2008-07-18 04:45:49 PDT
(In reply to comment #40)
> Changesets:
> 2ac5fa6f9090 (parsing support)
> b332a386f59e (rendering support)
> 7b54ff2319f6 (reftests)

I'm sure the latter one implies in-testsuite+.

Comment 42 Henrik Skupin (:whimboo) 2008-07-19 01:36:06 PDT
I tried to verify the implementation of box-shadow but it's not visible at all with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008071806 Minefield/3.1a1pre ID:2008071806

I used the above testcase (attachment 133322 [details]) for reference. Opening the page only shows a border around the strong elements but no shadow. I enabled Firebug to have a look at the CSS rules. The result is surprising. Why is the box-shadow property gone? The CSS tab only shows following:

strong {
  border:thin solid #000000;
}

Even after manually re-adding "box-shadow:0.2em 0.2em #ccc;" to the strong tag the shadow is not visible.

Why the reftests are using "-moz-box-shadow" instead of "box-shadow"?

=> Reopening.
Comment 43 philippe (part-time) 2008-07-19 01:41:59 PDT
Using "-moz-box-shadow" instead of "box-shadow" is by design, The spec is still a draft (similarly, Webkit uses -webkit-box-shadow).
Comment 44 Michael Ventnor 2008-07-19 02:00:08 PDT
What philippe said.
Comment 45 Henrik Skupin (:whimboo) 2008-07-19 02:53:11 PDT
Thanks for clarification. I missed that point. Now I can verify the "-moz-box-shadow" with following builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071603 Minefield/3.1a1pre ID:2008071603

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre)
Gecko/2008071806 Minefield/3.1a1pre ID:2008071806

Verified.
Comment 46 Hans Schmucker 2008-07-26 16:10:45 PDT
Maybe we should think about reopening it (or should I file a seperate bug?), because it seems to interfere with selections for some reason. I've attached a screenshot of Trunk 2008072503 on WindowsXP. It's not really critical, but still pretty irritating for an enduser.
Comment 47 Hans Schmucker 2008-07-26 16:11:25 PDT
Created attachment 331446 [details]
Screenshot of Selection bug
Comment 48 Ryan VanderMeulen [:RyanVM] 2008-07-26 16:11:45 PDT
File a new bug and mark it blocking this one.
Comment 49 Hans Schmucker 2008-07-27 05:56:13 PDT
Done #448182 "-moz-box-shadow overlays selection highlighting on left and top of element"
Comment 50 Darren Kalck [:D-Kalck] 2010-02-26 03:02:12 PST
Created attachment 429094 [details]
-moz-box-shadow overlay

I have a problem, before filing a new bug, I wanted to know if it's intended or not. The first box has a z-index of 100, but the shadow of the box below is still over.
Comment 51 philippe (part-time) 2010-02-26 03:22:03 PST
(In reply to comment #50)
> Created an attachment (id=429094) [details]
> -moz-box-shadow overlay
> 
> I have a problem, before filing a new bug, I wanted to know if it's intended or
> not. The first box has a z-index of 100, but the shadow of the box below is
> still over.

That is correct. z-index is only applied to positioned elements (and thus in your testcase, it has no effect)
http://www.w3.org/TR/CSS21/visuren.html#propdef-z-index

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