Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#9366 closed defect (fixed)

DateTextBox: cannot select 2009-10-18 (end of DST) w/Argentine timezone

Reported by: mariano Owned by: Adam Peller
Priority: high Milestone: 1.3.2
Component: Dijit - Form Version: 1.3.1
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by Adam Peller)

Set timezone to Buenos Aires (ART/ARST) Load http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/form/test_DateTextBox.html Try to select 2009-10-18. The value returned is 11pm the previous day. Try typing this value directly in the input field. The result returned is undefined, as if it did not parse.

See also #8521

Attachments (1)

dst.patch (9.0 KB) - added by Adam Peller 10 years ago.
_Calendar will return 1am where midnight is a dst leap. dojo.date.locale.parse will do the same.

Download all attachments as: .zip

Change History (38)

comment:1 Changed 10 years ago by Adam Peller

Description: modified (diff)

comment:2 Changed 10 years ago by Adam Peller

>>> new Date(2009,9,18)
Sat Oct 17 2009 23:00:00 GMT-0300 (ART)

comment:3 Changed 10 years ago by bill

Summary: Cannot select 2009-10-18 in DateTextBoxDateTextBox: cannot select 2009-10-18 (end of DST) w/Argentine timezone

From talking w/Adam, it's off by one hour because of the daylight savings time switch, which happens on 10/18 in Argentina.

Also, Argentina has only followed the DST switch since last year. And their DST change happens at midnight, not like America which (IIRC) happens at 2AM.

comment:4 Changed 10 years ago by Adam Peller

more fun related to this bug and assumptions about midnight existing:

>>> dojo.date.locale.parse("10/18/2009", {selector:'date',fullYear:true})
null

Changed 10 years ago by Adam Peller

Attachment: dst.patch added

_Calendar will return 1am where midnight is a dst leap. dojo.date.locale.parse will do the same.

comment:5 Changed 10 years ago by Adam Peller

Resolution: fixed
Status: newclosed

(In [17690]) Use 1am for date-timese where midnight is a dst leap. Fixes #9366, Refs #8521 !strict

comment:6 Changed 10 years ago by Adam Peller

Milestone: tbd1.4
Version: 1.3.01.3.1

comment:7 Changed 10 years ago by bill

See also #9107.

comment:8 in reply to:  4 Changed 10 years ago by davidmark

Replying to peller:

more fun related to this bug and assumptions about midnight existing:

>>> dojo.date.locale.parse("10/18/2009", {selector:'date',fullYear:true})
null

All you ever wanted to know about JS date issues:

http://www.merlyn.demon.co.uk/js-dates.htm

In particular, this seems relevant:

http://www.merlyn.demon.co.uk/js-datex.htm#OST

Not sure why the report is Opera-centric, but it looks like a similar problem.

comment:9 Changed 10 years ago by Adam Peller

@davidmark: That page is a bit difficult to follow. I don't think that bug is related. In the affected timezones, the DST change actually occurs at midnight, so I'm not sure there really is a midnight for that particular day... it goes straight to 1am. Still, getting 11pm the day before seems arbitrary. Perhaps it's compounded by an adjustment back in the fall instead of forward? (This is the southern hemisphere)

#9107 demonstrates what seems to be a timezone- and browser-dependent FF bug, and I don't think it's documented anywhere. I've been unable to get anyone at Mozilla to look at it so far.

comment:10 in reply to:  9 Changed 10 years ago by davidmark

Replying to peller:

@davidmark: That page is a bit difficult to follow.

No question. It holds very good information, but it can be hard to excavate.

I don't think that bug is related. In the affected timezones, the DST change actually occurs at midnight, so I'm not sure there really is a midnight for that particular day... it goes straight to 1am. Still, getting 11pm the day before seems arbitrary. Perhaps it's compounded by an adjustment back in the fall instead of forward? (This is the southern hemisphere)

Ask in CLJ. Stockton will almost certainly answer. JS dates, time zones, etc. are his thing.

#9107 demonstrates what seems to be a timezone- and browser-dependent FF bug, and I don't think it's documented anywhere. I've been unable to get anyone at Mozilla to look at it so far.

No doubt Stockton will be interested in that (assuming he doesn't already know about it.)

comment:12 in reply to:  11 Changed 10 years ago by davidmark

Replying to peller:

Ok, following up in c.l.j: http://groups.google.com/group/comp.lang.javascript/browse_thread/thread/30fd42a73261331c#

...and we're back. I recommend we back this out and re-evaluate what went wrong (seems this is only good for certain days in certain countries.)

comment:13 Changed 10 years ago by Adam Peller

@davidmark, I'm not sure that c.l.j. tangent was at all helpful. I've patched the code to cover underflow due to DST and tested it in the known problem environment. Because host behavior and local dst rules are not part of ECMAScript nor fixed going forward, we need to be able to tolerate situations like this, and we will likely rely on user testing, but I've covered the situations I could think of.

comment:14 Changed 10 years ago by Adam Peller

ok, so if we don't want to make the 1 hr assumption -- adding an hour to get out of this situation -- a safer algorithm might be, if setting a date to midnight results in an underflow, round up to midnight of the next day -- I'd probably do that by adding a day, then setting hours to 0. Can we assume that an underflow is from daylight savings and no greater than one day? I'd hope so. There's only so much code I want to add to cover this.

comment:15 Changed 10 years ago by Adam Peller

ugh, sorry, I meant rounding up to something safe the next day... setting hours to 0 would get us back to the previous day again :) So there, we'd probably set the hours to the change in offsets between the two days, usually 1. Sound reasonable?

comment:16 in reply to:  13 ; Changed 10 years ago by davidmark

Replying to peller:

@davidmark, I'm not sure that c.l.j. tangent was at all helpful. I've patched the code to cover underflow due to DST and tested it in the known problem environment. Because host behavior and local dst rules are not part of ECMAScript nor fixed going forward, we need to be able to tolerate situations like this, and we will likely rely on user testing, but I've covered the situations I could think of.

I think it was very helpful as I have an understanding of the various issues and several proposed solutions. The main issue is that we are storing a date without a time as a *local* Date. See Stockton's last comment (from just a moment ago.) We have several options for dealing with this.

I always want to know why a patch works, even if it can be demonstrated to work everywhere (and at any time in this case.) We know we can't test this one everywhere, so I think we should concentrate on the underlying issues with date storage (appears the ramifications are far-reaching.)

comment:17 in reply to:  15 ; Changed 10 years ago by davidmark

Replying to peller:

ugh, sorry, I meant rounding up to something safe the next day... setting hours to 0 would get us back to the previous day again :) So there, we'd probably set the hours to the change in offsets between the two days, usually 1. Sound reasonable?

If you mean, use new Date, check for any underflow and round the date up when needed, I think that is fine for this widget (or anything else that needs to store a date without time.) The time precision is gone after this adjustment, so we must make sure that this does not step on dates that will later be treated as times.

comment:18 in reply to:  17 ; Changed 10 years ago by Adam Peller

Replying to davidmark:

If you mean, use new Date, check for any underflow and round the date up when needed,

I think that is fine for this widget (or anything else that needs to store a date without time.) The time precision is gone after this adjustment, so we must make sure that this does not step on dates that will later be treated as times.

As you can see, the change is not specific to a widget. It needs to be done when date-only Date objects are calculated, e.g. dojo.date.locale.parse. The code checks for underflow without creating a new Date, and dojo.date.add can be used to increment the Date. For date-only Dates, the idea all along was to lose precision, hence using midnight. Sure, in retrospect, we would have been better off using noon, but you might say we're stuck with midnight for back-compat reasons, and this is a workaround only for these special cases.

comment:19 in reply to:  16 ; Changed 10 years ago by Adam Peller

Replying to davidmark:

I think it was very helpful as I have an understanding of the various issues and

several proposed solutions.

Glad you thought so :) The short summary I was hoping to get to clarify this for the c.l.j. community, perhaps buried in that thread somewhere, is that midnight through 00:59 in this place & time does not exist, so using the Date constructor with this data has to return something arbitrary. Unfortunately, the previous hour it repeats is on another date. So be it. This is a host decision, apparently, and we need to tolerate it.

The main issue is that we are storing a date without a time as a *local* Date.

