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

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: George, Assigned: Neil Deakin)

Tracking

unspecified
x86
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

181 bytes, application/vnd.mozilla.xul+xml
Details
12.41 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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
Where are you seeing this datepicker?
(Reporter)

Comment 2

7 years ago
(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;" />
Ah, ok.  I can confirm this; can't easily check whether it's still happening on trunk, though.  :(
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

7 years ago
Created attachment 487298 [details]
sample
(Assignee)

Comment 5

7 years ago
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.
(Reporter)

Comment 6

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

Comment 7

7 years ago
Created attachment 490642 [details] [diff] [review]
Patch with tests
(Assignee)

Updated

7 years ago
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #490642 - Flags: review?(neil)

Comment 8

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

Comment 9

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

Comment 11

6 years ago
Created attachment 519168 [details] [diff] [review]
Add displayedYear field
Attachment #490642 - Attachment is obsolete: true
Attachment #519168 - Flags: review?(neil)
Attachment #490642 - Flags: review?(neil)
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.
Attachment #519168 - Flags: review?(neil) → review+
(Assignee)

Comment 13

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

Comment 14

6 years ago
Created attachment 519397 [details] [diff] [review]
address comments
Attachment #519168 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/mozilla-central/rev/77a7b2f27c58
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.