503188 - text-shadow uses page colors even when page colors are disabled

Status: RESOLVED FIXED

Description

[Jonadab the Unsightly One (Nathan Eady)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5

If a stylesheet for a web page specifies a text-shadow property with a specific color, Firefox (and probably any other Gecko product, I imagine) uses that color, even if page colors are disabled.

Reproducible: Always

Steps to Reproduce:
1. Edit->Preferences->Content->Colors
2. Make sure "Allow pages to choose their own colors, instead of my selections above" is *unchecked*.  Close the prefs.
3. Visit any page that has text styled with a text-shadow in a color that doesn't go with your colors.
Actual Results:  
The shadow is displayed in the color the stylesheet specified, even though all other page elements use the user's colors.

Expected Results:  
The shadow should use a color derived from the text and background colors in some fashion.  (My vote would be, just re-use the text color, maybe with a hidden about:config pref to change it.)

I normally browse with page colors disabled, system colors set to #FFE6BC foreground on #294D4A background, so something like this really sticks out like a sore thumb.  I know, I'm weird.  But still, the software should be consistent, and if page-specified colors are disabled by the user, that means colors specified by the page should be ignored.  All of them.

Comment 1

[Mardeg]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5
Confirming. In the URL, the first line is not given a colour for text, background or text-shadow, and text-shadow is the colour chosen in prefs. In all the following lines text color and background colour from page is overridden by prefs but text-colour is not.

Comment 2

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

I suspect the right thing to do for text-shadow when the page colors pref is turned off is to disable text shadows completely; drawing them all in the default color is likely to lead to illegibility.

That should be a relatively straightforward change to nsCSSCompressedDataBlock::MapRuleInfoInto.  (Though we should really move the property lists to the flags field in nsCSSPropList.h, and just special-case background-color... or maybe get better about preserving alpha for backgrounds and borders more generally, and have two flags.)

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

That all sounds reasonable to me.

Comment 4

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 387765 [details] [diff] [review]
patch 1: put ignoring colors in the property flags

Comment 5

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 387766 [details] [diff] [review]
patch 2: ignore text-shadow when pref is to ignore colors

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 387765 [details] [diff] [review]
patch 1: put ignoring colors in the property flags

Looks ok, but with the simpler if condition can we use && instead of nested ifs and outdent the body?

Comment 7

[Mardeg]

Does this mean people that have page colors being ignored will miss out on the glory of effects like spotlight, flames, and smoke?

http://hacks.mozilla.org/2009/06/text-shadow-spotlight/
http://m8y.org/tmp/testcase63.xhtml
http://m8y.org/tmp/testcase67.xhtml

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Yes, absolutely.  It's the least of what they'll miss out on in terms of "effects"...

Comment 9

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

http://hg.mozilla.org/mozilla-central/rev/afac199a8153
http://hg.mozilla.org/mozilla-central/rev/a23d948042c9

Comment 10

[Mardeg]

Version field is 1.9.1 branch, but there is no wanted1.9.1.2 flag to set. Will this land in 3.5.x at all?

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

If you want to request branch triage, set the "status1.9.1" flag to "needstriage".

If you're trying to ask for approval to check in on the branch, request approval on the patches.

We're no longer doing per-dot-release wanted flags. See the "Changing how we use flags for branch management in Bugzilla" thread in m.d.planning.

Comment 12

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

I think we shouldn't change this on a security release branch.

Comment 13

[j.j.]

The same applies to -moz-box-shadow, no?

Comment 14

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 389071 [details] [diff] [review]
patch 3: -moz-box-shadow too, and also fix case error in test

Think we should also do this for -moz-box-shadow ?

Comment 15

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

landed as http://hg.mozilla.org/mozilla-central/rev/fdb670d3c8b3

Comment 16

[j.j.]

This should land on 1.9.1.x to avoid ugly or illegible text.

(Use of text-shadow and box-shadow may increase quickly since support of Fx 3.5)

Comment 17

[j.j.]

Created attachment 389528 [details]
testcase

Test with Fx 3.5 and your own colors (black on white) enabled.