529129 - Don't set lwthemetextcolor for lightweight themes that have text color in a mid luminosity range

Status: RESOLVED WONTFIX

(Toolkit :: UI Widgets, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bug 519486 adjusted the way dark vs bright text color is calculated, for setting lwthemetextcolor. The threshold used isn't optimal for some lightweight themes that have a text color that have a luminosity close to the threshold. These themes are getting a text-shadow that makes the text difficult to read.

Examples:
http://www.getpersonas.com/en-US/persona/46739
http://www.getpersonas.com/en-US/persona/47350
http://www.getpersonas.com/en-US/persona/40473
http://www.getpersonas.com/en-US/persona/34032


Changing the threshold would simply change which lightweight themes have issues, rather than actually reducing the number of themes that have issues. Instead, a mid-range of luminosity values can be defined to not have lwthemetextcolor set, thereby not showing a text-shadow.

The downside of this is that not having a text-shadow isn't as good as having the correct text-shadow. Unfortunately, which text-shadow (white or black) is "correct" also depends on the background color. So this may be the best realistic option, short of letting lightweight themes specify which text-shadow they should have.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

I'm not convinced this is the best solution possible, but it seems the best solution available.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Wouldn't block on it, can fix after we ship, but would likely take the patch.

Comment 3

[Dão Gottwald [:dao]]

70 seems really low. Is this for the halloween themes? Are you sure this doesn't turn off the shadow for too many cases where the shadow would help?

> http://www.getpersonas.com/en-US/persona/40473

I'm still getting the shadow here.

> http://www.getpersonas.com/en-US/persona/34032

This is largely unreadable regardless of whether there's a shadow. We shouldn't optimize for this.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #3)
> 70 seems really low. Is this for the halloween themes? Are you sure this
> doesn't turn off the shadow for too many cases where the shadow would help?

Yea, it is low. I don't think this makes it worse for themes, but I'm not convinced it makes it better either.

> > http://www.getpersonas.com/en-US/persona/40473
> 
> I'm still getting the shadow here.

Whoops - I either pasted the wrong link or uploaded a patch with the wrong threshold values.

> > http://www.getpersonas.com/en-US/persona/34032
> 
> This is largely unreadable regardless of whether there's a shadow. We shouldn't
> optimize for this.

Yea, fair enough. 

I'm wondering if we're trying to be too smart here (and failing) - and should just let the theme optionally specify whether it should have a text-shadow or not.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 412703 [details] [diff] [review]
Patch v1

per previous comments -- we shouldn't take this without being convinced that it helps more than it hurts

Comment 6

[Mike Kaply [:mkaply]]

> I'm wondering if we're trying to be too smart here (and failing) - and should
just let the theme optionally specify whether it should have a text-shadow or
not.

This seems the most logical.

Just allow a JSON attribute like textShadow: true/false

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #6)
> Just allow a JSON attribute like textShadow: true/false

Filed bug 549346 for this.