Erik Arvidsson Reporter
	Description • 22 years ago

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

	Anne (:annevk)
	Comment 1 • 21 years ago

Test case taken from the specification (slightly modified):
http://www.w3.org/TR/css3-border/#the-box-shadow

	Greg K.
	Comment 2 • 19 years ago

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

	Sierk Bornemann
	Comment 3 • 18 years ago

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.

	Michael Ventnor Assignee
	Comment 4 • 17 years ago

Taking, I've been working on this all day today and have a (AFAIK) perfectly working implementation. Will attach patches tomorrow.

	Michael Ventnor Assignee
	Comment 5 • 17 years ago

This adds support for parsing -moz-box-shadow in the style system. Once this gets checked in I'll attach the actual rendering patch.

	David Baron :dbaron:
	Comment 6 • 17 years ago

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.

	David Baron :dbaron:
	Comment 7 • 17 years ago

And you also need to bump the IID of nsIDOMNSCSS2Properties.

	Michael Ventnor Assignee
	Comment 8 • 17 years ago

I always forget that.

	David Baron :dbaron:
	Comment 9 • 17 years ago

Did the mochitests pass?

	Michael Ventnor Assignee
	Comment 10 • 17 years ago

Yes.

	David Baron :dbaron:
	Comment 11 • 17 years ago

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

	Michael Ventnor Assignee
	Comment 12 • 17 years ago

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

	David Baron :dbaron:
	Comment 13 • 17 years ago

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 .

	David Baron :dbaron:
	Comment 14 • 17 years ago

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.

	Michael Ventnor Assignee
	Comment 15 • 17 years ago

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.

	Michael Ventnor Assignee
	Comment 16 • 17 years ago

Passes Mochitests here.

	Michael Ventnor Assignee
	Comment 17 • 17 years ago

This renders the box shadows.

I'll make reftests once both the rendering and parsing patches are reviewed (but before checkin).

	David Baron :dbaron:
	Comment 18 • 17 years ago

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

	David Baron :dbaron:
	Comment 19 • 17 years ago

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

	Michael Ventnor Assignee
	Comment 20 • 16 years ago

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

	Michael Ventnor Assignee
	Comment 21 • 16 years ago

Forgot to nuke a now-unused variable.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 16 years ago

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.

	Michael Ventnor Assignee
	Comment 23 • 16 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 16 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 25 • 16 years ago

Sorry, I totally missed your changes to FinishAndStoreOverflow.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 26 • 16 years ago

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.

	Michael Ventnor Assignee
	Comment 27 • 16 years ago

Fixes comments, hopefully.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 28 • 16 years ago

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

	Michael Ventnor Assignee
	Comment 29 • 16 years ago

Fixes additional comments.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 30 • 16 years ago

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

But of course, you need tests.

	David Baron :dbaron:
	Comment 32 • 16 years ago

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

	David Baron :dbaron:
	Comment 33 • 16 years ago

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

	Michael Ventnor Assignee
	Comment 34 • 16 years ago

Fix comments.

	David Baron :dbaron:
	Comment 35 • 16 years ago

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)

	David Baron :dbaron:
	Comment 36 • 16 years ago

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

	Michael Ventnor Assignee
	Comment 37 • 16 years ago

Fix final comments.

	David Baron :dbaron:
	Comment 38 • 16 years ago

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

	Reed Loden [:reed]
	Comment 40 • 16 years ago

Pushed to mozilla-central:

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

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 41 • 16 years ago

(In reply to comment #40)
> Changesets:
> 2ac5fa6f9090 (parsing support)
> b332a386f59e (rendering support)
> 7b54ff2319f6 (reftests)

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

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 42 • 16 years ago

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.

	philippe (part-time)
	Comment 43 • 16 years ago

Using "-moz-box-shadow" instead of "box-shadow" is by design, The spec is still a draft (similarly, Webkit uses -webkit-box-shadow).

	Michael Ventnor Assignee
	Comment 44 • 16 years ago

What philippe said.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 45 • 16 years ago

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.

	Hans Schmucker
	Comment 46 • 16 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 16 years ago

File a new bug and mark it blocking this one.

	Hans Schmucker
	Comment 49 • 16 years ago

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

	Darren Kalck [:D-Kalck]
	Comment 50 • 15 years ago

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.

	philippe (part-time)
	Comment 51 • 15 years ago

(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