306241 - calendarManagement not looking for contrasting color when null

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Robin Edrenius]

calendarManagement is not looking for contrasting textcolor if 'null'

Comment 1

[Robin Edrenius]

Attachment: Patch v1

This should fix the issue.

Comment 2

[Joey Minta]

Comment on attachment 194097 [details] [diff] [review]
Patch v1

+	 if (color || 'null')

The original point of this 'if' was to take out exactly the 'null' case.  If
you want it to work anyway, you should remove the 'if' entirely.

I thought I put the 'if' in originally because getContrastingTextColor choked
on 'null', but that seems to be working OK for me now.	You might want to
double-check this though.

Comment 3

[Robin Edrenius]

Attachment: patch v2

Another attempt.
I see no choke on 'null'.

Comment 4

[Joey Minta]

Comment on attachment 194100 [details] [diff] [review]
patch v2

r=jminta

Comment 5

[Joey Minta]

Patch checked in.

Comment 6

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

Are you sure getContrastingTextColor doesn't die on null? It might not die on
'null', but that's something different. That's a string. getContrastingTextColor
does bgColor.replace(), and that dies if bgColor is null.
(Also, again, how can a calendar have no color? The boxes always have some color)

Comment 7

[Joey Minta]

(In reply to comment #6)
> Are you sure getContrastingTextColor doesn't die on null? It might not die on
> 'null', but that's something different. That's a string. getContrastingTextColor
> does bgColor.replace(), and that dies if bgColor is null.

I tested this by creating calendar with no color.  This should set the pref to
null (I think) and everything worked fine there.  In short, I found no way for a
user to perform and action that kills getContrastingTextColor.  If you can find
one, it's easy enough to fix with "if (!bgColor) return 'black';"

> (Also, again, how can a calendar have no color? The boxes always have some color)
The point is (especially if a different theme is being used), that
style.backgroundColor shouldn't be required to be set.  Setting white seems like
they have no color, but 'transparent'/'none' should be an option, which right
now it isn't.

Comment 8

[Joey Minta]

OK, I did manage to finally kill getContrastingTextColor this morning.

Status:Re-open

Comment 9

[Joey Minta]

Attachment: handle null

sigh...I knew there had to be a reason that 'if' was there.

Comment 10

[Joey Minta]

Comment on attachment 194186 [details] [diff] [review]
handle null

bad idea

Comment 11

[Joey Minta]

(Apologies for all this bugspam.)

I only made getContrastingTextColor die this morning using my own custom
colorpicker.  Normal colorpickers assign "" if no color is chosen, which can be
handled.  Mine assigned null, which died.  So, this bug is actually fixed.

Comment 12

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

Comment on attachment 194100 [details] [diff] [review]
patch v2

getCalendarPref can return null, so getContrastingTextColor can get called with
null.
Please back this patch out. It is wrong.