469664 - Selected tasks (with start/due date) are unreadable on windows with black desktop

Status: VERIFIED FIXED

(Calendar :: Tasks, defect)

Description

[Andreas Treumann]

STEPS TO REPRODUCE:
===================

- use a black desktop color 
- create a future task with start date and due date
- select this task in the task list

RESULT:
=======

- the selected task is completely black (background color and characters) and unreadable

EXPECTED RESULT:
================

- the selected task should be readable


REPRODUCIBLE:
=============

- always

Comment 1

[Simon Paquet [:sipaq]]

-> me

The problem is, that we are using system background-colors here without corresponding system text colors. We just assume that the text color is always black unless defined otherwise. 

So if you combine a black text color with a black background color (as defined by the system), things get messy :)

Thanks for noticing, Andreas.

Comment 2

[Berend Cornelius]

Attachment: patch v. #1

I also found out that this is a regression to Bug 393748 -  Active tasks are dimmed (grey) in task list while completed tasks are shown in black
and prepared a patch for it. Looking at that patch I have certain doubts if similar flaws as described here in conjunctions with duetoday¨¨where the textcolor is not fitting to the background. Without having investigated any deeper I would prefer to return to the constant values that we were using before bug 393748

Comment 3

[Simon Paquet [:sipaq]]

Berend,
constant values suck in terms of accessibility and will break you even more often. Just think about what might happen with your patch, if the text color is blue in combination with your constant blue background color?

Like I said, the right thing to do is to find a suitable system foreground color that works well with the assigned system background color on all system color schemas.

Let me hook up a patch tonight and run this by you, ok?

Comment 4

[Simon Paquet [:sipaq]]

Attachment: My Patch - v1

Here's my take on this bug.

My goals here were:
- Always try to find a good corresponding system background-color to a system 
  foreground color and vice versa
- Choose an accompanying hardcoded background-color or foreground color in case 
  there is already a hardcoded color defined, which has no system color equivalent

Comment 5

[Christian Jansen]

Comment on attachment 353114 [details] [diff] [review]
My Patch - v1

Looks good for me. r=christian

Comment 6

[Berend Cornelius]

My concerns are that the constants ¨Menu¨ ¨Background¨...  refer to the current desktop background, menu-color and so on and are likely to change with the surrounding conditions like a theme change. I think that the user does not want this in this context. Once he learned that a blue background-color is associated with a ¨duetoday- task¨ a future task with another color and so on he does not want to learn new associations again after the theme changed or the desktop color...

Comment 7

[Berend Cornelius]

Comment on attachment 353114 [details] [diff] [review]
My Patch - v1

Apart from my last comment I am afraid I have to deny the review. When I tested it under Windows I got the strange result that you see in the screenshot that I will attach. But besides that the patch worked fine for all kinds of tasks.

Comment 8

[Berend Cornelius]

Attachment: screenshot of the last patch ¨MyPatch - v1¨

Comment 9

[Simon Paquet [:sipaq]]