Yes, I know. That was by design. Telling our users to deal in UTC is simply not usable IMO, sorry. Whatever, we're stuck with it. Yes, we need to use UTC internally for some calculations to achieve correct results, but by design we interact with the developer in local time.

comment:20 in reply to:  19 Changed 10 years ago by davidmark

Replying to peller:

Replying to davidmark:

I think it was very helpful as I have an understanding of the various issues and

several proposed solutions.

Glad you thought so :) The short summary I was hoping to get to clarify this for the c.l.j. community, perhaps buried in that thread somewhere, is that midnight through 00:59 in this place & time does not exist, so using the Date constructor with this data has to return something arbitrary. Unfortunately, the previous hour it repeats is on another date. So be it. This is a host decision, apparently, and we need to tolerate it.

I had a distinct feeling that we were chasing the wrong issue. Turns out to be the case. The Date constructor is not at fault. The issue is that we are storing a time/date as a local Date. Once the stored point in time is converted back to a string (for display as in the test or to send to the server), it may be incorrect (e.g. a day off.) And for reasons I haven't investigated yet, parsing may fail completely.

The other feeling I had was that this patch was for an arbitrary time and country. That has been confirmed, so we need to change the algorithm. I'm leaning towards making this adjustment for date-only storage (i.e. rounding up when the Date does not match the passed date) optional (e.g. with a flag.) This calendar widget is an example where you would want to set this flag. Routines that calculate time differences (e.g. a scheduler) would not.

The main issue is that we are storing a date without a time as a *local* Date.

Yes, I know. That was by design. Telling our users to deal in UTC is simply not usable IMO, sorry. Whatever, we're stuck with it. Yes, we need to use UTC internally for some calculations to achieve correct results, but by design we interact with the developer in local time.

We can expose whatever we want to the users. The internals are the problem. This calendar widget is a good example of how the internals need to be changed. Take persisting the selection as a cookie and restoring it later. You'd lose a day. That shouldn't happen and won't happen once this patch is finished. Should be documented that the time portion of the Date value exposed by the calendar is arbitrary (or set it to the same for all.)

There are likely some other widgets that would benefit from this optional adjustment. Anything that deals with dates should be looked at. I'm going to be working on/with Dijit shortly and will keep these issues in mind.

comment:21 in reply to:  18 Changed 10 years ago by davidmark

Replying to peller:

Replying to davidmark:

If you mean, use new Date, check for any underflow and round the date up when needed,

I think that is fine for this widget (or anything else that needs to store a date without time.) The time precision is gone after this adjustment, so we must make sure that this does not step on dates that will later be treated as times.

As you can see, the change is not specific to a widget. It needs to be done when date-only Date objects are calculated, e.g. dojo.date.locale.parse. The code checks for underflow without creating a new Date, and dojo.date.add can be used to increment the Date. For date-only Dates, the idea all along was to lose precision, hence using midnight. Sure, in retrospect, we would have been better off using noon, but you might say we're stuck with midnight for back-compat reasons, and this is a workaround only for these special cases.

Sure. See my other reply. I'm fine with it, but it needs to be optional (unless there is never a case where you need the time portion.) I am about to delve into the date handling and look at the previous patch.

comment:22 in reply to:  18 ; Changed 10 years ago by davidmark

Replying to peller:

Just checked in my changes. Underflow is adjusted optionally in parse. Calendar widget doesn't appear to parse. Calendar calls same adjustment function on day click, which is the only internal set that requires attention (and provides an expected day.) Default get/set use raw date values, so no expected date can be inferred. Those are GIGO.

Oddly enough, my calendar had no getter. Updated that as well. Looks to be some other minor changes since the branch.

Note that the test for underflow is different than in the current patch. The parse tries to correct if the parsed date does not match the computed date. A day is added and the hour set to midnight. That will get these DST shifting cases past the final validation, which compares both month and day (previously returning null for these as the day was one off.)

comment:23 in reply to:  22 ; Changed 10 years ago by Adam Peller

Replying to davidmark: Did you try this against the test case? As I mentioned earlier, I'm pretty confident that setHours(0) won't work. It just puts you back at 23pm, on the wrong calendar day, where you started. There is no midnight in Buenos Aires on that date.

