608603 - datepicker (popup) Forward button: October 31 advances to December

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[George]

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 [:bzbarsky]]

Where are you seeing this datepicker?

Comment 2

[George]

(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 [:bzbarsky]]

Ah, ok.  I can confirm this; can't easily check whether it's still happening on trunk, though.  :(

Comment 4

[Alice0775 White]

Attachment: sample

Comment 5

[Neil Deakin]

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]

(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]

Attachment: Patch with tests

Comment 8

[[e-mail]]

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]

(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

[[e-mail]]

(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]

Attachment: Add displayedYear field

Comment 12

[[e-mail]]

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]

(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]

Attachment: address comments

Comment 15

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/77a7b2f27c58