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

RESOLVED FIXED in Thunderbird 3.0a3

Status

defect
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: steffen.wilberg, Assigned: pi)

Tracking

(Depends on 1 bug)

Trunk
Thunderbird 3.0a3
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

11 years ago
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).
Flags: blocking-thunderbird3?
(Assignee)

Comment 1

11 years ago
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
(Assignee)

Updated

11 years ago
Assignee: nobody → joshgeenen+bugzilla
Status: NEW → ASSIGNED
(Reporter)

Comment 2

11 years ago
(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
(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.
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.
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b1
Whiteboard: [needs status update pi]
OS: Windows Vista → All
Hardware: PC → All
(Assignee)

Comment 5

11 years ago
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #339319 - Flags: review?(bugzilla)
(Reporter)

Comment 6

11 years ago
(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 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.
Whiteboard: [needs status update pi] → [has patch, may need update]
(Assignee)

Comment 8

11 years ago
Posted patch Patch 2 (obsolete) — Splinter Review
(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.
Attachment #339319 - Attachment is obsolete: true
Attachment #339397 - Flags: review?(bugzilla)
Attachment #339319 - Flags: review?(bugzilla)
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).
Attachment #339397 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 10

11 years ago
Posted patch Updated patch (obsolete) — Splinter Review
(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).
Attachment #339397 - Attachment is obsolete: true
Attachment #339451 - Flags: superreview?(neil)
Whiteboard: [has patch, may need update] → [has patch][one issue remaining][needs review neil]
(Assignee)

Comment 11

11 years ago
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #339451 - Attachment is obsolete: true
Attachment #339600 - Flags: superreview?(neil)
Attachment #339451 - Flags: superreview?(neil)
(Assignee)

Updated

11 years ago
Depends on: 456220
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?)
(Assignee)

Comment 13

11 years ago
Posted patch Fixed Patch (obsolete) — Splinter Review
(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.
Attachment #339600 - Attachment is obsolete: true
Attachment #339610 - Flags: superreview?(neil)
Attachment #339600 - Flags: superreview?(neil)
Comment on attachment 339610 [details] [diff] [review]
Fixed Patch

+      var date = (new Date(year, month - 1, day));
Nit: unnecessary  ^                              ^
Attachment #339610 - Flags: superreview?(neil) → superreview+
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.
Attachment #339610 - Flags: approval-seamonkey2.0a1?

Comment 16

11 years ago
Comment on attachment 339610 [details] [diff] [review]
Fixed Patch

a=me for SeaMonkey 2 Alpha 1.
Attachment #339610 - Flags: approval-seamonkey2.0a1? → approval-seamonkey2.0a1+
(Assignee)

Comment 17

11 years ago
Attachment #339610 - Attachment is obsolete: true
Comment on attachment 339657 [details] [diff] [review]
[checked in] Final patch - addressing nit

Checked in, changeset id: 392:4aec6bde0ed8
Attachment #339657 - Attachment description: Final patch - addressing nit → [checked in] Final patch - addressing nit
All the items in comment 0 have now been fixed apart from item 1 which will be handled in bug 456220.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][one issue remaining][needs review neil]
You need to log in before you can comment on or make changes to this bug.