Also, I'm not sure what's wrong with doing the Calendar fix-up in the getter, rather than calling and adjustment function on day click.

comment:24 in reply to:  23 Changed 10 years ago by davidmark

Replying to peller:

Replying to davidmark: Did you try this against the test case? As I mentioned earlier, I'm pretty confident that setHours(0) won't work. It just puts you back at 23pm, on the wrong calendar day, where you started. There is no midnight in Buenos Aires on that date.

If setHours won't work, then we will bump it another way. It's only done in one place now. And I don't think I ever reproduced the problem at all. So let me know if setHours is a no go.

Also, I'm not sure what's wrong with doing the Calendar fix-up in the getter, rather than calling and adjustment function on day click.

The widget's getter and setter should have no side effects on the values. The GUI generates its own value, which gets corrected when it does not match the selection. Everything else is handled by parse. The new dateOnly option is easy to implement, just add it to constraints. I just updated the test page.

There was another mutation in there too. Related to this problem I'm sure.

comment:25 in reply to:  23 Changed 10 years ago by davidmark

Replying to peller:

Yeah, tried again on XP. Can't reproduce it at all. Assuming setHours re-triggers the conversion problem, I changed it to add a day and then loop back one hour at a time until it finds the date threshold. As noted, it may not actually be midnight, but the date will definitely be correct.

comment:26 Changed 10 years ago by Adam Peller

It's essential that you can reproduce the bug if you're going to attempt a patch. I just reproduced on XP. All I had to do was set the timezone to Buenos Aires and use the nightly build from June 1. (Last night's has my fix)

What's wrong with just taking the current fix, and instead of adding 1, add the difference in timezone offsets between the day before the DST shift and the day after? It's O(1), and only a few additional lines of code.

Frankly, I'm not that uncomfortable with leaving the current code alone and assuming a 1 hr daylight savings shift. Do you know of any dsts that are more than an hour? I suppose it's possible now or in the future.

comment:27 in reply to:  26 ; Changed 10 years ago by davidmark

Replying to peller:

It's essential that you can reproduce the bug if you're going to attempt a patch. I just reproduced on XP. All I had to do was set the timezone to Buenos Aires and use the nightly build from June 1. (Last night's has my fix)

I wanted you or the OP to test it and then you could adjust your patch. You've got a slightly newer set of these files at the moment. And I don't necessarily need to see the symptom to fix this problem, but it would have been nice. I tried twice.

Do you mean to say that the test page linked to this has been fixed? If so, that would explain it.

What's wrong with just taking the current fix, and instead of adding 1, add the difference in timezone offsets between the day before the DST shift and the day after? It's O(1), and only a few additional lines of code.

The current fix is very different from mine for both the parse and the widget (also has some other stuff mixed in.) I do think the lines of code involved has remained roughly the same. I created a new method, but removed duplicate logic from the widget. I really think this logic should be centralized in the locale module.

Frankly, I'm not that uncomfortable with leaving the current code alone and assuming a 1 hr daylight savings shift. Do you know of any dsts that are more than an hour? I suppose it's possible now or in the future.

I'm not uncomfortable with it, but it will make the merge easier if the two branches make the same change now. No idea about other time zones, but I'm assuming there are others.

comment:28 in reply to:  27 ; Changed 10 years ago by Adam Peller

Replying to davidmark:

Do you mean to say that the test page linked to this has been fixed? If so, that would explain it.

Yes, the 'nightly' part of the url is a symlink to the most current build. To see the bug, set your machine to Buenos Aires and hit http://archive.dojotoolkit.org/dojo-2009-06-01/dojotoolkit/dijit/tests/form/test_DateTextBox.html YMMV on reproducing the undefined part -- I think that only happened on a Mac.

I would not suggest making patches for anything but feature detection on the feature detection branch, or it's going to be hell to merge back in, IMHO.

comment:29 Changed 10 years ago by bill

To reiterate what Adam said, I'm not going to allow other changes slipping into dijit along with feature-detection changes. If you have some other refactoring you want to do you'll need to submit those patches separately, only after actually *testing* them, with an explanation about why the refactoring is good.

comment:30 Changed 10 years ago by Adam Peller

(In [17731]) Merge [17730] to correct comment. Refs #9366

comment:31 in reply to:  28 Changed 10 years ago by davidmark

Replying to peller:

Replying to davidmark:

Do you mean to say that the test page linked to this has been fixed? If so, that would explain it.

Yes, the 'nightly' part of the url is a symlink to the most current build. To see the bug, set your machine to Buenos Aires and hit http://archive.dojotoolkit.org/dojo-2009-06-01/dojotoolkit/dijit/tests/form/test_DateTextBox.html YMMV on reproducing the undefined part -- I think that only happened on a Mac.

I think we're done at this point. (?)

I would not suggest making patches for anything but feature detection on the feature detection branch, or it's going to be hell to merge back in, IMHO.

Too late. It will require a collaborative effort to test and merge, but it will be well worth it. :)

comment:32 in reply to:  29 Changed 10 years ago by davidmark

Replying to bill:

To reiterate what Adam said, I'm not going to allow other changes slipping into dijit along with feature-detection changes. If you have some other refactoring you want to do you'll need to submit those patches separately, only after actually *testing* them, with an explanation about why the refactoring is good.

These aren't patches I'm working on. And I expect everyone will help out with the testing. :)

