Bug 58511 – CSS box-shadow default color should be something other than transparent

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[noel gordon]

Webkit currently uses Color:transparent, and that is standards conformant (the UA may use any color it likes).  Not sure
why Color:transparent was used in the first place, but for web-compat (FF4.0, Opera 11, IE10preview), black seems to
be the default choice. Thoughts?

[noel gordon]

Created attachment 89531 [details]
Patch

[Simon Fraser (smfr)]

There's discussion on www-style about this. You need to be careful about compatibility (esp. with "walled garden" content like Dashboard widgets, iTunes Extras and LPs etc).

[Simon Fraser (smfr)]

Comment on attachment 89531 [details]
Patch

See http://lists.w3.org/Archives/Public/www-style/2011Mar/0060.html

[Simon Fraser (smfr)]

*** Bug 61622 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

*** Bug 77981 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

CSS WG that the default color should be the value of the color property on the element (somewhat like currentColor, ignoring inheritance).

[Simon Fraser (smfr)]

*** Bug 83210 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

See <http://lists.w3.org/Archives/Public/www-style/2012Feb/0528.html>

RESOLVED: Default color of box-shadows is value of 'color'

[Dave Tharp]

*** Bug 83223 has been marked as a duplicate of this bug. ***

[Dave Tharp]

Created attachment 136249 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 136249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136249&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:3441
> +            else if (m_style)
> +                color = m_style->color();

Has style->color() always been set by this point?

> LayoutTests/ChangeLog:8
> +        Two ietestcenter pixel tests need rebaselining due to box-shadow default color fix.

Does this change affect the computed value of box-shadow? Should text-shadow have similar logic?

[Dave Tharp]

(In reply to comment #11)
> (From update of attachment 136249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136249&action=review
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:3441
> > +            else if (m_style)
> > +                color = m_style->color();
> 
> Has style->color() always been set by this point?
I can't be  100% certain, but no test failures occur other than the new ietestcenter failures that highlighted this issue. If color is not set, the behavior defaults to the original, pre-fix behavior.

> 
> > LayoutTests/ChangeLog:8
> > +        Two ietestcenter pixel tests need rebaselining due to box-shadow default color fix.
> 
> Does this change affect the computed value of box-shadow? Should text-shadow have similar logic?
This does not change the computed value of box-shadow. It simply manages the case where the box-shadow color was not specified, per spec.  Text shadow was not considered for this fix.

[Dave Tharp]

Partial answers:
> Has style->color() always been set by this point?
In reviewing the code and the call stack, I would say yes at this point. The call stack looks like this:
0	WebCore::CSSStyleSelector::applyProperty
1	WebCore::CSSStyleSelector::applyProperties<false>
2	WebCore::CSSStyleSelector::applyMatchedProperties<false>
3	WebCore::CSSStyleSelector::applyMatchedProperties
4	WebCore::CSSStyleSelector::styleForElement
5	WebCore::Element::styleForRenderer
6	WebCore::NodeRendererFactory::createRendererIfNeeded
7	WebCore::Node::createRendererIfNeeded
8	WebCore::Element::attach
9	WebCore::executeTask
...	<More>

The call in WebCore::NodeRendererFactory::createRendererIfNeeded looks like:
    if (element)
        m_context.setStyle(element->styleForRenderer());

So I think it is reasonable to say that since we doing a setStyle, that we are in a state such that the style information has been processed and is available.  Empirically, this is the case with the test cases I've been working with, and I have run the entire LayoutTest suite against this fix and not seen any issues that would cause me to think this is an invalid assumption.

Note that *even if* there is some corner case that resulted in an un-initialized  color object, the fix is designed to "do no harm" and default to the original behaviour.

> Does this change affect the computed value of box-shadow?
No.  The fix is designed to use the color property of the element in the case that no color is specified in the box-shadow property.  This seems to be the correct behavior according to spec.  I do not think that modifying the computed value would be warranted, however I would have to admit to not being a CSS expert. Let me know if I'm wrong :)

> Should text-shadow have similar logic?
I will reviewing the spec and working up a test case for text shadow this evening, I'll post findings either later tonight or tomorrow morning. If I find that we do need a similar fix for text shadow, should we create a new bug, or include the fix with the patch on this one?

[Tab Atkins Jr.]

(In reply to comment #13)
> > Does this change affect the computed value of box-shadow?
> No.  The fix is designed to use the color property of the element in the case that no color is specified in the box-shadow property.  This seems to be the correct behavior according to spec.  I do not think that modifying the computed value would be warranted, however I would have to admit to not being a CSS expert. Let me know if I'm wrong :)

You're right (at least in the intended behavior).  A box-shadow declaration missing a color continues to miss its color in the computed value.  It only gains a color in the "used value".

(Thinking about this, we need to special-case this somehow for transitioning.  Right now, it's not defined whether you should be able to transition to/from a shadow without a color, and if you can, what it should mean.)

[j.j.]

(In reply to comment #13)

> > Should text-shadow have similar logic?
> I will reviewing the spec

The same is needed for text-shadow, see linked comments here:
http://www.w3.org/Style/CSS/Tracker/issues/222?changelog

[Dave Tharp]

Created attachment 136380 [details]
Testcase for textshadow with unspecified shadow color

Reviewing the spec for text-shadow, it is clear the same handling for box shadow applies: "Values are interpreted as for ‘box-shadow’. [CSS3BG]"

I created a test case (attached: will upload with an updated patch) which shows that the proposed fix actually handles the case of text shadow color as well.

[Dave Tharp]

Created attachment 136467 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136467 [details]
Patch

Clearing flags on attachment: 136467

Committed r113770: <http://trac.webkit.org/changeset/113770>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.