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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Jonadab the Unsightly One (Nathan Eady), Assigned: dbaron)

Tracking

1.9.1 Branch
mozilla1.9.2a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

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.

Updated

8 years ago
Component: General → Layout: Text
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Version: unspecified → 1.9.1 Branch

Comment 1

8 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All

Updated

8 years ago
Blocks: 10713
(Assignee)

Comment 2

8 years ago
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.)
Component: Layout: Text → Style System (CSS)
QA Contact: layout.fonts-and-text → style-system
That all sounds reasonable to me.
(Assignee)

Updated

8 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 4

8 years ago
Created attachment 387765 [details] [diff] [review]
patch 1: put ignoring colors in the property flags
Attachment #387765 - Flags: superreview?(bzbarsky)
Attachment #387765 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

8 years ago
Created attachment 387766 [details] [diff] [review]
patch 2: ignore text-shadow when pref is to ignore colors
Attachment #387766 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #387765 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #387766 - Flags: review?(roc)
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?
Attachment #387765 - Flags: review?(bzbarsky) → review+
Attachment #387766 - Flags: review?(bzbarsky) → review+
Attachment #387766 - Flags: review?(roc) → review+

Comment 7

8 years ago
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
Yes, absolutely.  It's the least of what they'll miss out on in terms of "effects"...
(Assignee)

Comment 9

8 years ago
http://hg.mozilla.org/mozilla-central/rev/afac199a8153
http://hg.mozilla.org/mozilla-central/rev/a23d948042c9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 10

8 years ago
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?
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.
(Assignee)

Comment 12

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

Comment 13

8 years ago
The same applies to -moz-box-shadow, no?
(Assignee)

Comment 14

8 years ago
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 ?
Attachment #389071 - Flags: review?(bzbarsky)
Attachment #389071 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

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

Comment 16

8 years ago
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

8 years ago
Created attachment 389528 [details]
testcase

Test with Fx 3.5 and your own colors (black on white) enabled.
You need to log in before you can comment on or make changes to this bug.