WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
58511
CSS box-shadow default color should be something other than transparent
https://bugs.webkit.org/show_bug.cgi?id=58511
Summary
CSS box-shadow default color should be something other than transparent
noel gordon
Reported
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?
Attachments
Patch
(4.50 KB, patch)
2011-04-13 21:55 PDT
,
noel gordon
no flags
Details
Formatted Diff
Diff
Patch
(3.42 KB, patch)
2012-04-09 10:06 PDT
,
Dave Tharp
no flags
Details
Formatted Diff
Diff
Testcase for textshadow with unspecified shadow color
(920 bytes, text/html)
2012-04-09 19:49 PDT
,
Dave Tharp
no flags
Details
Patch
(5.10 KB, patch)
2012-04-10 09:15 PDT
,
Dave Tharp
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
noel gordon
Comment 1
2011-04-13 21:55:55 PDT
Created
attachment 89531
[details]
Patch
Simon Fraser (smfr)
Comment 2
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).
Simon Fraser (smfr)
Comment 3
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
Simon Fraser (smfr)
Comment 4
2011-05-27 21:33:38 PDT
***
Bug 61622
has been marked as a duplicate of this bug. ***
Simon Fraser (smfr)
Comment 5
2012-02-07 06:18:08 PST
***
Bug 77981
has been marked as a duplicate of this bug. ***
Simon Fraser (smfr)
Comment 6
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).
Simon Fraser (smfr)
Comment 7
2012-04-05 16:26:42 PDT
***
Bug 83210
has been marked as a duplicate of this bug. ***
Simon Fraser (smfr)
Comment 8
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'
Dave Tharp
Comment 9
2012-04-05 16:39:11 PDT
***
Bug 83223
has been marked as a duplicate of this bug. ***
Dave Tharp
Comment 10
2012-04-09 10:06:56 PDT
Created
attachment 136249
[details]
Patch
Simon Fraser (smfr)
Comment 11
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?
Dave Tharp
Comment 12
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.
Dave Tharp
Comment 13
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?
Tab Atkins Jr.
Comment 14
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.)
j.j.
Comment 15
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
Dave Tharp
Comment 16
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.
Dave Tharp
Comment 17
2012-04-10 09:15:53 PDT
Created
attachment 136467
[details]
Patch
WebKit Review Bot
Comment 18
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
>
WebKit Review Bot
Comment 19
2012-04-10 14:41:32 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug