Turn the adapted birthday datepicker into an inherited/extended xbl widget.

ASSIGNED
Assigned to

Status

MailNews Core
Address Book
ASSIGNED
9 years ago
2 years ago

People

(Reporter: standard8, Assigned: pi)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 16 obsolete attachments)

40.02 KB, image/png
clarkbw
: ui-review+
Details
51.29 KB, patch
Ian Neal
: review-
Details | Diff | Splinter Review
6.34 KB, text/plain
Details
(Reporter)

Description

9 years ago
In bug 13595 we did some customisations to the toolkit datepicker widget. We should incorporate this into our own custom widget (possibly in addrbookWidgets.xml) so we can reuse it easily for things like anniversaries.
(Reporter)

Updated

9 years ago
Blocks: 456025

Comment 1

9 years ago
AS I commented in Bug 456025 pertaining to Anniversaries, this is a nice to have widget tool. Business might like this to aid tracking account longevity for loyalty promotions.
(Assignee)

Comment 2

9 years ago
I'm working on this now and over the next few days, so I'll assign it to myself.
Assignee: nobody → joshgeenen+bugzilla
Status: NEW → ASSIGNED
(Assignee)

Comment 3

9 years ago
Created attachment 341854 [details] [diff] [review]
Work in progress

Adds a binding in addrbook/resources/content/addrbookWidgets.xml that extends the popup datepicker then modifies it to collapse the year field/separator and accept blank values.

Modifies messenger.css to add it as a datepicker type="noyear".

Comment 4

