Add support for CSS3 box-shadow

VERIFIED FIXED in mozilla1.9.1a1

Status

()

Core
CSS Parsing and Computation
--
enhancement
VERIFIED FIXED
14 years ago
4 years ago

People

(Reporter: Erik Arvidsson, Assigned: Michael Ventnor)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {css3, dev-doc-complete, testcase})

Trunk
mozilla1.9.1a1
css3, dev-doc-complete, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 9 obsolete attachments)

289 bytes, application/xhtml+xml
Details
18.44 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.99 KB, patch
Details | Diff | Splinter Review
38.38 KB, patch
Details | Diff | Splinter Review
13.43 KB, image/png
Details
430 bytes, text/html
Details
(Reporter)

Description

14 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

Updated

14 years ago
Keywords: css3

Comment 1

14 years ago
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

Updated

14 years ago
Keywords: testcase

Updated

12 years ago
OS: Windows XP → All
Hardware: PC → All

Comment 2

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

Comment 3

10 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.
Assignee: dbaron → nobody
QA Contact: ian → style-system
(Assignee)

Comment 4

9 years ago
Taking, I've been working on this all day today and have a (AFAIK) perfectly working implementation. Will attach patches tomorrow.
Status: NEW → ASSIGNED
(Assignee)

Comment 5

9 years ago
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.
Assignee: nobody → ventnor.bugzilla
Attachment #325646 - Flags: superreview?(dbaron)
Attachment #325646 - Flags: review?(dbaron)
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.
Attachment #325646 - Flags: superreview?(dbaron)
Attachment #325646 - Flags: superreview-
Attachment #325646 - Flags: review?(dbaron)
Attachment #325646 - Flags: review-
And you also need to bump the IID of nsIDOMNSCSS2Properties.
(Assignee)

Comment 8

9 years ago
Created attachment 325656 [details] [diff] [review]
Parser patch 1.1

I always forget that.
Attachment #325646 - Attachment is obsolete: true
Attachment #325656 - Flags: superreview?(dbaron)
Attachment #325656 - Flags: review?(dbaron)
Did the mochitests pass?
(Assignee)

Comment 10

9 years ago
Yes.
Also, what spec have you been using?  Probably http://dev.w3.org/csswg/css3-background/#the-box-shadow is best.
(Assignee)

Comment 12

9 years ago
Argh, didn't notice that. OK, will update the patch.
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 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.
(Assignee)

Comment 15

9 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.
(Assignee)

Comment 16

9 years ago
Created attachment 325708 [details] [diff] [review]
Parser patch 2

Passes Mochitests here.
Attachment #325656 - Attachment is obsolete: true
Attachment #325708 - Flags: superreview?(dbaron)
Attachment #325708 - Flags: review?(dbaron)
Attachment #325656 - Flags: superreview?(dbaron)
Attachment #325656 - Flags: review?(dbaron)
(Assignee)

Comment 17

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

Comment 20

9 years ago
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.
Attachment #325886 - Attachment is obsolete: true
Attachment #327536 - Flags: superreview?(roc)
Attachment #327536 - Flags: review?(roc)
Attachment #325886 - Flags: superreview?(roc)
Attachment #325886 - Flags: review?(roc)
(Assignee)

Comment 21

9 years ago
Created attachment 327537 [details] [diff] [review]
Rendering patch 2.01

Forgot to nuke a now-unused variable.
Attachment #327536 - Attachment is obsolete: true
Attachment #327537 - Flags: superreview?(roc)
Attachment #327537 - Flags: review?(roc)
Attachment #327536 - Flags: superreview?(roc)
Attachment #327536 - Flags: review?(roc)
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.
(Assignee)

Comment 23

9 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.
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.
Sorry, I totally missed your changes to FinishAndStoreOverflow.
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.
(Assignee)

Comment 27

9 years ago
Created attachment 327732 [details] [diff] [review]
Rendering patch 3

