Closed Bug 248472 Opened 21 years ago Closed 21 years ago

Use white text for dark calendar colors for better readability

Categories

(Calendar :: Sunbird Only, defect)

defect
Not set
normal

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: 21 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: 21 years ago21 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.

Attachment

General

Created:
Updated:
Size: