455797 - birthday picker doesn't let me enter certain dates, resets day and year

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Steffen Wilberg]

A few tests with the new birthday datepicker, using a German system, which has the DD.MM.YYYY date format:

Bug 1:
1. Enter 20.01. and press OK.
Result: In the details pane of "Address Book" window, it displays:
Sat Jan 20 1900 00:00:00 GMT +0100
Expected: Montag, 20. Januar (localized format without year, time, or timezone)

The error console displays:
Error: aAddressData is undefined
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 1021
This error is being logged every time I leave the card by pressing OK.

2. Enter 20.01.1975:
Display in the details pane: Montag, 20. Januar 1975
--> ok


Bug 2:
1. Go to the same card, which still has 20.01.1975, leave the day as is.
2. Tab to the month and modify it by entering a different value or use the up or down arrow keys:
Result: The day jumps to the current day (18 right now).


Bug 3:
1. Enter 31.07.1975:
Result: Display in the details pane: Donnerstag, 31. Juli 1975

2. Enter 01.08.1975 (Aug 1, 1975):
Result: Display in the details pane: Sonntag, 1. Dezember 1974 (!)

3. Open the card again: it displays the current day and month (Sep 17)
JS Console displays: "Error: uncaught exception: Invalid Month"

Bug 4:
1. Enter 30.06.
2. Tab away and back to the day field, and enter 31
3. Press tab
Result: 01.06.
Expected: June 31 doesn't exist, so switch to the next day (July 1) when I leave the datepicker, but don't do anything until I have left the day and month field since I might be going to enter a valid date like 31.07. (July 31).

Comment 1

[Josh Geenen (:pi)]

I don't have much time until tomorrow and Friday, but here are my notes so far...

