Closed Bug 248472 Opened 16 years ago Closed 16 years ago

Use white text for dark calendar colors for better readability

Categories

(Calendar :: Sunbird Only, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Stefan.Borggraefe, Assigned: Stefan.Borggraefe)

Details

Attachments

(4 files, 1 obsolete file)

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.
Attached patch PatchSplinter Review
Assignee: mostafah → Stefan.Borggraefe
Status: NEW → ASSIGNED
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.
Attachment #152068 - Flags: first-review?(mostafah)
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.
(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 on attachment 152068 [details] [diff] [review]
Patch

Isn't there a more scientific definition of a dark color though?
Attachment #152068 - Flags: first-review?(mostafah) → first-review+
(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.

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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attached patch Follow-up patchSplinter Review
I realised only now that Day View needs some special treatment, because it set
its text-colors explicitly. Sorry for being late with this. :-(
Attachment #152180 - Flags: first-review?(mostafah)
Comment on attachment 152180 [details] [diff] [review]
Follow-up patch

r=mostafah
Attachment #152180 - Flags: first-review?(mostafah) → first-review+
Comment on attachment 152180 [details] [diff] [review]
Follow-up patch

Second patch checked in.
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.
(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 ;-) ).
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
25% of max L value is best, give or take, half is a bit bright on most of the scale.
Michiel, Jason, what do you think?
(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.
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.
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.
Attachment #152295 - Attachment is obsolete: true
Attachment #152299 - Flags: first-review?(mvl)
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.
Attachment #152299 - Flags: first-review?(mvl) → first-review+
(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!
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
The bugspam monkeys have been set free and are feeding on Calendar :: Sunbird Only. Be afraid for your sanity!
QA Contact: gurganbl → sunbird
verified with default windows theme

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060901 Calendar/0.3a2+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.