(In reply to comment #6)
> My concerns are that the constants ¨Menu¨ ¨Background¨...  refer to the 
> current desktop background, menu-color and so on and are likely to change 
> with the surrounding conditions like a theme change.

Exactly.

> I think that the user does not want this in this context. Once he learned 
> that a blue background-color is associated with a ¨duetoday- task¨ a future 
> task with another color and so on he does not want to learn new associations 
> again after the theme changed or the desktop color...

I think this is exactly what the user wants. If he wanted all colors to stay 
the same, why would he change his color scheme in the first place?

(In reply to comment #7)
> Apart from my last comment I am afraid I have to deny the review. When I 
> tested it under Windows I got the strange result that you see in the 
> screenshot that I will attach. But besides that the patch worked fine for 
> all kinds of tasks.

It seems that the hardcoded color combination does not work. This does not 
only not work for the selected overdue task in your example, but also for a 
selected task in progress. I will attach an updated patch soon.

Comment 10

[Simon Paquet [:sipaq]]

Attachment: My Patch - v2

New patch with issues fixed that Berend raised in his review. Carrying forward UI-review.

Comment 11

[Berend Cornelius]

Andreas told me on Thursday last week that he still discovered a problem with the new patch under a certain Linux environment, but I have no closer information about this. I guess I have to wait with the review under these circumstances.

Comment 12

[Simon Paquet [:sipaq]]

(In reply to comment #11)
> Andreas told me on Thursday last week that he still discovered a problem with
> the new patch under a certain Linux environment, but I have no closer
> information about this. I guess I have to wait with the review under these
> circumstances.

Ok, CC'ing Andreas here for his input.

Comment 13

[Andreas Treumann]

Attachment: unselected task, linux

Comment 14

[Andreas Treumann]

Attachment: selected task with/without Simos patch

Comment 15

[Andreas Treumann]

The problem on linux is not caused by Simons patch.

Unselected tasks with a start date today, e.g. 2 hours in the future and one hour duration are displayed in light Grey color (characters) on a white background (see screen shot). This bug occurs also with the official nightly build.

The visibility of the same task but selected is much better with Simons patch ( see second screen shot)

Comment 16

[Berend Cornelius]

Comment on attachment 353550 [details] [diff] [review]
My Patch - v2

I am really sorry but when I tested this under Linux I saw several flaws that just can't be ignored as can bee seen in the screenshot that I will attach. Basically the problem is that the background-color is not set over the whole column but only underneath the script. With "duetoday" the script is even the  same as the background-color so that it is completely invisible.
I personally don't find that the menu-color (grey) is really suitable for the background on Linux. But if we should decide to use it we should take it for all rows and not only for some I think.

Comment 17

[Berend Cornelius]

Attachment: screenshot

Comment 18

[Simon Paquet [:sipaq]]

Berend, if you have a problem with the grey script color, then this is a new bug that should be fixed separately. As Andreas said, this was also a problem before my patch came along.

Comment 19

[Berend Cornelius]

Attachment: test-calendar

in reply to comment #18:
I could not detect the mentioned flaws of comment #16 without the patch, so I guess that we somehow have to deal with this in this issue. Attached I send an ics file in which the problems can be reproduced. Please notice that the dates of the tasks might be adapted after some time.
Andreas could not provide an ics file so I set up one on my own with five different tasks. Based on these tasks I could observe the following flaws:
-In all tasks  except "overdue" only the character's background color is set. The rest of the field remains white (see also my latest screeenshot).
-due-today tasks have grey characters on grey background and are therefor invisible.
- When selected "overdue" and "due-today" have mixed background-color, too.

Comment 20

[Simon Paquet [:sipaq]]

Attachment: Patch v3

I finally managed to work through this. I also finally realized the fine distinction between -moz-tree-row and -moz-tree-cell-text and adjusted the patch accordingly.

I'll attach a new test calendar (also containing an open-ended task) and three screenshots showing the new patch behavior.

Comment 21

[Simon Paquet [:sipaq]]

Attachment: Tasks Test Calendar v2

Comment 22

[Simon Paquet [:sipaq]]

Attachment: Screenshot - all tasks selected (with focus)

Comment 23

[Simon Paquet [:sipaq]]

Attachment: Screenshot - all tasks selected (without focus)

Comment 24

[Simon Paquet [:sipaq]]

Attachment: Screenshot - one task selected (with focus)

Comment 25

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Mac screenshot with patch - only one selected

Comment 26

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Mac screenshot with patch - all selected with focus

Comment 27

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Mac screenshot with patch - due today date entry updated to really be due today

Comment 28

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Mac screenshot with patch - all selected without focus

This is the only case when the bug still exists, when due today is actually due today and it is selected and there's no focus on lightning.

Almost there, it seems. :)

Comment 29

[Simon Paquet [:sipaq]]

Comment on attachment 356074 [details] [diff] [review]
Patch v3

Canceling review for now. I'll work on the small Mac issue that Gary found.

I would still appreciate it if someone could test this patch on Linux. So that I don't have to jump through more hoops to finally get this in.

Comment 30

[Simon Paquet [:sipaq]]

Attachment: Patch v4

So, hopefully this is the last installment of this patch.

The only difference to the v3 patch is here:

 .calendar-task-tree > treechildren::-moz-tree-cell-text(duetoday) {
-    color: ActiveCaption;
+    color: WindowText;
+    font-weight: bold;
 }

As can be seen from attachment 356185 [details], OS X uses a light grey color for ActiveCaption instead of the blue color that WinXP uses, which makes the entry unreadable when the entry is selected but without focus, as the light grey of the unfocused selection is very similar to the light grey of ActiveCaption.

So instead I switched to a simple black color. But as normal tasks and future tasks already have that text-color, I choose to make tasks that are due today more easily identifiable by making the script bold.

I'm not sure whether that decision is still covered by Christian ui-review, that he originally gave for the v1 patch. Berend, I leave it to your discretion whether I should ask for ui-review again or not.

Again, if someone with a Linux box, who uses the standard desktop theme on a GNOME or KDE desktop could test this patch and post some screenshots here, that would be very much appreciated.

Comment 31

[Andreas Treumann]

Attachment: completed tasks, with selection but no focus -> contrast maybe a little bit to low with KDE.

I checked this patch (v4) with GNOME and KDE3.5/4, default desktop theme (see screen shot), looks okay. Minor issue: completed tasks, with selection but no focus -> contrast maybe a little bit to low with KDE.

Comment 32

[Berend Cornelius]

Comment on attachment 356313 [details] [diff] [review]
Patch v4

Patch looks good and works fine from my point of view. I noticed that you only provide rules for the combination of "selected" and "focus", but nonfocused selected rows always have the same  background color. Maybe it is an option to set a certain opacity for these cases, but this should of course not block this issue and should be decided by Christian.

Comment 33

[Berend Cornelius]

Comment on attachment 356313 [details] [diff] [review]
Patch v4

I forgot to mention you could consolidate the rules where you assign the color HighlightText.

Comment 34

[Berend Cornelius]

Maybe while you are at it you could also remove all the redundant 
    list-style-image: url(chrome://calendar/skin/unifinder-images.png);
rules, too.

Comment 35

[Simon Paquet [:sipaq]]

(In reply to comment #32)
> I noticed that you only provide rules for the combination of "selected" and 
> "focus", but nonfocused selected rows always have the same  background color. 
> Maybe it is an option to set a certain opacity for these cases

(In reply to comment #33)
> I forgot to mention you could consolidate the rules where you assign the 
> color HighlightText.

(In reply to comment #34)
> Maybe while you are at it you could also remove all the redundant 
>     list-style-image: url(chrome://calendar/skin/unifinder-images.png);
> rules, too.

Berend, I tried all three of those suggestions, but to no avail. 

- Setting an opacity of 0.5 only for selected (but not focussed) tasks did 
  not work (no change visible)
- I did not manage to Consolidate the the rules where I assign the color 
  HighlightText and remove all 
    list-style-image: url(chrome://calendar/skin/unifinder-images.png)
  It had unintended side-effects, probably caused by my lack of deep CSS 
  knowledge. I'll file a followup-bug for this. Maybe you can take it as 
  you probably know much better what you're doing?

Comment 36

[Simon Paquet [:sipaq]]

Attachment: Patch v5 (as checked in)

This is what I checked in.

3 changes happened compared to the v4 patch that was reviewed:
- I added myself to the credits
- I removed the rule text-decoration: line-through for the class 
  .calendar-task-tree > treechildren::-moz-tree-row(completed, selected, focus)
  as this rule was already covered by the class
  .calendar-task-tree > treechildren::-moz-tree-cell-text(completed)
- Since the GrayText-color doesn't work equally on all platforms and is hardly 
  visible when selected but not focussed even on the Mac or Windows platform, 
  I decided to give it a black text-color as well, but show it in an italic 
  font-style.

Comment 37

[Simon Paquet [:sipaq]]

Changeset ce533e4625e8 pushed to comm-central

Comment 38

[Andreas Treumann]

Checked in lightning and sunbird build 20090113 -> verified.

Comment 39

[Philipp Preißer-Kewisch [:Fallen][☁️📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".