248472 - Use white text for dark calendar colors for better readability

Status: VERIFIED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Stefan Borggraefe]

Steps to reproduce:
1. File|New Calendar
2. Enter a calendar name and select a dark color (let's say black ;-) )
3. Close the Calendar window and open it again (because of bug 248467)

Result:
The newly created calendar is rendered with black text on black background, so
you can't read its name. When you create events for this calendar, you can't
read them either.

Expected Result:
Use white text instead of black text when a darker Calendar color is used, so
you can (better) read your calendar names and entries. I think this would be a
better solution compared to bug 243923.

Comment 1

[Stefan Borggraefe]

Attachment: Patch

Comment 2

[Stefan Borggraefe]

Comment on attachment 152068 [details] [diff] [review]
Patch

Mostafah: This patch implements what I proposed. What do you think?

I chose a treshold of 256 after I tested some different values.

Comment 3

[Justin Wood (:Callek)]

I'm not mostafah but I r=- since a user *can* change the style based on
important rules and such, above our useage.

we should do a getComputerStyle if this is done, I also don't personally like
the method used.

But O well.

Comment 4

[Stefan Borggraefe]

(In reply to comment #3)
> I'm not mostafah but I r=- since a user *can* change the style based on
> important rules and such, above our useage.
> 
> we should do a getComputerStyle if this is done,

I don't really see the problem here. The user still can assing other colors to
calendars if he/she likes to. I just tried

.calendar0 {
  background-color: black !important;
  color : red !important;
}

in userChrome.css and it still works with my patch. Why use getComputedStyle()
here? What is the advantage compared to read the background-color from the RDF
and calculate the text-color accordingly?

> I also don't personally like the method used.

Why? What's wrong with it? I don't like the fact that the code is duplicated in
two files, but so is the rest of the color assignment code.

Sorry, I tried to understand your concerns, but I don't really see the problem
here. Can you explain it a little more? Perhaps I'm just to tired to see it. ;-)
Thanks! :-)

Comment 5

[Mostafa Hosseini]

Comment on attachment 152068 [details] [diff] [review]
Patch

Isn't there a more scientific definition of a dark color though?

Comment 6

[Stefan Borggraefe]

(In reply to comment #5)
> Isn't there a more scientific definition of a dark color though?

I don't know about a better definition either, but the simple algorithm I used
works well in my opinion.

Thanks for the review! :-) I'll wait with the checkin a bit so Justin gets a
chance to answer.

Comment 7

[Justin Wood (:Callek)]

My issue is if someone uses a userStyleSheet to set the background transparant
!important, then with your color adjust from the rdf it would not feasibly work
correctly, as "whats underneath" could be different.

As a first example.  But I do agree that it is few and far between, and if we do
use a dark background color "something" needs to be available.

My personal method (which is not possible at the moment) would be something
similar to the method described at the following link, with its corresponding
CSS property:
http://www.w3.org/TR/2003/CR-css3-text-20030514/#text-shadows

Again that preferred way is not possible, it (would) allow us to create a
contrast around the text itself without changing the inert text.

In closing, my preffered would have been getComputedStyle, but seeing how it is
such a slim case, I don't know if it is worth its means, though any
error-catching (light color) is better than no-error-checking (currently
unreadable text).

I leave it in your discretion to commit or change patch.

Comment 8

[Stefan Borggraefe]

(In reply to comment #7)
> My issue is if someone uses a userStyleSheet to set the background transparant
> !important, then with your color adjust from the rdf it would not feasibly work
> correctly, as "whats underneath" could be different.

Just change both color and background-color in your user stylesheet and you are
in control again.

> In closing, my preffered would have been getComputedStyle, but seeing how it is
> such a slim case, I don't know if it is worth its means, though any
> error-catching (light color) is better than no-error-checking (currently
> unreadable text).

I'll check my patch in following this spirit ;-) and because I still don't see
what the win of using getComputedStyle() would be in this case. Please file a
followup bug and/or provide an alternative patch if you think it's worth it.
Perhaps I'll understand what you mean when I see a patch implementing it.

Comment 9

[Stefan Borggraefe]

Attachment: Follow-up patch

I realised only now that Day View needs some special treatment, because it set
its text-colors explicitly. Sorry for being late with this. :-(

Comment 10

[Mostafa Hosseini]

Comment on attachment 152180 [details] [diff] [review]
Follow-up patch

r=mostafah

Comment 11

[Stefan Borggraefe]

Comment on attachment 152180 [details] [diff] [review]
Follow-up patch

Second patch checked in.

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

the sum algorithm used has the result that bright red (#FF0000) will be
considered dark too, and gets a white text.
A better way would be to convert the color to HSV, and use the V (value) to see
how bright the color is. I'm not sure if that is easy to do in javascript though.

Comment 13

[Stefan Borggraefe]

Attachment: Comparing black and white text on red background (#FF0000)

(In reply to comment #12)
> the sum algorithm used has the result that bright red (#FF0000) will be
> considered dark too, and gets a white text.
> A better way would be to convert the color to HSV, and use the V (value) to
see
> how bright the color is. I'm not sure if that is easy to do in javascript
though.

This would be as easy. V is just the maximum of R, G and B.

So...

+	// sum is between 3 * 255 = 765 and 0.
+	var sum = parseInt(red, 16) + parseInt(green, 16) + parseInt(blue, 16);

+
+	// Consider all colors with a sum lower than 256 as dark backgrounds
+	// and use white as the foreground color.
+	if (sum < 256)

...would need to be changed to something like...

+	// value is between 0 and 255.
+	var  value = Math.max(Math.max(parseInt(red, 16), parseInt(green, 16)),
parseInt(blue, 16));
+
+	// Consider all colors with a value lower than 128 as dark backgrounds
+	// and use white as the foreground color.
+	if (value < 128)

Do you all think this would be better? I'm not sure. To stay at Michiel's
example I think #FF0000 is a darker color than #FFFFFF, but both colors have
the same Value. Also I think white text on red background is as readable as
black text on red background. See attached screenshot (made with a little help
from DOM Inspector ;-) ).

Comment 14

[Justin Wood (:Callek)]

Well they both are 'ok' but Black is much better (I knew there was a reason why
I didnt like the original algorithim, and now I remember why ;-)

I would suggest using the reverse of the algorithim presented in CSS3 Colors for
HSL (and HSLA) to convert to rgb, it would allow for a correct 'v' in 'hsv'
easily, this is without me peeking at the CSS3 colors right now to know if the
algorithim presented in c#13 is correct, thanks.

Re-opening based on recent comments.

Comment 15

[Stefan Borggraefe]

(In reply to comment #14)
> I would suggest using the reverse of the algorithim presented in CSS3 Colors for
> HSL (and HSLA) to convert to rgb, it would allow for a correct 'v' in 'hsv'
> easily, this is without me peeking at the CSS3 colors right now to know if the
> algorithim presented in c#13 is correct, thanks.

The formulas for Lightness (L) and Value (V) are:

L = (max(R,G,B) + min(R,G,B)) / 2
V = (max(R,G,B))

(see http://en.wikipedia.org/wiki/HLS_color_space)

So both L and V are between 0 and 255.

I already tried to use V with a treshold of 128 (see comment 13), but the
results are worse than with the current algorithm (e.g. (128, 0, 0), a fairly
dark red, is still displayed with black text).

We don't need to convert RGB to HSL to get a correct V value. The algorithm in
comment 13 calculates the V value correct already.

L looks more promising. :-) Maybe we found a more scientific definition for dark
color. :-)

The algorithm based on the L value will just calculate the L value and then
determine the font color using this value, again with 128 as the treshold.

I will attach a new patch implementing this soon.

Comment 16

[Justin Wood (:Callek)]

25% of max L value is best, give or take, half is a bit bright on most of the scale.

Comment 17

[Stefan Borggraefe]

Attachment: New patch using the L(ightness) value of the HSL color system

Michiel, Jason, what do you think?

Comment 18

[Stefan Borggraefe]

(In reply to comment #16)
> 25% of max L value is best, give or take, half is a bit bright on most of the
scale.

Sorry, I haven't seen this comment before I uploaded my new patch (which uses
50%). I liked the result with 50%, but will try 25%, too.

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

From a quick try with the gimp, i don't think the algorithm matter mich, but
more the treshold. a L of 25% is a bit dark, but 50% is bright red. It should be
somewhere inbetween.

Comment 20

[Stefan Borggraefe]

Attachment: Same as attachment 152295 with a slightly lower threshold

After a brief discussion on IRC mvl and I agreed a threshold just below 50%
would be ideal. This way colors like #FF0000, #00FF00 and #0000FF still get
black text, which looked better for these background-colors.

A lower threshhold results in black text on to dark colors.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 152299 [details] [diff] [review]
Same as attachment 152295 [details] [diff] [review] with a slightly lower threshold

Ok, looks good.

Although i noticed that the second time i changed the background color, it
didn't update without restarting. The text color was updated. But that would be
a different bug.

Comment 22

[Stefan Borggraefe]

(In reply to comment #21)
> (From update of attachment 152299 [details] [diff] [review])
> Ok, looks good.
> 
> Although i noticed that the second time i changed the background color, it
> didn't update without restarting. The text color was updated. But that would be
> a different bug.

Perhaps bug 248467?

I just checked in attachment 152299 [details] [diff] [review], so I'm marking this bug FIXED again. Thanks
for the reviews!

Comment 23

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Sunbird Only. Be afraid for your sanity!

Comment 24

[Damian Szczepanik]

verified with default windows theme

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060901 Calendar/0.3a2+