212633 - Add support for CSS3 box-shadow

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Erik Arvidsson]

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

Attachment: 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.]

Is any of the work in bug 10713 applicable to this one?

Comment 3

[Sierk Bornemann]

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]

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]

Attachment: 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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

And you also need to bump the IID of nsIDOMNSCSS2Properties.

Comment 8

[Michael Ventnor]

Attachment: Parser patch 1.1

I always forget that.

Comment 9

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

Did the mochitests pass?

Comment 10

[Michael Ventnor]

Yes.

Comment 11

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

Also, what spec have you been using?  Probably http://dev.w3.org/csswg/css3-background/#the-box-shadow is best.

Comment 12

[Michael Ventnor]

Argh, didn't notice that. OK, will update the patch.

Comment 13

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

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-4, no longer working on Mozilla)]

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]

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]

Attachment: Parser patch 2

Passes Mochitests here.

Comment 17

[Michael Ventnor]

Attachment: 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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

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

Attachment: Rendering patch 2

This is much more efficient since it only makes one temporary surface max under any conditions.

Comment 21

[Michael Ventnor]

Attachment: Rendering patch 2.01

Forgot to nuke a now-unused variable.

Comment 22

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

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]

(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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

Sorry, I totally missed your changes to FinishAndStoreOverflow.

Comment 26

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

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]

Attachment: Rendering patch 3

Fixes comments, hopefully.

Comment 28

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

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

Attachment: Rendering patch 4

Fixes additional comments.

Comment 30

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

Comment on attachment 327741 [details] [diff] [review]
Rendering patch 4

But of course, you need tests.

Comment 31

[Michael Ventnor]

Attachment: Reftests

Comment 32

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

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-4, no longer working on Mozilla)]

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

Attachment: Parser patch 3

Fix comments.

Comment 35

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

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-4, no longer working on Mozilla)]

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]

Attachment: Parser patch 4

Fix final comments.

Comment 38

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

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]

Attachment: Parser patch 4.1

Comment 40

[Reed Loden [:reed]]

Pushed to mozilla-central:

Changesets:
2ac5fa6f9090 (parsing support)
b332a386f59e (rendering support)
7b54ff2319f6 (reftests)

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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][⌚️UTC+2]]

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

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]

What philippe said.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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]

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]

Attachment: Screenshot of Selection bug

Comment 48

[Ryan VanderMeulen [:RyanVM]]

File a new bug and mark it blocking this one.

Comment 49

[Hans Schmucker]

Done #448182 "-moz-box-shadow overlays selection highlighting on left and top of element"

Comment 50

[Darren Kalck [:D-Kalck]]

Attachment: -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)]

(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