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

RESOLVED FIXED in Thunderbird 46.0

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 46.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird45 fixed, thunderbird46 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

4 years ago
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).
Assignee

Comment 1

4 years ago
Posted patch patch (obsolete) — Splinter Review
So much duplication between TB and SM. Hopefully bug 456024 will once unify the picker.
Attachment #8575637 - Flags: review?(neil)
Attachment #8575637 - Flags: review?(mconley)
Assignee

Updated

4 years ago
Attachment #8575637 - Flags: review?(mkmelin+mozilla)
What date are you trying to save? A birthday in 2000 seems to get stored fine for me even without the patch.
Assignee

Comment 3

4 years ago
Posted patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #8575637 - Attachment is obsolete: true
Attachment #8575637 - Flags: review?(neil)
Attachment #8575637 - Flags: review?(mkmelin+mozilla)
Attachment #8575637 - Flags: review?(mconley)
Attachment #8693228 - Flags: review?(mkmelin+mozilla)
Attachment #8693228 - Flags: review?(iann_bugzilla)
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.
Attachment #8693228 - Flags: review?(mkmelin+mozilla) → review-
Assignee

Comment 5

4 years ago
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.
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.
Assignee

Comment 7

4 years ago
Posted patch patch v1.2 (obsolete) — Splinter Review
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 ;)
Attachment #8693228 - Attachment is obsolete: true
Attachment #8693228 - Flags: review?(iann_bugzilla)
Attachment #8693277 - Flags: review?(mkmelin+mozilla)
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
Assignee

Comment 9

4 years ago
Posted patch patch v1.3 (obsolete) — Splinter Review
OK.
Attachment #8693277 - Attachment is obsolete: true
Attachment #8693277 - Flags: review?(mkmelin+mozilla)
Attachment #8697072 - Flags: review?(mkmelin+mozilla)
Attachment #8697072 - Flags: review?(iann_bugzilla)
Assignee

Updated

4 years ago
Status: NEW → ASSIGNED

Comment 10

4 years ago
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.
Attachment #8697072 - Flags: review?(iann_bugzilla) → review+
Assignee

Comment 11

4 years ago
Posted patch patch v1.4Splinter Review
Thanks. I think it is not needed as getFullYear() should return an integer (number type, not string).
Attachment #8697072 - Attachment is obsolete: true
Attachment #8697072 - Flags: review?(mkmelin+mozilla)
Attachment #8697891 - Flags: review?(mkmelin+mozilla)
Attachment #8697891 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 12

4 years ago
Thanks.
Keywords: checkin-needed
Assignee

Comment 13

4 years ago
https://hg.mozilla.org/comm-central/rev/55858ec8f8ff
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0

Comment 14

4 years ago
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.
Attachment #8697891 - Flags: approval-comm-aurora?

Comment 15

4 years ago
Aceman, is it OK to put this into Aurora?
Flags: needinfo?(acelists)
Assignee

Comment 16

4 years ago
I think so, but I can't give approval.
Flags: needinfo?(acelists)

Comment 17

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

I can ;-)
Landing soon.
Attachment #8697891 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.