Fixes comments, hopefully.
Attachment #327537 - Attachment is obsolete: true
Attachment #327732 - Flags: superreview?(roc)
Attachment #327732 - Flags: review?(roc)
Attachment #327537 - Flags: superreview?(roc)
Attachment #327537 - Flags: review?(roc)
+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.
(Assignee)

Comment 29

9 years ago
Created attachment 327741 [details] [diff] [review]
Rendering patch 4

Fixes additional comments.
Attachment #327732 - Attachment is obsolete: true
Attachment #327741 - Flags: superreview?(roc)
Attachment #327741 - Flags: review?(roc)
Attachment #327732 - Flags: superreview?(roc)
Attachment #327732 - Flags: review?(roc)
Comment on attachment 327741 [details] [diff] [review]
Rendering patch 4

But of course, you need tests.
Attachment #327741 - Flags: superreview?(roc)
Attachment #327741 - Flags: superreview+
Attachment #327741 - Flags: review?(roc)
Attachment #327741 - Flags: review+
(Assignee)

Comment 31

9 years ago
Created attachment 327769 [details] [diff] [review]
Reftests
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.)
Attachment #325708 - Flags: superreview?(dbaron)
Attachment #325708 - Flags: superreview-
Attachment #325708 - Flags: review?(dbaron)
Attachment #325708 - Flags: review-
(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.
(Assignee)

Comment 34

9 years ago
Created attachment 327903 [details] [diff] [review]
Parser patch 3

Fix comments.
Attachment #325708 - Attachment is obsolete: true
Attachment #327903 - Flags: superreview?(dbaron)
Attachment #327903 - Flags: review?(dbaron)
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 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
Attachment #327903 - Flags: superreview?(dbaron)
Attachment #327903 - Flags: superreview+
Attachment #327903 - Flags: review?(dbaron)
Attachment #327903 - Flags: review+
(Assignee)

Comment 37

9 years ago
Created attachment 328071 [details] [diff] [review]
Parser patch 4

Fix final comments.
Attachment #327903 - Attachment is obsolete: true
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."
Attachment #328071 - Flags: superreview+
Attachment #328071 - Flags: review+
(Assignee)

Comment 39

9 years ago
Created attachment 328288 [details] [diff] [review]
Parser patch 4.1
Attachment #328071 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Pushed to mozilla-central:

Changesets:
2ac5fa6f9090 (parsing support)
b332a386f59e (rendering support)
7b54ff2319f6 (reftests)
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1a1
(Assignee)

Updated

9 years ago
Keywords: dev-doc-needed

Updated

9 years ago
Depends on: 444237

Updated

9 years ago
Depends on: 445741
(In reply to comment #40)
> Changesets:
> 2ac5fa6f9090 (parsing support)
> b332a386f59e (rendering support)
> 7b54ff2319f6 (reftests)

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

Flags: in-testsuite+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Using "-moz-box-shadow" instead of "box-shadow" is by design, The spec is still a draft (similarly, Webkit uses -webkit-box-shadow).
(Assignee)

Comment 44

9 years ago
What philippe said.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED

Comment 46

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

Comment 47

9 years ago
Created attachment 331446 [details]
Screenshot of Selection bug
File a new bug and mark it blocking this one.

Updated

9 years ago
Depends on: 448182

Comment 49

9 years ago
Done #448182 "-moz-box-shadow overlays selection highlighting on left and top of element"
Depends on: 453641
Depends on: 458031
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 466353

Updated

9 years ago
Depends on: 475197

Updated

8 years ago
Depends on: 475723

Updated

8 years ago
Blocks: 514670

Updated

8 years ago
No longer blocks: 514670
Depends on: 514670

Updated

8 years ago
Depends on: 514917

Updated

8 years ago
Depends on: 485149

Updated

8 years ago
Depends on: 527695
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.
(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
Blocks: 631373

Updated

4 years ago
Depends on: 829712

Updated

4 years ago
No longer depends on: 829712

Updated

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