comment:33 in reply to:  30 Changed 10 years ago by davidmark

Replying to peller:

(In [17731]) Merge [17730] to correct comment. Refs #9366

This looks like the previous. Are we going to re-open this now that we know the original patch is using the wrong algorithm?

comment:34 Changed 10 years ago by Adam Peller

@davidmark: [17731] was primarily to correct the ticket number in the comment. I'm satisfied with what's in trunk, including the comment I left as a TODO. I don't see any problem with the current algorithm. You haven't presented me with any failing test cases, and I can't justify a loop and extra function entry point to solve this problem. Anyway, we usually discourage this level of detail in trac comments. Please feel free to contact me in IRC or by e-mail (peller at dojotoolkit dot org) if you wish to discuss further.

As for bill's comment, I think he is suggesting that any change to Dojo is a patch, whether in a branch or a file, and must be tested by its author prior to being submitted.

comment:35 in reply to:  34 Changed 10 years ago by davidmark

Replying to peller:

@davidmark: [17731] was primarily to correct the ticket number in the comment. I'm satisfied with what's in trunk, including the comment I left as a TODO. I don't see any problem with the current algorithm. You haven't presented me with any failing test cases, and I can't justify a loop and extra function entry point to solve this problem. Anyway, we usually discourage this level of detail in trac comments. Please feel free to contact me in IRC or by e-mail (peller at dojotoolkit dot org) if you wish to discuss further.

The current algorithm is wrong for several reasons noted. For one, you can't test only if the date is less than expected (think about it.) I've removed hundreds of extra function calls where they matter. It doesn't matter in this case and we need the logic centralized for other modules (e.g. the calendar widget.) The trunk has the widget wrong as well (the getter/setter should not mutate.) And the (one line) loop is the only sure way to get the correct time (e.g. "midnight.") It will virtually never run, so I'm not worried about performance (though the loop could be optimized by moving forward rather than back.)

I think there needs to be a public record of these discussions so that everyone can be involved. Email will just lock out other collaborators and will provide no history of the problem and proposed solutions. IRC is really out of the question, but I would be glad to Skype about this any time. I prefer to discuss issues like this by voice (saves a lot of time in my experience.) Once through talking, the issues resolved should be noted here.

As for bill's comment, I think he is suggesting that any change to Dojo is a patch, whether in a branch or a file, and must be tested by its author prior to being submitted.

I'm not submitting anything at the moment. I'm just trying to keep the trunk in sync, point out mistakes and improve understanding of critical issues (e.g. bad dates, memory leaks.) A bad patch is just creating future work. I can't imagine there is any hurry with these patches (i.e. nightly builds don't affect production.)

comment:36 Changed 10 years ago by Adam Peller

Milestone: 1.41.3.2

comment:37 Changed 8 years ago by bill

Component: DijitDijit - Form
Note: See TracTickets for help on using tickets.