Last Comment Bug 488627 - Datepicker doesn't work with some short date formats.
: Datepicker doesn't work with some short date formats.
Status: RESOLVED FIXED
[qa?]
:
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on:
Blocks: 534466 677884
  Show dependency treegraph
 
Reported: 2009-04-15 21:35 PDT by Josh Geenen (:pi)
Modified: 2011-12-12 05:29 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Shows the problem in datepickers (49.11 KB, image/jpeg)
2009-04-16 19:19 PDT, Josh Geenen (:pi)
no flags Details
A patch that should work with more date formats (7.49 KB, patch)
2009-04-19 15:08 PDT, Josh Geenen (:pi)
no flags Details | Diff | Splinter Review
The datepickers after the patch was applied (48.31 KB, image/jpeg)
2009-04-19 15:17 PDT, Josh Geenen (:pi)
no flags Details
Patch v1 (2.06 KB, patch)
2011-10-04 13:01 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (2.22 KB, patch)
2011-10-04 13:52 PDT, Mike Conley (:mconley) - (Needinfo me!)
enndeakin: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Josh Geenen (:pi) 2009-04-15 21:35:58 PDT
The birthday datepicker (including my most current widget for Bug 456024) doesn't work with certain date formats as mentioned in Bug 456024 comment 11.

So far, any short date format with MMM and MMMM as the month result in Oct or October between the actual month (as a number from 01 - 12) and day in Vista.  The original reporter was using XP.  I haven't tried to reproduce it in Linux yet.

I'll break out the DOM Inspector and take a look soon.  At least one problem lies in the _init method when collapsing elements, and _setFieldValue may need to detect the short date month format and use that instead of 01 through 12.
Comment 1 Josh Geenen (:pi) 2009-04-16 19:19:16 PDT
Created attachment 373249 [details]
Shows the problem in datepickers

The screenshot shows four datepickers (noyear/birthday, popup, grid, and normal) when the short date format contains MMM.  All but the grid datepicker are incorrect, and datetimepickers are probably affected as well. MMMM behaves the same way, just with the full month name where the abbreviation is in the datepickers.

It looks like the datepickers find the month (and preceding space) as a separator because of a regular expression which assumes a numeric month in the short date format (%x):
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/datetimepicker.xml#790
numberFields is:
0 - 04 Oct 2002
1 - 
2 - 04
3 -  Oct 
4 - 200
5 - 
6 - 2
7 - 

Versus MM/DD/YYYY:
0 - 10/04/2002
1 - 
2 - 10
3 - /
4 - 04
5 - /
6 - 2002
7 - 

The month is October because of line 792.
Comment 2 Sean Smith 2009-04-16 20:33:31 PDT
For what it's worth, this doesn't seem reproduceable on OS X (10.5.6).  There are four date formats in OS X: short, medium, long and full.

Setting short and medium to dd MMM yy and dd MMM yyyy respectively don't trigger the Oct phenomenon.
Comment 3 Josh Geenen (:pi) 2009-04-19 15:02:42 PDT
I'm moving this into toolkit since it is a bug with normal and popup datepickers, too.

