Last Comment Bug 608603 - datepicker (popup) Forward button: October 31 advances to December
: datepicker (popup) Forward button: October 31 advances to December
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Neil Deakin
:
: Neil Deakin
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-31 07:25 PDT by George
Modified: 2011-04-13 11:28 PDT (History)
3 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sample (181 bytes, application/vnd.mozilla.xul+xml)
2010-11-01 01:11 PDT, Alice0775 White
no flags Details
Patch with tests (8.75 KB, patch)
2010-11-15 12:02 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Add displayedYear field (12.39 KB, patch)
2011-03-14 09:58 PDT, Neil Deakin
neil: review+
Details | Diff | Splinter Review
address comments (12.41 KB, patch)
2011-03-15 07:12 PDT, Neil Deakin
no flags Details | Diff | Splinter Review

Description George 2010-10-31 07:25:29 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12

On October 31,2010  clicking the forward button in the popup shows December, 2010

Reproducible: Always

Steps to Reproduce:
1. Click dropmarker
2. Click forward in popup
3.
Actual Results:  
Shows December, 2010

Expected Results:  
Show November, 2010
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2010-10-31 12:50:29 PDT
Where are you seeing this datepicker?
Comment 2 George 2010-10-31 19:00:46 PDT
(In reply to comment #1)
> Where are you seeing this datepicker?

In an extension I am developing. XUL:

<datepicker id="simdown-dpickDate"
										type="popup"
										tooltiptext="&tip.datepicker;" />
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2010-10-31 19:32:44 PDT
Ah, ok.  I can confirm this; can't easily check whether it's still happening on trunk, though.  :(
Comment 4 Alice0775 White 2010-11-01 01:11:22 PDT
Created attachment 487298 [details]
sample
Comment 5 Neil Deakin 2010-11-01 12:11:50 PDT
Looks like the datepicker just needs to handle the following case:
 new Date(new Date("Oct 31 2010").setMonth(10))

where there is no November 31.
Comment 6 George 2010-11-01 12:45:56 PDT
(In reply to comment #5)
> Looks like the datepicker just needs to handle the following case:
>  new Date(new Date("Oct 31 2010").setMonth(10))
> 
> where there is no November 31.

Yes, I think this occurs whenever the day of the current month does not exist in the next month, eg January 29, 30 or 31, 2011 will advance to March, 2011
Comment 7 Neil Deakin 2010-11-15 12:02:58 PST
Created attachment 490642 [details] [diff] [review]
Patch with tests
Comment 8 neil@parkwaycc.co.uk 2011-03-08 05:53:47 PST
Comment on attachment 490642 [details] [diff] [review]
Patch with tests

I don't understand the point of displayedMonth. What is it for?

>+              if (aValue != -1 && aValue != 12 &&
It's fortunate that January and December have the same number of days.
Comment 9 Neil Deakin 2011-03-09 12:12:39 PST
(In reply to comment #8)
> Comment on attachment 490642 [details] [diff] [review]
> Patch with tests
> 
> I don't understand the point of displayedMonth. What is it for?

The actual fix is just the change at line 1040 or so.

The rest is for the test, but there is benefit of having a property for the displayed month in general, since, in the popup datepicker, pressing the month forward and back navigation buttons adjusts the month that is displayed, but doesn't affect the currently selected month.

> 
> >+              if (aValue != -1 && aValue != 12 &&
> It's fortunate that January and December have the same number of days.

I'm assuming that was just an observation rather than something to change.
Comment 10 neil@parkwaycc.co.uk 2011-03-11 14:38:47 PST
(In reply to comment #9)
> The actual fix is just the change at line 1040 or so.
Right, I have no trouble with that.

> The rest is for the test, but there is benefit of having a property for the
> displayed month in general, since, in the popup datepicker, pressing the month
> forward and back navigation buttons adjusts the month that is displayed, but
> doesn't affect the currently selected month.
Surely the month isn't much use without the year...

> > >+              if (aValue != -1 && aValue != 12 &&
> > It's fortunate that January and December have the same number of days.
> I'm assuming that was just an observation rather than something to change.
Indeed. I don't know how hard it would be not to require that assumption.
Comment 11 Neil Deakin 2011-03-14 09:58:14 PDT
Created attachment 519168 [details] [diff] [review]
Add displayedYear field
Comment 12 neil@parkwaycc.co.uk 2011-03-15 06:58:06 PDT
Comment on attachment 519168 [details] [diff] [review]
Add displayedYear field

>       <method name="_updateUI">
>         <parameter name="aField"/>
>         <parameter name="aValue"/>
>-        <parameter name="aIncDecMonth"/>
>+        <parameter name="aChangeMonth"/>
Conveniently this makes it easier for someone to add UI to page by year. But maybe the parameter should be named aCheckMonth. (Or can you get rid of it completely, since you only need to check the month if you provide a field?)

>+                var valueToSet = aValue;
>+                aValue = this._displayedDate.getMonth();
>+                this._displayedDate.setFullYear(valueToSet);
>+              }
>+
>+              if (aValue != -1 && aValue != 12 &&
>+                  aValue != this._displayedDate.getMonth()) {
Rather than using aValue, I think it would be clearer if you used a new variable; "expectedMonth" was my best stab at a name. r=me with that fixed.
Comment 13 Neil Deakin 2011-03-15 07:07:32 PDT
(In reply to comment #12)
> Or can you get rid of it completely, since you only need to check the month if
> you provide a field?

The field is also supplied in calls from _setFieldValue.
Comment 14 Neil Deakin 2011-03-15 07:12:56 PDT
Created attachment 519397 [details] [diff] [review]
address comments

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