1141620 - birthday field in AB does not keep year 2000 and throws on invalid age input

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[:aceman]

I've noticed some small problems in the birthday field in Addressbook cards, like:
1. Choosing a date in year 2000 (the default), does not fill in the year field and the card is saved without it.
2. Inputing invalid (e.g. text) value into the 'age' field clears the year, but throws an exception:
Error: ReferenceError: ageElem is not defined
Source File: chrome://messenger/content/addressbook/abCardOverlay.js
Line: 769
Sadly we can't change it to <inputbox type=number> as that would disallow keeping it empty (unless we declare a value of 0 to mean "empty"/unset).

Comment 1

[:aceman]

Attachment: patch

So much duplication between TB and SM. Hopefully bug 456024 will once unify the picker.

Comment 2

[Magnus Melin [:mkmelin]]

What date are you trying to save? A birthday in 2000 seems to get stored fine for me even without the patch.

Comment 3

[:aceman]

Attachment: patch v1.1

Try to pick Nov 28th 2000 in the birthday date picker. The year is not filled in. (Do not fill it in manually). Try a date in any other year and the year gets filled in.
Patch unbitrotted.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8693228 [details] [diff] [review]
patch v1.1

Review of attachment 8693228 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, I think the original behavior is intended, so that you "force" the user to enter a year (scrolling month by month a few decades is a lot of work, so just choose a date and the force year input).

Even though birthdays are dates then are problematic as date inputs. You don't normally want to select a given day in the past, you just want the date. So I'm not sure why we bother showing a calendar at all there... oh well.

Comment 5

[:aceman]

Yes, it was intentional,there was code to make it ignore year 2000. But why do we need it? If we didn't want to include the year, we could ignore all other years too. Now that a birthday in 2000 becomes much more common as people's contacts in TB, we should think about this. Maybe we could at least make a different year the default? E.g. the current year?

Also do not forget the patch also solves the problem 2.

Comment 6

[Magnus Melin [:mkmelin]]

I suppose the idea is that if the user actually bothered to scroll to a certain year they really meant that year.
Yeah we could make the current year default.

Comment 7

[:aceman]

Attachment: patch v1.2

OK, that would work for me. The default year must be a leap year so it chooses the nearest last leap year from now. Hopefully there is a much lower chance to have 0-4 year old contacts than 15+ year olds ;)

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8693277 [details] [diff] [review]
patch v1.2

Review of attachment 8693277 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abCommon.js
@@ +21,5 @@
>  var kDefaultAscending = "ascending";
>  var kDefaultDescending = "descending";
>  // kDefaultYear will be used in birthday calculations when no year is given;
>  // this is a leap year so that Feb 29th works.
> +const kDefaultYear = parseInt(new Date().getFullYear() / 4) * 4;

My brain's to tired to verify if this gets correct (since 2000 is a leap year), but in any case it's cryptic. 

Stackoverflow suggests 
isLeap = new Date(year, 1, 29).getMonth() == 1 

for checking if leap (and you could loop the year on from that

Comment 9

[:aceman]

Attachment: patch v1.3

OK.

Comment 10

[Ian Neal]

Comment on attachment 8697072 [details] [diff] [review]
patch v1.3

>+/**
>+ * Returns the nearest leap year below aYear.
>+ */
>+function nearestLeap(aYear) {
>+  for (let year = parseInt(aYear); year > 0; year--) {
Is parseInt() still needed?
>+    if (new Date(year, 1, 29).getMonth() == 1)
>+      return year;
>+  }
>+
>+  return 2000;
>+}
r=me with that addressed.

Comment 11

[:aceman]

Attachment: patch v1.4

Thanks. I think it is not needed as getFullYear() should return an integer (number type, not string).

Comment 12

[:aceman]

Thanks.

Comment 13

[:aceman]

https://hg.mozilla.org/comm-central/rev/55858ec8f8ff

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8697891 [details] [diff] [review]
patch v1.4

[Approval Request Comment]
Regression caused by (bug #): No regression.

Looks like this just missed the branch date. Would be nice to fix this for TB 45. Correct me if I'm wrong.

Comment 15

[Jorg K (CEST = GMT+2)]

Aceman, is it OK to put this into Aurora?

Comment 16

[:aceman]

I think so, but I can't give approval.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8697891 [details] [diff] [review]
patch v1.4

I can ;-)
Landing soon.

Comment 18

[Jorg K (CEST = GMT+2)]

Aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/c30508ebf85e