(In reply to comment #2)
> For what it's worth, this doesn't seem reproduceable on OS X (10.5.6).  There
> are four date formats in OS X: short, medium, long and full.
> 
> Setting short and medium to dd MMM yy and dd MMM yyyy respectively don't
> trigger the Oct phenomenon.
Interesting, I'm guessing that evaluating this line in the Error Console shows a numeric month even with MMM set?
alert(new Date().toLocaleFormat("%x"));

I can reproduce the bug in Windows by changing MM to MMM in the short date format and in Linux by changing %m in d_fmt to %b, for example.
Comment 4 Josh Geenen (:pi) 2009-04-19 15:08:31 PDT
Created attachment 373586 [details] [diff] [review]
A patch that should work with more date formats

This patch (a work in progress) fixes the bug for me with most date formats but still only uses numeric dates regardless of the system format.  It replaces the regex with several checks for different formats for the year, month, and date.  The separators are hardcoded at the moment.

I'll take the bug and create a formal patch for review if this patch is a solution worth pursuing.
Comment 5 Josh Geenen (:pi) 2009-04-19 15:17:29 PDT
Created attachment 373587 [details]
The datepickers after the patch was applied
Comment 6 Josh Geenen (:pi) 2009-04-21 18:38:33 PDT
Here are most of the unique d_fmt variables in /usr/share/i18n/locales/ on my machine running Gentoo.  I wrote a script to do this so there may be some mistakes.  See the link below for details on what each variable means.
%A %d %B %Y
%A %d %b
%A %d %b %Y
%A %d,%B,%Y
%A, %B %d, %Y
%A, %d %B, %Y
%A, %d t
%B %d %A %Y
%Y-%b-%d
%Y-%m-%d
%Y.%m.%d
%Y.%m.%d.
%a %e.%b %Y
%d %b %Y
%d %b, %Y
%d-%m-%Y
%d-%m-%y
%d. %b %Y
%d. %m. %Y
%d. %m. %y
%d.%m.%Y
%d.%m.%Y.
%d.%m.%y
%d/%d/%Y
%d/%m-%Y
%d/%m/%Y
%d/%m/%y
%e-%m-%Y
%e.%m.%Y
%m/%d/%Y
%m/%d/%y

http://www.cplusplus.com/reference/clibrary/ctime/strftime/
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2011-10-04 13:01:07 PDT
Created attachment 564651 [details] [diff] [review]
Patch v1

Here's my first run at a patch.  If we cannot figure out the ordering of the month, day and years from the system locale, we default to YYYY/MM/DD.

All feedback welcome!

-Mike
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2011-10-04 13:03:59 PDT
Locally tested, and seems to fix the issue for bug 677884 and I believe addresses bug 534466.
Comment 9 Neil Deakin 2011-10-04 13:42:44 PDT
Comment on attachment 564651 [details] [diff] [review]
Patch v1

>             var dt = new Date(2002,9,4).toLocaleFormat("%x");
>             var numberFields = dt.match(numberOrder);
>             if (numberFields) {
>+              this.yearLeadingZero = (numberFields[yi].length > 1);
>+              this.monthLeadingZero = (numberFields[mi].length > 1);
>+              this.dateLeadingZero = (numberFields[di].length > 1);

yi, mi and di aren't defined yet.
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2011-10-04 13:52:51 PDT
Created attachment 564661 [details] [diff] [review]
Patch v2

Gah, you're absolutely right - great catch!

This time, tested with both a funky locale (with the date time format being Tuesday Oct 4), and my standard locale (11-10-2011).
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2011-10-05 14:34:36 PDT
Ok, this looks good on try (https://tbpl.mozilla.org/?tree=Try&rev=9438fb599e7f), committed to inbound: 

https://hg.mozilla.org/integration/mozilla-inbound/rev/79dc3f1ea63a
Comment 12 Mike Conley (:mconley) - (Needinfo me!) 2011-10-05 14:57:37 PDT
Comment on attachment 564661 [details] [diff] [review]
Patch v2

It's a pretty safe patch on code that hasn't been changed in quite a while (hg log says July).

I'm on the Thunderbird team, and we'd love to get this fixed for TB 9 - even 8, if we can muster it.
Comment 13 Ed Morley [:emorley] 2011-10-06 03:47:11 PDT
https://hg.mozilla.org/mozilla-central/rev/79dc3f1ea63a

Was likely just a one off due to muscle memory, but normally bugs are only marked fixed when they merge to mozilla-central, rather than when they land on inbound :-)
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2011-10-06 05:09:34 PDT
Ed:

Whoops - you're right - sorry about that.  :)

-Mike
Comment 15 christian 2011-10-06 14:20:32 PDT
Comment on attachment 564661 [details] [diff] [review]
Patch v2

Approved for aurora. We are denying this for beta, though feel free to keep landing it on beta relbranches if it is that important.
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-17 12:21:05 PST
Is there a minimized testcase QA can use to verify this fix?
Comment 17 Mike Conley (:mconley) - (Needinfo me!) 2011-11-22 06:18:05 PST
Anthony:

There are STR for Thunderbird here:  https://bugzilla.mozilla.org/show_bug.cgi?id=534466#c0

-Mike
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 11:15:15 PST
(In reply to Mike Conley (:mconley) from comment #17)
> Anthony:
> 
> There are STR for Thunderbird here: 
> https://bugzilla.mozilla.org/show_bug.cgi?id=534466#c0
> 
> -Mike

Does this only affect Thunderbird?
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2011-12-08 11:26:54 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18)
> Does this only affect Thunderbird?

No, I imagine this would affect anyone using datepicker with certain region settings for date (for example,  https://bug534466.bugzilla.mozilla.org/attachment.cgi?id=417314)

-Mike
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 11:41:25 PST
Okay, we will try to verify the fix using the testcase in comment 19.
Comment 21 Ioana (away) 2011-12-12 05:29:07 PST
Verified on:
Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0
Build ID: 20111206234556

STR:
1.  Modify regional settings short-date format to an extended format (eg "MMM", "MMMM").
2.  Restart Firefox so that it reads the new date-format.
3.  Evaluate "new Date().toLocaleFormat("%x")" in the Error Console.

Since today is 12/12/2011 I changed the date to 12/11/2011 to make sure if the bug still reproduced it was visible. The right date was displayed in the Error Console ("12-Nov-2011", "12-November-2011").

Is this a right way to verify this bug? 
The steps are those referenced in comment 18, except for the last one (there is no address book in Firefox). I am asking this since I tried to reproduce the bug on some older builds using the above STR and the bug never reproduced.

Note You need to log in before you can comment on or make changes to this bug.