Bug 58511

Created attachment 89531 [details] Patch

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 on attachment 89531 [details] Patch See http://lists.w3.org/Archives/Public/www-style/2011Mar/0060.html

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

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

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

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

See <http://lists.w3.org/Archives/Public/www-style/2012Feb/0528.html> RESOLVED: Default color of box-shadows is value of 'color'

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

Created attachment 136249 [details] Patch

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?

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

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?

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

(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

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.

Created attachment 136467 [details] Patch

Comment on attachment 136467 [details] Patch Clearing flags on attachment: 136467 Committed r113770: <http://trac.webkit.org/changeset/113770>

All reviewed patches have been landed. Closing bug.