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)
Calendar
Sunbird Only
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: Stefan.Borggraefe, Assigned: Stefan.Borggraefe)
Details
Attachments
(4 files, 1 obsolete file)
2.98 KB,
patch
|
mostafah
:
first-review+
|
Details | Diff | Splinter Review |
5.07 KB,
patch
|
mostafah
:
first-review+
|
Details | Diff | Splinter Review |
2.09 KB,
image/png
|
Details | |
4.49 KB,
patch
|
mvl
:
first-review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•21 years ago
|
||
Assignee: mostafah → Stefan.Borggraefe
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•21 years ago
|
||
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)
Comment 3•21 years ago
|
||
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.
Assignee | ||
Comment 4•21 years ago
|
||
(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•21 years ago
|
||
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+
Assignee | ||
Comment 6•21 years ago
|
||
(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•21 years ago
|
||
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.
Assignee | ||
Comment 8•21 years ago
|
||
(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
Assignee | ||
Comment 9•21 years ago
|
||
I realised only now that Day View needs some special treatment, because it set its text-colors explicitly. Sorry for being late with this. :-(
Assignee | ||
Updated•21 years ago
|
Attachment #152180 -
Flags: first-review?(mostafah)
Comment 10•21 years ago
|
||
Comment on attachment 152180 [details] [diff] [review] Follow-up patch r=mostafah
Attachment #152180 -
Flags: first-review?(mostafah) → first-review+
Assignee | ||
Comment 11•21 years ago
|
||
Comment on attachment 152180 [details] [diff] [review] Follow-up patch Second patch checked in.
Comment 12•21 years ago
|
||
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.
Assignee | ||
Comment 13•21 years ago
|
||
(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•21 years ago
|
||
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 → ---
Assignee | ||
Comment 15•21 years ago
|
||
(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•21 years ago
|
||
25% of max L value is best, give or take, half is a bit bright on most of the scale.
Assignee | ||
Comment 17•21 years ago
|
||
Michiel, Jason, what do you think?
Assignee | ||
Comment 18•21 years ago
|
||
(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•21 years ago
|
||
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.
Assignee | ||
Comment 20•21 years ago
|
||
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
Assignee | ||
Updated•21 years ago
|
Attachment #152299 -
Flags: first-review?(mvl)
Comment 21•21 years ago
|
||
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+
Assignee | ||
Comment 22•21 years ago
|
||
(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 ago → 21 years ago
Resolution: --- → FIXED
Comment 23•18 years ago
|
||
The bugspam monkeys have been set free and are feeding on Calendar :: Sunbird Only. Be afraid for your sanity!
QA Contact: gurganbl → sunbird
Comment 24•18 years ago
|
||
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.
Description
•