Closed Bug 519486 Opened 11 years ago Closed 11 years ago

Some themes are illegible with the text shadow implementation

Categories

(Firefox :: Theme, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: tchung, Assigned: Unfocused)

References

()

Details

(Keywords: verified1.9.2)

Attachments

(5 files, 1 obsolete file)

With the personas uplift work, the shadow behind the text can look terrible on some themes.  For example, DJ_World will completely blur the text out on the tabs and status bar completely.

See screenshot

Repro:
1) install trunk build:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20090929 Namoroka/3.6b1pre
2) go to personas staging site; DJ World theme:  http://sm-personas01.mozilla.org/en-US/persona/8819
3) Wear that theme
4) Verify text on tab bar and status bar, find bar, are completely blurred and illegible.
Blocks: 511104
OS: Mac OS X → All
Hardware: x86 → All
Arcane is another one with blurry text in the tabs.  http://sm-personas01.mozilla.org/en-US/persona/2400
flagging for blocking 3.6.  if you can get to it before 3.6, great.  if not, im fine with this going to 3.next since its only a handful of themes causing this.
Flags: blocking-firefox3.6?
Is this any different from how they look in the Personas Add-on? If not, then I think this is INVALID. If so, then it's a blocker. Either way marking blocking+ for now - we should be consistent with its implementation.
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Priority: -- → P2
(In reply to comment #5)
> Is this any different from how they look in the Personas Add-on? If not, then I
> think this is INVALID. If so, then it's a blocker. Either way marking blocking+
> for now - we should be consistent with its implementation.

Yes, its different - the Personas addon doesn't seem to add the text shadow, and the text is readable when using Personas.

A different text shadow is added depending on whether the text color is determined to be either light (black shadow) or dark (white shadow). In the case of the DJ World persona, a white shadow is applied - if a black shadow is applied, then the text is far more legible.

So hopefully the fix will be just some adjustment of how a light/dark text color is determined.
I should also point out that the text shadow will generally improve legibility of the text.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Time for some color theory! Instead of brightness (a physical property of color), this patch calculates luminance, which is closer to how the color is actually perceived (by weighting different color components differently). Additionally, based off testing numerous personas, I've also lowered the threshold from 127 down to 110.

All the examples given here are now legible, as are the other personas I've tested. I haven't yet been able to test all 10000+ personas.
Attachment #404143 - Flags: review?(dao)
Where did you get these numbers from? I'm curious, because the SVG filter spec mentions uses numbers (0.2125, 0.7154 and 0.0721).
(In reply to comment #9)
> Where did you get these numbers from? I'm curious, because the SVG filter spec
> mentions uses numbers (0.2125, 0.7154 and 0.0721).

There are several standards, depending on the output colorspace. Turns out the one in the patch is an older FCC standard. The one the SVG filter uses is newer (BT.709), and the adopted standard for the web - I'm more inclined to use that (thanks!). New patch coming up.
Attached patch Patch v1.1Splinter Review
Attachment #404143 - Attachment is obsolete: true
Attachment #404151 - Flags: review?(dao)
Attachment #404143 - Flags: review?(dao)
Comment on attachment 404151 [details] [diff] [review]
Patch v1.1

Looks good, thanks!

The status bar is still broken with <http://sm-personas01.mozilla.org/en-US/persona/2400>, but that's also the case without any text shadow.
Attachment #404151 - Flags: review?(dao) → review+
http://hg.mozilla.org/mozilla-central/rev/4270c70462b5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Attached image Halloween Party Persona
Not sure this is quite fixed on trunk and 1.9.2 branch.  Some are still causing legible issues, like "halloween party" persona.  (http://www.getpersonas.com/en-US/persona/46739) 

See screenshot.

Tested on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091111 Minefield/3.7a1pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b3pre) Gecko/20091112 Namoroka/3.6b3pre
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hm, that is pretty bad. And it looks fine using the Personas Addon. The luminosity of that persona is 105.9.

"A Happy Halloween" has the same issue (http://www.getpersonas.com/en-US/persona/47350), with a luminosity of 97.1.

As a more extreme example, "Bright Streaming" has issues too (http://www.getpersonas.com/en-US/persona/507), with a luminosity of 67.9.


While the luminosity threshold could be lowered to accommodate these, it would introduce issues with other personas that work well at the moment. Currently, the threshold is 110 - I think 90 would be the lowest I'm comfortable lowering it to. 

Dao, any thoughts on this?



I also found some personas that will have text legibility issues regardless of what we do, as their text color is very similar to the background color. For example:
http://www.getpersonas.com/en-US/persona/26284
http://www.getpersonas.com/en-US/persona/29188
We could set neither dark nor bright for a certain range, although we should only do so if this helps more themes than it hurts. Somehow themes will always be able to screw up by choosing bad colors.
(In reply to comment #17)
> We could set neither dark nor bright for a certain range, although we should
> only do so if this helps more themes than it hurts. Somehow themes will always
> be able to screw up by choosing bad colors.

Would it be helpful if we pulled a list of top persona themes used out there, and test your workaround fix to make sure these are looking right?   There will always be themes that will screw with the design, but if we can at least identify the more popular ones, we can at least get a better sampling of how bad things really are, and hopefully "fix" 90% of them.
(In reply to comment #17)
> We could set neither dark nor bright for a certain range, although we should
> only do so if this helps more themes than it hurts.

Yea, I just had that thought too. I'll play around with it this weekend.

(In reply to comment #18)
> Would it be helpful if we pulled a list of top persona themes used out there,
> and test your workaround fix to make sure these are looking right?   There will
> always be themes that will screw with the design, but if we can at least
> identify the more popular ones, we can at least get a better sampling of how
> bad things really are, and hopefully "fix" 90% of them.

Yea, that'd be helpful. At the moment I'm testing on a combination of the most popular, and a random sampling (it could be that some personas aren't as popular because of this bug).
There seems to be a bug in GetDocumentLWTheme (<http://mxr.mozilla.org/mozilla-central/source/content/xul/document/src/nsXULDocument.cpp#4763>). :-moz-lwtheme matches only if the lwthemetextcolor attribute is either bright or dark. Instead, it should just depend on the lwtheme attribute.
the most popular themes can be found here: http://www.getpersonas.com/en-US/gallery/All/Popular.

note that we are reorganizing the ordering of the gallery page from "popular" to "up and coming" which will likely have impact on which personas get the most usage.
Depends on: 528792
(In reply to comment #17)
> We could set neither dark nor bright for a certain range, although we should
> only do so if this helps more themes than it hurts. Somehow themes will always
> be able to screw up by choosing bad colors.

I had a play with this strategy using the patch from bug 528792 (described in comment 20). My comments here only apply to text that is neither obviously dark or obviously bright.

This strategy mostly works fine for themes with plain (ie, flat color) backgrounds. For themes with non-plain backgrounds, not having a textshadow is (usually) better than having the wrong textshadow, but worse than having the correct textshadow.

Of course, there's a catch: which textshadow is "correct" depends on the luminosity of the background.
Depends on: 529129
I've added bug 529129 to add a luminosity range where lwthemetextcolor should not be set.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Blair, is this really fixed on 1.9.2 and trunk?   If so, can you paste the changeset here?  Because i'm still seeing the illegible examples on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b5pre) Gecko/20091202 Namoroka/3.6b5pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091202 Minefield/3.7a1pre
http://hg.mozilla.org/mozilla-central/rev/4270c70462b5
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/860c1b4c4454

Its fixed in that any further tweaking seems to just make more themes illegible or change which themes are illegible, rather than actually reduce the number of illegible themes. Unfortunately, there's no perfect fix for this.
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b5pre) Gecko/20091203 Namoroka/3.6b5pre, Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b5pre) Gecko/20091204 Namoroka/3.6b5pre, and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091204 Minefield/3.7a1pre
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.