Bug 58511

Summary: CSS box-shadow default color should be something other than transparent
Product: WebKit Reporter: noel gordon <noel.gordon>
Component: CSSAssignee: noel gordon <noel.gordon>
Status: RESOLVED FIXED    
Severity: Normal CC: dtharp, hyatt, jackalmage, macpherson, menard, mitz, moz, rludgero, simon.fraser, tabatkins, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Testcase for textshadow with unspecified shadow color
none
Patch none

Description noel gordon 2011-04-13 21:44:47 PDT
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?
Comment 1 noel gordon 2011-04-13 21:55:55 PDT
Created attachment 89531 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-04-13 22:19:41 PDT
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).
Comment 3 Simon Fraser (smfr) 2011-04-13 22:21:07 PDT
Comment on attachment 89531 [details]
Patch

See http://lists.w3.org/Archives/Public/www-style/2011Mar/0060.html
Comment 4 Simon Fraser (smfr) 2011-05-27 21:33:38 PDT
*** Bug 61622 has been marked as a duplicate of this bug. ***
Comment 5 Simon Fraser (smfr) 2012-02-07 06:18:08 PST
*** Bug 77981 has been marked as a duplicate of this bug. ***
Comment 6 Simon Fraser (smfr) 2012-02-07 06:19:17 PST
CSS WG that the default color should be the value of the color property on the element (somewhat like currentColor, ignoring inheritance).
Comment 7 Simon Fraser (smfr) 2012-04-05 16:26:42 PDT
*** Bug 83210 has been marked as a duplicate of this bug. ***
Comment 8 Simon Fraser (smfr) 2012-04-05 16:30:20 PDT
See <http://lists.w3.org/Archives/Public/www-style/2012Feb/0528.html>

RESOLVED: Default color of box-shadows is value of 'color'
Comment 9 Dave Tharp 2012-04-05 16:39:11 PDT
*** Bug 83223 has been marked as a duplicate of this bug. ***
Comment 10 Dave Tharp 2012-04-09 10:06:56 PDT
Created attachment 136249 [details]
Patch
Comment 11 Simon Fraser (smfr) 2012-04-09 10:09:36 PDT
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?
Comment 12 Dave Tharp 2012-04-09 10:15:55 PDT
(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.
Comment 13 Dave Tharp 2012-04-09 16:52:51 PDT
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?
Comment 14 Tab Atkins Jr. 2012-04-09 17:51:38 PDT
(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.)
Comment 15 j.j. 2012-04-09 18:50:45 PDT
(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
Comment 16 Dave Tharp 2012-04-09 19:49:24 PDT
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.
Comment 17 Dave Tharp 2012-04-10 09:15:53 PDT
Created attachment 136467 [details]
Patch
Comment 18 WebKit Review Bot 2012-04-10 14:41:26 PDT
Comment on attachment 136467 [details]
Patch

Clearing flags on attachment: 136467

Committed r113770: <http://trac.webkit.org/changeset/113770>
Comment 19 WebKit Review Bot 2012-04-10 14:41:32 PDT
All reviewed patches have been landed.  Closing bug.