9 years ago
(In reply to comment #3)
> Created an attachment (id=341854) [details]
> Work in progress
> 
> Adds a binding in addrbook/resources/content/addrbookWidgets.xml that extends
> the popup datepicker then modifies it to collapse the year field/separator and
> accept blank values.
> 
> Modifies messenger.css to add it as a datepicker type="noyear".

How many options does this present to a programmer developing an extension?
I.E. does this offer:
1. Day/Month only
2. Year only
3. Longevity only
4. Multi-fields in combination (perhaps using the numerical combining convention: 1+2+3=7)
(Assignee)

Comment 5

9 years ago
(In reply to comment #4)
> How many options does this present to a programmer developing an extension?
> I.E. does this offer:
> 1. Day/Month only
> 2. Year only
> 3. Longevity only
> 4. Multi-fields in combination (perhaps using the numerical combining
> convention: 1+2+3=7)

Good question.  The datepicker in that patch is only composed of the Day and Month portion of a normal datepicker.  The year and age elements of the birthday "field" are separate textboxes.

Comment 6

9 years ago
(In reply to comment #5)
OK, shows I did not correctly understand the date picker. I got the concept for the question from how the protocol logger gets configured. By adding together the digits assigned to logging levels, the complexity can be built up from simple to complex. It looks to simplify parameter passing with just a single value.
(Assignee)

Comment 7

9 years ago
Created attachment 343856 [details]
A modified datepicker widget with empty text

How does this look with empty text?  Only one letter is able to fit.  Empty text requires adding several event listeners, but it should work since I copied much of it from the textbox widget.
(Assignee)

Comment 8

9 years ago
Created attachment 353983 [details]
Work in progress 2

My newest work in progress that works for me in Thunderbird and Seamonkey in Linux.

The emptytext for the month and day are properties of the datepicker and are undefined by default.  In this patch they are set in abCardOverlay.xul based on the localized values in abCardOverlay.dtd.  I am not sure if this is the correct way to localize.
Attachment #341854 - Attachment is obsolete: true
(Assignee)

Comment 9

9 years ago
Created attachment 354225 [details]
Screenshot - Work in progress 2
Attachment #343856 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #353983 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 10

9 years ago
The screenshot definitely looks good, I'll try and have a look at the patch in the next couple of days.

Comment 11

8 years ago
Created attachment 372914 [details]
always october

I don't know if this is the appropriate place or not, but this attachment shows how the day/month box is auto-defined to "Oct" for me.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shredder/3.0b3pre

Comment 12

8 years ago
Created attachment 372915 [details]
always october 2

And here's after picking a date.

Comment 13

8 years ago
Specifically, this happens if you have Win XP set to use the following date format:

dd MMM yy which would display as 15 Apr 09 for today's date.

When I switched back to MM/dd/yyyy (04/15/2009) and restarted Thunderbird, the day/month box auto-fills with just "/".
(Reporter)

Comment 14

8 years ago
(In reply to comment #11)
> I don't know if this is the appropriate place or not, but this attachment shows
> how the day/month box is auto-defined to "Oct" for me.

Please file a separate bug - changing the current datepicker into a widget and problems with dates are two separate issues.
(Assignee)

Comment 15

8 years ago
(In reply to comment #14)
> (In reply to comment #11)
> > I don't know if this is the appropriate place or not, but this attachment shows
> > how the day/month box is auto-defined to "Oct" for me.
> 
> Please file a separate bug - changing the current datepicker into a widget and
> problems with dates are two separate issues.

I reproduced the bug in Vista with and without the new widget and filed Bug 488627.

I can provide an updated patch, although I've had clean merges since the last patch IIRC.
(Assignee)

Comment 16

8 years ago
Created attachment 373588 [details] [diff] [review]
Work in progress 3

This patch was taken against the current trunk with very few, if any, changes from the previous patch.  It behaves like the current datepicker on trunk builds, but adds localized emptytext for the date and month fields and uses a new XBL widget that extends, rather than modifies, the popup datepicker.

Works with or without the patch to Bug 488627, which lets datepickers work with more short date formats.
Attachment #353983 - Attachment is obsolete: true
(Assignee)

Comment 17

8 years ago
Created attachment 373832 [details]
Updated screenshot

This is a screenshot of the most recent patch.

After some discussion in IRC I am going to put my fix for Bug 488627 into this patch until that bug gets taken care of, but the UI won't change.  Until then this datepicker won't work with short date formats that contain anything other than a numeric date, month, and year.

The UI changed from a popup datepicker modified through JS after the datepicker was added to the new/edit card dialog to an extended popup datepicker with the same modifications.  I added localized emptytext, which is worth testing.
Attachment #354225 - Attachment is obsolete: true
Attachment #373832 - Flags: ui-review?(clarkbw)
Comment on attachment 373832 [details]
Updated screenshot

looks good from the screenshot!
Attachment #373832 - Flags: ui-review?(clarkbw) → ui-review+
(Assignee)

Comment 19

8 years ago
Created attachment 374047 [details] [diff] [review]
Work in progress 4

This patch contains a fix for Bug 488627 and detects the separators.  All fields are still numeric regardless of their values in the short date format.

I temporarily added a textbox below the datepicker that lets you type a custom month format and see the resulting datepicker for testing purposes.  The format must be valid when used with toLocaleFormat [0] and contain a date, month, and year (ie %d/%m/%y).  Leaving the textbox blank results in %x, which is the system's short date format.  I have a list of most of the unique d_fmt variables in /usr/share/i18n/locales in Bug 488627 comment 6.  I forgot to mention there that certain values, such as %e do not work in Windows, and the datepicker will throw an exception for those that do not include a year, such as %A %d %b.  How should those be handled?  I put the exception there for testing purposes.

This can still be broken if one field is present more than once in the date format if in a certain order (%d, %a/%m/%y which doesn't make much sense).

I finished this very late last night so I'm going to test it after work today before removing that temporary field and requesting r.

[0] https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Global_Objects/Date/ToLocaleFormat
Attachment #373588 - Attachment is obsolete: true
(Assignee)

Comment 20

8 years ago
Created attachment 374206 [details] [diff] [review]
Patch v1

I compiled and tested this patch with a trunk build of Shredder (Gentoo and Vista) and Seamonkey (Gentoo).  It has a few fixes since the last patch.
I added %e into the date options even thought it doesn't work in Windows and there is a later check for one-digit months because %e is padded with a space which can end up in a separator otherwise.

Here are the date formats I tested (all seemed to work)

Linux:
%A %d %B %Y
%A %d %b %Y
%A %d,%B,%Y
%A, %B %d, %Y
%A, %d %B, %Y
%B %d %A %Y
%Y-%b-%d
%Y-%m-%d
%Y.%m.%d
%d %b %Y
%d %b, %Y
%d-%m-%Y
%d-%m-%y
%d. %b %Y
%d. %b %Y
%d. %m. %Y
%d. %m. %y
%d.%m.%Y
%d.%m.%y
%d/%m-%Y
%d/%m/%Y
%d/%m/%y
%m/%d/%Y
%m/%d/%y
%e-%m-%Y (fixed in this patch)
%m-%e %Y (fixed in this patch)
%a %e.%b %Y (fixed in this patch)

Windows Vista: (Regional and Language Settings)
M/d/yyyy
MM/dd/yy
yy/MM/dd
dd-MMM-yy
d MMMM yy
d/M/yy
Attachment #372914 - Attachment is obsolete: true
Attachment #372915 - Attachment is obsolete: true
Attachment #374047 - Attachment is obsolete: true
Attachment #374206 - Flags: review?(bugzilla)
(Assignee)

Comment 21

8 years ago
Created attachment 374207 [details] [diff] [review]
Patch v1 with an easy way to change formats

This is the same thing as patch v1 but adds a textbox to abCardOverlay.xul that lets you change the date format that it uses for testing purposes.
(Reporter)

Comment 22

8 years ago
Comment on attachment 374206 [details] [diff] [review]
Patch v1

I've not tested this yet, here are some initial comments though.

>+      // if the year doesn't exist, just display the month name and day
>+      if (!year) {
>+        var dt = new Date(2008, 0, 4);
>+        var string = dt.toLocaleDateString();
>+        // find the location of the month and date in the string
>+        var monthIndex = string.indexOf("1");
>+        var dateIndex = string.indexOf("4");
>+        var monthStr = date.toLocaleFormat("%B");
>+        // form the birthday string by placing the day before or after the
>+        // localized month name
>+        if (monthIndex < dateIndex)
>+          dateStr = monthStr + " " + day;
>+        else
>+          dateStr = day + " " + month;

This won't work with right-to-left locales.

>+<!-- LOCALIZATION NOTE: Please make sure the the month and day empty text
>+     values fit in the birthday field (in the new/edit contact dialog) -->
>+<!ENTITY Month.emptytext                "M">
>+<!ENTITY Day.emptytext                  "D">

How about providing an extra (localised) width attribute so that locales can adjust the size of the month and day fields?

>           <label control="Birthday" value="&Birthday.label;"
>                  accesskey="&Birthday.accesskey;"/>
>           <hbox class="AddressCardEditWidth" align="center">
>-            <!-- NOTE: This datepicker is modified.
>-                 See abCardOverlay.js for details-->
>-            <datepicker id="Birthday" type="popup"/>
>+            <datepicker id="Birthday" type="noyear"
>+                        accesskey="&Birthday.accesskey;"
>+                        monthtext="&Month.emptytext;"
>+                        daytext="&Day.emptytext;"/>

Doesn't the accesskey attribute on the label automatically hook up to the birthday field?

>           <label control="JobTitle" value="&JobTitle.label;"
>-                 accesskey="&JobTitle.accesskey;"/>
>+                 ccesskey="&JobTitle.accesskey;"/>

We don't want this change.

>+  <binding id="datepicker-noyear"
>+           extends="chrome://global/content/bindings/datetimepicker.xml#datepicker-popup">
...
>+      <method name="_init">
...
>+            var format       = this.getAttribute("dateformat");
>+            if (!format) format = "%x";

nit: prefer this on two lines.

>+            this._separatorFirst.value  =
>+              dateStr.substr(sep1Start, sep1End - sep1Start);
>+            this._separatorSecond.value =
>+              dateStr.substr(sep2Start, sep2End - sep2Start);
>+              

nit: unnecessary whitespace on blank line.
(Assignee)

Comment 23

8 years ago
Created attachment 376172 [details] [diff] [review]
Patch v1.1

(In reply to comment #22)
> >+      // if the year doesn't exist, just display the month name and day
> >+      if (!year) {
> >+        var dt = new Date(2008, 0, 4);
> >+        var string = dt.toLocaleDateString();
> >+        // find the location of the month and date in the string
> >+        var monthIndex = string.indexOf("1");
> >+        var dateIndex = string.indexOf("4");
> >+        var monthStr = date.toLocaleFormat("%B");
> >+        // form the birthday string by placing the day before or after the
> >+        // localized month name
> >+        if (monthIndex < dateIndex)
> >+          dateStr = monthStr + " " + day;
> >+        else
> >+          dateStr = day + " " + month;
> 
> This won't work with right-to-left locales.

Good point, but I'm not sure how to fix it.  Is there a good place to look for how to do this?

This attachment fixes all the other issues.  I have a patch with the textbox to change the format available.
Attachment #374206 - Attachment is obsolete: true
Attachment #376172 - Flags: review?(bugzilla)
Attachment #374206 - Flags: review?(bugzilla)
(Reporter)

Comment 24

8 years ago
(In reply to comment #23)
> Created an attachment (id=376172) [details]
> Patch v1.1
> 
> (In reply to comment #22)
> > >+      // if the year doesn't exist, just display the month name and day
> > >+      if (!year) {
> > >+        var dt = new Date(2008, 0, 4);
> > >+        var string = dt.toLocaleDateString();
> > >+        // find the location of the month and date in the string
> > >+        var monthIndex = string.indexOf("1");
> > >+        var dateIndex = string.indexOf("4");
> > >+        var monthStr = date.toLocaleFormat("%B");
> > >+        // form the birthday string by placing the day before or after the
> > >+        // localized month name
> > >+        if (monthIndex < dateIndex)
> > >+          dateStr = monthStr + " " + day;
> > >+        else
> > >+          dateStr = day + " " + month;
> > 
> > This won't work with right-to-left locales.
> 
> Good point, but I'm not sure how to fix it.  Is there a good place to look for
> how to do this?

Given its effectively a combined string, I would suggest putting it in the locale files then locales can decide what format they want.
(Assignee)

Comment 25

8 years ago
Created attachment 376372 [details] [diff] [review]
Patch v1.2

(In reply to comment #24)
> Given its effectively a combined string, I would suggest putting it in the
> locale files then locales can decide what format they want.

Done in this patch.  I changed %e to %d for Windows, but haven't had time to compile and test it on my Windows machine yet.  The original reason I switched from doing that was to let that reflect the current system settings and not be forced into the localized format.
Attachment #376172 - Attachment is obsolete: true
Attachment #376372 - Flags: review?(bugzilla)
Attachment #376172 - Flags: review?(bugzilla)
(Reporter)

Comment 26

8 years ago
(In reply to comment #25)
> Done in this patch.  I changed %e to %d for Windows, but haven't had time to
> compile and test it on my Windows machine yet.  The original reason I switched
> from doing that was to let that reflect the current system settings and not be
> forced into the localized format.

Hmm, now I think about this again, I think you may have actually had it right, because the system format is going to determine that order. I think maybe v1.1 is the right version to go with.
(Reporter)

Comment 27

8 years ago
Comment on attachment 376172 [details] [diff] [review]
Patch v1.1

>+      // get the full date as the default string (system and locale-dependent)
>+      dateStr = date.toLocaleDateString();
>+      // if the year doesn't exist, just display the month name and day
>+      if (!year) {
>+        var dt = new Date(2008, 0, 4);
>+        var string = dt.toLocaleDateString();
>+        // find the location of the month and date in the string
>+        var monthIndex = string.indexOf("1");
>+        var dateIndex = string.indexOf("4");
>+        var monthStr = date.toLocaleFormat("%B");
>+        // form the birthday string by placing the day before or after the
>+        // localized month name
>+        if (monthIndex < dateIndex)
>+          dateStr = monthStr + " " + day;
>+        else
>+          dateStr = day + " " + month;

I think it may be worth thinking about finding the splitter as well. For instance, I just get "09 06" with my default settings.

>+            if (dayWidth) {
>+              this.dateField.parentNode.style.width = dayWidth;
>+            }
>+            if (monthWidth) {
>+              this.monthField.parentNode.style.width = monthWidth;
>+            }

nit: no braces necessary on single line ifs.
(Reporter)

Comment 28

8 years ago
Josh, can you update with v1.1 and think about the splitter as well.

Adding dev-doc-needed as if we take this patch we'll want to document our specialism of the datepicker.
Keywords: dev-doc-needed
(Reporter)

Updated

8 years ago
Attachment #376372 - Flags: review?(bugzilla)
(Assignee)

Comment 29

8 years ago
(In reply to comment #28)
> Josh, can you update with v1.1 and think about the splitter as well.

Sure, but I'm not sure how to best find the separator.  Ideally it would look the same as the datepicker if no year is present, or like the modified datepicker would with the year field not collapsed.  However, that may require duplicating most of the _init method of the datepicker or placing a disabled and modified 'noyear' datepicker in the contact view pane.  I could try getting the short date format with a fake year then removing that year and the previous or next character, but some formats have more than one character as the separator or more than the normal three parts.

Also, the code in v1.1 might break with some of the same date formats that break the original datepicker.

> Adding dev-doc-needed as if we take this patch we'll want to document our
> specialism of the datepicker.

Definitely, and let me know if any of the comments in the code aren't clear enough.
(Assignee)

Comment 30

8 years ago
Created attachment 384337 [details] [diff] [review]
Patch v1.3

This patch moves the date format detection into a new JavaScript module addrbookWidgets.jsm in mailnews/addrbook/src (wasn't sure where to put it or what to name it).  The date displayed in the contact view pane should match the datepicker (with the possible addition of the year).

I also added dateVal and monthVal properties to the widget to allow setting and getting the day and month more easily.  The previous patch would actually store the empty text M or D as the month and day because it used the value of the textbox without checking if it is a number.  dateVal and monthVal avoid this problem and should be used instead of the field's value.

This patch worked for me in Linux and Windows Vista with a few formats.

I'd like to take one last look and maybe add a few comments before requesting review.  Is the JS module in the correct location and used properly?
Attachment #374207 - Attachment is obsolete: true
Attachment #376372 - Attachment is obsolete: true
(Assignee)

Comment 31

8 years ago
Created attachment 384560 [details] [diff] [review]
Patch v1.3 commented

I added some comments and made a few small changes.
Attachment #384337 - Attachment is obsolete: true
Attachment #384560 - Flags: review?(bugzilla)
(Reporter)

Comment 32

8 years ago
Comment on attachment 384560 [details] [diff] [review]
Patch v1.3 commented

This is looking good, r=Standard8
Attachment #384560 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

8 years ago
Attachment #384560 - Flags: superreview?(neil)
(Assignee)

Comment 33

8 years ago
Created attachment 390653 [details] [diff] [review]
Patch v1.3 fixes bitrot

This patch should apply cleanly to the trunk.
Attachment #384560 - Attachment is obsolete: true
Attachment #390653 - Flags: superreview?(neil)
Attachment #390653 - Flags: review+
Attachment #384560 - Flags: superreview?(neil)
(Assignee)

Comment 34

8 years ago
Created attachment 419787 [details] [diff] [review]
Patch for 3.2a1pre

I'm going to request r again since it has been a while.
Attachment #390653 - Attachment is obsolete: true
Attachment #419787 - Flags: review?(bugzilla)
Attachment #390653 - Flags: superreview?(neil)
(Reporter)

Comment 35

8 years ago
Comment on attachment 419787 [details] [diff] [review]
Patch for 3.2a1pre

r=Standard8

We should think about unit tests for this - especially the widget. https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing has some more details.

I'm happy to wave the requirement for tests on check-in as this patch was done way before the requirement was raised. However I'd still like to work with you to get some tests up if you're willing to.
Attachment #419787 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

8 years ago
Attachment #419787 - Flags: superreview?(neil)
(Assignee)

Comment 36

7 years ago
Created attachment 429459 [details] [diff] [review]
Updated patch - fixes one digit dates

This patch fixes a mistake in abCardViewOverlay.js and one digit dates ('d' in Windows or %e in anything but Windows).

I cannot test with certain short date formats in Windows, such as yy/MM/dd because
alert(new Date(2010, 1, 28).toLocaleFormat("%x"));
shows "10/02/2010" for some reason.  Due to that odd behavior the datepicker throws an error stating that it cannot find the day in that string (I can't, either).

Is anyone else able to confirm that behavior with Windows?

When something like this happens, should the datepicker throw an error or fall back on some defaults?  I'm guessing the latter, but currently it will throw an error and stop initializing the datepicker.
Attachment #419787 - Attachment is obsolete: true
Attachment #429459 - Flags: superreview?(neil)
Attachment #429459 - Flags: review+
Attachment #419787 - Flags: superreview?(neil)
(Assignee)

Comment 37

7 years ago
Created attachment 429460 [details]
Test for date parsing in addrbookWidgets.jsm

This is a quick test I created for addrbookWidgets.jsm (just the file, not a patch).  It basically uses several common date formats and sends them to initDateInfo to check that it parses them correctly.
(In reply to comment #36)
> alert(new Date(2010, 1, 28).toLocaleFormat("%x"));
> shows "10/02/2010" for some reason.  Due to that odd behavior the datepicker
> throws an error stating that it cannot find the day in that string (I can't,
> either).
> 
> Is anyone else able to confirm that behavior with Windows?
No, I get "28/02/2010" here, using Windows XP.
(Reporter)

Comment 39

7 years ago
Josh, Neil, what's the way forward here?
(Assignee)

Comment 40

7 years ago
(In reply to comment #39)
> Josh, Neil, what's the way forward here?

I've been waiting on the sr or comments on my patch or test.  I can attach a patch that fixes any bitrot after this weekend is over.

Comment 41

6 years ago
Any progress on this?
Read that it has an effect on the birthday field in the address book...which is the problem I am having (see Bug 534466)
Hope can get fixed.
Thanks.
(Assignee)

Comment 42

6 years ago
(In reply to comment #41)
> Any progress on this?
> Read that it has an effect on the birthday field in the address book...which
> is the problem I am having (see Bug 534466)
> Hope can get fixed.
> Thanks.

Still waiting on that sr.  Neil, will you have time to review this?  I can update my patch to fix the bitrot but don't want to if it won't get reviewed.
(Reporter)

Comment 43

6 years ago
Comment on attachment 429459 [details] [diff] [review]
Updated patch - fixes one digit dates

Ian, could you take a look at this patch please? I'd like to get this moving forward again.
Attachment #429459 - Flags: superreview?(neil) → review?(iann_bugzilla)

Comment 44

6 years ago
Comment on attachment 429459 [details] [diff] [review]
Updated patch - fixes one digit dates

On code inspection.

>+++ b/mail/components/addrbook/content/abCardViewOverlay.js	Sun Feb 28 22:23:41 2010 -0500
>+    var day   = card.getProperty("BirthDay", null);
>     var month = card.getProperty("BirthMonth", null);
>+    var year  = card.getProperty("BirthYear", null);
Should the default value really be null or would it be better to be 0 or something like that?
>     var dateStr;
>+    // if there is a valid day and valid month, form a string representing the
>+    // birthday
>     if (day > 0 && day < 32 && month > 0 && month < 13) {
>+      var dt = new Date(2000, month - 1, day);
>       // if the year exists, just use Date.toLocaleString
>+      if (year > 0 && year < 9999) {
Wouldn't 9999 be a valid year?

>+        dt.setFullYear(year);
>+        // get the full date as the default string (system and locale-dependent)
>+        dateStr = dt.toLocaleFormat(gDateFormat);
>       }
>+      // if the year doesn't exist then use the addrbookWidgets JS module to
>+      // determine the order
>+      else {
>+        Components.utils.import("resource://app/modules/addrbookWidgets.jsm");
>+        initDateInfo(gDateFormat);
Currently initDateInfo does a return true, should we not be either checking that or just doing a return?

Pity the whole of the dateStr code isn't in the jsm.

>+++ b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd	Sun Feb 28 22:23:41 2010 -0500

>+<!-- LOCALIZATION NOTE: Please make sure the the month and day empty text fits
>+     in the birthday field (2 characters each).  Adjust the width values as
>+     necessary -->
>+<!ENTITY Month.emptytext                "M">
>+<!ENTITY Day.emptytext                  "D">
We use .placeholder these days.
>+<!ENTITY Month.width                    "1.8em">
>+<!ENTITY Day.width                      "1.8em">
Widths should be in ch rather than em

>+++ b/mail/test/mozmill/mozmilltests.list	Sun Feb 28 22:23:41 2010 -0500
>@@ -1,8 +1,1 @@
>-folder-display
>-junk-commands
>-search-window
>-cookies
>-content-policy
>-content-tabs
>-message-header
>-folder-pane
>+addressbook
Are we really wanting to get rid of all these tests?

>+++ b/mailnews/addrbook/content/abCardOverlay.xul	Sun Feb 28 22:23:41 2010 -0500
>+            <datepicker id="Birthday" type="noyear"
>+                        monthtext="&Month.emptytext;"
>+                        datetext="&Day.emptytext;"
>+                        monthwidth="&Month.width;"
>+                        datewidth="&Day.width;"/>
This is forked as well now, see previous comment about emptytext -> placeholder.

>+++ b/mailnews/addrbook/content/addrbookWidgets.xml	Sun Feb 28 22:23:41 2010 -0500

>+      <!-- Called internally to set the value for a field -->
>+      <method name="_setFieldValue">
>+        <parameter name="aField"/>
>+        <parameter name="aValue"/>
>+        <body>
>+          <![CDATA[
>+            if (aField == this.yearField && aValue > 0 && aValue < 10000) {
When is this called with a yearField as an argument?

>+              var oldDate = this._dateValue;
>+              var oldDay  = this.dateField.value;
>+              this._dateValue.setFullYear(aValue);
>+              if (oldDate != this._dateValue)
>+                this._dateValue.setDate(0);
>+              this._updateUI(this.dateField, this.date);
>+              if (!oldDay || isNaN(oldDay))
>+                this.dateField.value = oldDay;
>+            }
>+            else if (aValue != null && !isNaN(aValue)) {
>+              var oldDate = this._dateValue;
>+              if (aField == this.monthField)
>+                this._dateValue.setMonth(aValue);
>+              else
>+                this._dateValue.setDate(aValue);
>+              if (oldDate != this._dateValue)
>+                this._dateValue.setDate(0);
>+              this.setAttribute("value", this.value);
>+              this._updateUI(this.dateField, this.date);
>+              var month = this._dateValue.getMonth() + 1;
>+              this.monthField.value = month < 10 && this.monthLeadingZero
>+                ? "0" + month
>+                : month;
Do we need to do anything with month when we're passed something that is not a monthField?

>+            var max = (aField == this.monthField) ? 11 : 9999;
>+            var min = (aField == this.monthField) ?  0 :    1;
It's a shame that we're having to keep doing the same constraint tests in various different parts.

>+            // set the size and maxLength of each field
>+            this.yearField.size       = twoDigitYear ? 2 : 4;
>+            this.yearField.maxLength  = twoDigitYear ? 2 : 4;
>+            this.monthField.size      = 2;
>+            this.monthField.maxLength = 2;
>+            this.dateField.size       = 2;
>+            this.dateField.maxLength  = 2;
Would we ever have a field type that wasn't numeric? e.g. full or abbrev for month?

>+            // display the empty text
>+            this.dateField.style.color  = "GrayText";
>+            this.monthField.style.color = "GrayText";
Should use a class rather than a color so that correct color is used depending on OS/theme.

>+            var color = "Black";
>+            if (!this.hasAttribute("focused") &&
>+                !(this.dateField.value && this.monthField.value) ||
>+                this.dateField.value == this.datetext) {
>+              this.setAttribute("empty", true);
>+              color = "GrayText";
>+              this.dateField.value  = this.datetext  ? this.datetext  : null;
>+              this.monthField.value = this.monthtext ? this.monthtext : null;
>+            }
>+            this.dateField.style.color  = color;
>+            this.monthField.style.color = color;
Should use a class rather than a color.

>+            if (this.hasAttribute("empty")) {
>+              this.dateField.value  = "";
>+              this.monthField.value = "";
>+              this.dateField.style.color  = "Black";
>+              this.monthField.style.color = "Black";
Should use a class rather than a color.

On some basic testing:
Manually enter 31/01
Go back change 01 to 02
Automatically changes to 02/03 - is that correct behaviour?

Manually enter 31/02
Automatically changes to 01/02 - is that correct behaviour?

Shouldn't the behaviour be consistent?

The spacing of M/ D looks a bit strange too but not sure if M/D looks any better either. Perhaps MM/DD?

r- for the moment, I think it would be good to get sr or feedback from Neil too
Attachment #429459 - Flags: review?(iann_bugzilla) → review-

Comment 45

2 years ago
It is a pity this piece of work wasn't finished.

Pi, are you still working on these AB bugs?
You need to log in before you can comment on or make changes to this bug.