(In reply to comment #0)
> A few tests with the new birthday datepicker, using a German system, which has
> the DD.MM.YYYY date format:
> 
> Bug 1:
> 1. Enter 20.01. and press OK.
> Result: In the details pane of "Address Book" window, it displays:
> Sat Jan 20 1900 00:00:00 GMT +0100
> Expected: Montag, 20. Januar (localized format without year, time, or timezone)

This doesn't happen for me, but I'll try to duplicate it more.  If you go in the error console and evaluate this text:

alert(new Date().toLocaleDateString());

what is in the dialog that pops up?

> The error console displays:
> Error: aAddressData is undefined
> Source File: chrome://messenger/content/msgHdrViewOverlay.js
> Line: 1021
> This error is being logged every time I leave the card by pressing OK.

I didn't look into this error yet, but I was able to reproduce it.  I think that error is from a patch for Bug 450724 [1] and may or may not have to do with the birthday patch.

> Bug 2:
> 1. Go to the same card, which still has 20.01.1975, leave the day as is.
> 2. Tab to the month and modify it by entering a different value or use the up
> or down arrow keys:
> Result: The day jumps to the current day (18 right now).

I can duplicate this and will check it out soon.

> Bug 3:
> 1. Enter 31.07.1975:
> Result: Display in the details pane: Donnerstag, 31. Juli 1975
> 
> 2. Enter 01.08.1975 (Aug 1, 1975):
> Result: Display in the details pane: Sonntag, 1. Dezember 1974 (!)
> 
> 3. Open the card again: it displays the current day and month (Sep 17)
> JS Console displays: "Error: uncaught exception: Invalid Month"

Good catch, I accidentally call parseInt without specifying base 10, and parseInt("08") evaluates to 0, so it tries to set the month as -1.  The datepicker then throws Invalid Month.  In my fix I'll change the parseInt(month) to parseInt(month, 10)

> Bug 4:
> 1. Enter 30.06.
> 2. Tab away and back to the day field, and enter 31
> 3. Press tab
> Result: 01.06.
> Expected: June 31 doesn't exist, so switch to the next day (July 1) when I
> leave the datepicker, but don't do anything until I have left the day and month
> field since I might be going to enter a valid date like 31.07. (July 31).
This involves the datepicker widget, not the birthday patch.  You can see why it does this here:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/datetimepicker.xml#503

[1] https://bugzilla.mozilla.org/attachment.cgi?id=336332

Comment 2

[Steffen Wilberg]

(In reply to comment #1)
> This doesn't happen for me, but I'll try to duplicate it more.  If you go in
> the error console and evaluate this text:
> 
> alert(new Date().toLocaleDateString());
> 
> what is in the dialog that pops up?
Donnerstag, 18. September 2008

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #1)
> > The error console displays:
> > Error: aAddressData is undefined
> > Source File: chrome://messenger/content/msgHdrViewOverlay.js
> > Line: 1021
> > This error is being logged every time I leave the card by pressing OK.
> 
> I didn't look into this error yet, but I was able to reproduce it.  I think
> that error is from a patch for Bug 450724 [1] and may or may not have to do
> with the birthday patch.

This error is indeed due to that bug. I checked in a fix yesterday that should have fixed it.

Comment 4

[Mark Banner (:standard8)]

Setting blocking beta 1 for now (we'll re-evaluate as we go). As this is a new feature, we should try and have it as tidy as possible for beta.

Comment 5

[Josh Geenen (:pi)]

Attachment: Patch

mailnews.js wasn't modified in the repository for some reason with the last patch, so the attribute map changes are in this patch as well.

(In reply to comment #0)
> Bug 1:
> 1. Enter 20.01. and press OK.
> Result: In the details pane of "Address Book" window, it displays:
> Sat Jan 20 1900 00:00:00 GMT +0100
> Expected: Montag, 20. Januar (localized format without year, time, or timezone)

It definitely shouldn't show that.  That output is from calling the toString method of a date object.  This isn't addressed in this patch yet because I do not know why it is happening yet.

> Bug 2:
> 1. Go to the same card, which still has 20.01.1975, leave the day as is.
> 2. Tab to the month and modify it by entering a different value or use the up
> or down arrow keys:
> Result: The day jumps to the current day (18 right now).

This should be fixed with this patch - abCardOverlay.js wasn't setting the date (day) property of the datepicker in GetCardValues (it did set month and year).

What comes up if you run this code in the error console:

alert(new Date().toLocaleFormat("%B %e"));

As a future note, it is probably better to generate this manually based on the system settings instead of the locale in addressbook.Properties

> Bug 3:
> 1. Enter 31.07.1975:
> Result: Display in the details pane: Donnerstag, 31. Juli 1975
> 
> 2. Enter 01.08.1975 (Aug 1, 1975):
> Result: Display in the details pane: Sonntag, 1. Dezember 1974 (!)
> 
> 3. Open the card again: it displays the current day and month (Sep 17)
> JS Console displays: "Error: uncaught exception: Invalid Month"

This should also be fixed with this patch - the parseInt(month); was changed to parseInt(month, 10); specifying base 10.

Comment 6

[Steffen Wilberg]

(In reply to comment #5)
> What comes up if you run this code in the error console:
> 
> alert(new Date().toLocaleFormat("%B %e"));
Thu Sep 18 2008 22:40:05 GMT +0200
(using an en-US nightly on a German Windows)

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 339319 [details] [diff] [review]
Patch

   // get the month of the year (1 - 12)
   var month = cardproperty.getProperty("BirthMonth", null);
   // set the datepicker's month and prepend a zero if necessary
   if (month) {
-    birthday.month = parseInt(month) - 1;
+    birthday.month = parseInt(month, 10) - 1;
     if (month.length < 2)
       month = "0" + month;
   }
   birthday.monthField.value = month;
 
   // get the date of the month (1 - 31)
   var date = cardproperty.getProperty("BirthDay", null);
+  if (date)
+    birthday.date = date;
   birthday.dateField.value = date;

Now I look at this again, I don't understand why you're setting both the date (or month etc) and the dateField (or monthField etc).

From looking at the datepicker xml, it seems like just setting birthday.date will set birthday.dateField internally as well.

Comment 8

[Josh Geenen (:pi)]

Attachment: Patch 2

(In reply to comment #7)
> Now I look at this again, I don't understand why you're setting both the date
> (or month etc) and the dateField (or monthField etc).
> 
> From looking at the datepicker xml, it seems like just setting birthday.date
> will set birthday.dateField internally as well.

I think originally I just set the fields and added the month later, but you are right and it should change.  I removed that part and added an else statement to set the field to null if there is not a valid birthdate or birthmonth stored.

I'll try to catch you on IRC tomorrow about the toLocaleFormat problem.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 339397 [details] [diff] [review]
Patch 2

 REQUIRES	= xpcom \
+      mailnews \
 		  string \
 		  necko \
 		  msgbase \
 		  pref \
 		  uconv \
 		  $(NULL)

r=me if you drop this change (me got rid of that include directory).

Comment 10

[Josh Geenen (:pi)]

Attachment: Updated patch

(In reply to comment #9)
> (From update of attachment 339397 [details] [diff] [review])
>  REQUIRES    = xpcom \
> +      mailnews \
>            string \
>            necko \
>            msgbase \
>            pref \
>            uconv \
>            $(NULL)
> 
> r=me if you drop this change (me got rid of that include directory).

Removed, sorry about that (a local change).

Comment 11

[Josh Geenen (:pi)]

Attachment: Patch

Birthdays weren't removed permanently despite appearing to do so.  Reopening the address book would show the 'deleted' information because deleteProperty doesn't work for those fields.  Now setProperty is used and the value of the property is null if the info was deleted in the dialog.

This also updates the LDIF unit test.

Comment 12

[[e-mail]]

Comment on attachment 339600 [details] [diff] [review]
Patch

>   // get the month of the year (1 - 12)
>   var month = cardproperty.getProperty("BirthMonth", null);
>+  if (month && month > 0 && month < 13)
Is it safe to compare month < 13 before you call parseInt?
Also, month > 0 implies month, so you only need two checks.

>+    birthday.month = parseInt(month, 10) - 1;
I suppose you can't set birthday.monthField.value?

>   var date = cardproperty.getProperty("BirthDay", null);
>+  if (date && date > 0 && date < 32)
>+    birthday.date = date;
(similarly here)

>-  birthday.year = year ? year : kDefaultYear;
>+  birthday.year = year && year < 10000 && year > 0 ? year : kDefaultYear;
>   birthYear.value = year;
Why do you need to set both? (Or is birthYear not part of birthday?)

Comment 13

[Josh Geenen (:pi)]

Attachment: Fixed Patch

(In reply to comment #12)
> Is it safe to compare month < 13 before you call parseInt?
> Also, month > 0 implies month, so you only need two checks.
Fixed, and the parseInt isn't necessary, so it was removed.  Both month < 0 and  month < 13 return false if month is NaN.

> >+    birthday.month = parseInt(month, 10) - 1;
> I suppose you can't set birthday.monthField.value?
No, but the parseInt isn't necessary because subtracting one will convert month into an integer, if possible.  If month is NaN it won't get to this part of code.

> (similarly here)
Fixed again.

> Why do you need to set both? (Or is birthYear not part of birthday?)
birthYear is a separate element.

Comment 14

[[e-mail]]

Comment on attachment 339610 [details] [diff] [review]
Fixed Patch

+      var date = (new Date(year, month - 1, day));
Nit: unnecessary  ^                              ^

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 339610 [details] [diff] [review]
Fixed Patch

Requesting SeaMonkey alpha 1 approval. I think this is a useful patch that will make the birthday datepicker a bit more logical and fix various bugs, which folk are especially likely to notice because its new and they will try it out; also fixes LDAP to work with the new birthday field.

Comment 16

[Robert Kaiser]

Comment on attachment 339610 [details] [diff] [review]
Fixed Patch

a=me for SeaMonkey 2 Alpha 1.

Comment 17

[Josh Geenen (:pi)]

Attachment: [checked in] Final patch - addressing nit

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 339657 [details] [diff] [review]
[checked in] Final patch - addressing nit

Checked in, changeset id: 392:4aec6bde0ed8

Comment 19

[Mark Banner (:standard8)]

All the items in comment 0 have now been fixed apart from item 1 which will be handled in bug 456220.