Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make LocalTZA take 't' and 'isUTC' and drop DSTA(t). #778

Merged
merged 7 commits into from Dec 29, 2017

Conversation

@jungshik
Copy link
Contributor

jungshik commented Jan 23, 2017

Currently, LocalTimezoneAdjustment is assumed to be constant across time
zone rule changes and does not take any argument. Daylight Savings Time
Adjustment does take an argument t.

DSTA(t) can 'absorb' the standard time zone offset changes
(e.g. Europe/Moscow changed the standard time zone offset multiple times
in the last decade.). However, the spec stipulates that it should only
reprsent an additional adjustment necessary when DST is in effect.

In practice, at least two implementations (Spidermonkey/Firefox and
JSC/Safari) do not follow the spec to the letter to produce results
expected by users across time zone rule changes.

This PR revises LocalTZA to accept two arguments, 't' and 'isUTC' and
drop DaylightSavingsTimeAdjustment(t). When isUTC is true, LocalTZA
interprets 't' as a time value representing UTC and gives a time zone
offset in effect at time t for the local time zone (for US ET, it'd be
-4*msPerHour in summer and -5*msPerHour in winter). When isUTC is false,
't' is interpreted as local time value and gives a time zone offset in
effect at t = t_local.

It's also specified that LocalTZA(t = t_local, false) will return the offset
before the transition when t_local is local time repeating multiple
times at a negative transition (e.g. fall backward) or skipped local time
at a positive time zone transition (e.g. spring forward).

Currently, implementations do not agree with each other as to how to handle repeating
or skipped local time. This change will standardize the behavior in line with the way Java 8 does.

Finally, UTC(t) and Localtime(t) are reformulated with LocalTZA(t = t_local, false) and
LocalTZA(t = t_UTC, true).

Will fix #725.

@jungshik
Copy link
Contributor Author

jungshik commented Jan 23, 2017

@bterlson, @ediosyncratic, @littledan : could you take a look? I like this better than #771.

Thanks.

@mj1856
Copy link

mj1856 commented Jan 24, 2017

I'm very glad that there is an attempt to normalize this in the spec. However I must say that the proposed standard is exactly backwards of what should be done (IMHO):

1:30 AM on November 5, 2017 in America/New_York is repeated twice (fall backward), but it must be interpreted as 1:30 AM UTC-05 instead of 1:30 AM UTC-04.

This makes little sense. 1:30 UTC-5 is indeed the "standard" time, but it is the second of the two occurrences. Time moves in a forward-only direction. A person watching their clock would not see the first 1:30, and say to themselves "oh wait, not this one, I think I'll wait a bit for another". They would act on the first occurrence - the "daylight" time occurrence. Then logically when the second one came around, they would not act again because they already had done so.

2:30 AM on March 12, 2017 in America/New_York does not exist, but it must be interpreted as 2:30 AM UTC-04.

This is also backwards. 2:30 UTC-4 does not exist in the US Eastern time zone. Through math, it would be normalized to 1:30 UTC-5. This is moving the wall time backward an hour. A sane person in the local time zone would not do this. They would not see that their regular time is 2:30 but remember to run an hour earlier because of the DST change they had not done yet! Instead, they would at 2:00 advance their clocks to 3:00, then they would do the scheduled activity either right then, or at 3:30. The latter is preferred, because if you had tasks at 2:05, 2:10, 2:15, 2:20, etc.. you would not do them ALL at the same time at 3:00. Another way to think about this is: if a local time is non-existent then the local clock did not move forward. Therefore, track the local time in place, and advance the clock forward by the amount of the gap.

To summarize, the text should be changed to:

1:30 AM on November 5, 2017 in America/New_York is repeated twice (fall backward), but it must be interpreted as 1:30 AM UTC-04 instead of 1:30 AM UTC-05.

and

2:30 AM on March 12, 2017 in America/New_York does not exist, but it must be interpreted as 3:30 AM UTC-4 (which is equivalent to 2:30 AM UTC-05).

These are the ONLY sensical options. Moving backwards is nonsensical. Time doesn't move backwards.

Note that this is exactly what Java 8 does in it's new time API, and is also what most production job scheduling systems will do. It's also what we do in moment.js (when we have control over it with moment-timezone), and it's what NodaTime is switching to in its upcoming 2.0 release.

I strongly suggest that ECMAScript follow suit.

Also - keep in mind that Australia/Lord_Howe only advances by 30 minutes - so don't assume the transition is always one hour.

@mj1856
Copy link

mj1856 commented Jan 24, 2017

FYI, I just checked in current versions of Chrome, IE, Edge, and Node.js, and they all follow my suggested implementation.

I also checked FireFox, and it follows the alternative implementation - the one that was originally suggested and that I am against.

I've checked this a few times over the years, and noticed that some browsers have flip-flopped on this point. It should indeed be standardized - but towards the way that Chrome, IE, Edge and Node.js are currently implementing it.

@maggiepint
Copy link
Member

maggiepint commented Jan 24, 2017

Agree with Matt here, but have to add a couple semantics. It is properly daylight saving time. There is no s. Also, I'm generally fine with the term "wall time", but that in and of itself should be defined and codified. When I give conference talks on this matter, I will frequently use the term "local time" to mean basically the same thing, and others still will use "civil time". It is worth noting that the Java 8 documentation refers to it as local time: https://docs.oracle.com/javase/8/docs/api/java/time/LocalDateTime.html

This is some of the best work in date/time out there, so their choices are good to model after.

@maggiepint
Copy link
Member

maggiepint commented Jan 24, 2017

One other thing of note - this is correct in not tracking a separate adjustment for daylight saving time. The 'normal' way (if there is one - it it as least the way I know and have seen) to convert between UTC and local time in a timezone is to use a table that contains transition points for a time zone in UTC, and the new offset at that point, to determine what the correct offset is for a given point on the global timeline. This system will work for both DST, and for changes to a country's standard offset, as happened in several places in Russia in 2016.

@jungshik
Copy link
Contributor Author

jungshik commented Jan 24, 2017

Thanks a lot for the review/feedback.

A couple of points to clarify:

  1. v8 is 'broken' across the timezone rule/offset changes (e.g. multiple times in the last decade in Europe/Moscow) because it follows the current spec to its letters.
    See https://bugs.chromium.org/p/v8/issues/detail?id=3547

  2. I didn't check v8's behavior when it works: that is, America/New_York. It's my mistake. Because of #1, I only checked JSC and Spidermonkey
    I didn't check Edge (I meant to), either. I should have done more thorough investigation of the behavior of various implementations.

  3. Having found that JSC and Spidermonkey behaved the way I spec'd (and the first point above about v8), I thought that it'd be better to make the default behavior what current implementations (that handles not just ordinary DST transitions but also zone offset transitions) do. However, again, I should have checked v8 and Edge as well.

With the tz set to America/New_York, Firefox does the following:

(new Date(2017,10,5,1,30)).toUTCString()
"Sun, 05 Nov 2017 06:30:00 GMT"                  :  1:30 AM is interpreted as UTC-05
(new Date(2017,2,12,2,30)).toUTCString()
"Sun, 12 Mar 2017 06:30:00 GMT"                  : 2:30 AM is interpreted as UTC-04

v8 does the opposite, indeed.

V8 version 5.7.0 (candidate)
d8> (new Date(2017,10,5,1,30)).toUTCString()
"Sun, 05 Nov 2017 05:30:00 GMT"
d8> (new Date(2017,2,12,2,30)).toUTCString()
"Sun, 12 Mar 2017 07:30:00 GMT"

Because of the 4th point below, I thought it'd be better to consider adding an option to control the behavior around time zone offset transition, but that is left for the future because it requires changing the public API signature.

  1. I'm not sure if we can say that one way to handle skipped and repeated wall time is better than others. For instance, ICU offers three different ways [1] and Python's proposed spec offers two options [2].

Anyway, I'm perfectly fine with the default behavior as suggested by @mj1856 because having read his argument, I'm kinda persuaded that the behavior he wants seems to make sense (from the pov of a person going through the transition looking at a clock) and I don't have any preference. My primary goal is to get rid of the ambiguity.

I'll revise the PR and ask you to take another look. Thanks again.

[1] ICU Calendar API has skipped wall time option (https://goo.gl/h0bP26) and
repeated wall time option (https://goo.gl/Q1VX3j).

[2] Python proposal to handle skipped/repeated time:
https://www.python.org/dev/peps/pep-0495/

@jungshik
Copy link
Contributor Author

jungshik commented Jan 24, 2017

@maggiepint

The 'normal' way (if there is one - it it as least the way I know and have seen) to convert between UTC and local time in a timezone is to use a table that contains transition points for a time zone in UTC, and the new offset at that point, to determine what the correct offset is for a given point on the global timeline. This system will work for both DST, and for changes to a country's standard offset, as happened in several places in Russia in 2016.

Sure. I wish I'd not have to worry about converting t_local (t value calculated from y,M,d, h, m, s in local time) to t_UTC. That is, I wish I'd not have to deal with LocalTZA(t, isUTC = false)). Unfortunately, the way Date constructor taking 'y,M,d,h,m,s' is spec'd (y,M,d,h.m,s are supposed to be interpreted in the local time), I cannot avoid it. See #725.

@mj1856
Copy link

mj1856 commented Jan 24, 2017

FWIW, most APIs I've seen in other languages just pick one or the other, and don't offer much control. Java 8, JodaTime, and NodaTime being the exception.

When I have had this discussion with library/platform authors 1 on 1, of those that chose the 1:30 UTC-5 option (in above context), they have often told me something like, "well, we weren't sure what to do, so we just went with the 'standard' time option". Of course, there are the arguments I made earlier, and also as Maggie pointed out with Russia - that not every transition is due to DST.

I've heard a lot of use cases around this, and scheduling a daily event seems to be the one that catches on it most frequently - and in that scenario there really is no option but to pick the first ambiguous instance and shift invalid times forward. That does push both of them into the "daylight" period, but so be it.

The only scenario I've ever personally encountered that might want the second of two instances is when a business such as a bar or movie theater might close at 1:30 AM, but for whatever reason wants to remain open until the second occurrence. Even then - they are already thinking about this edge case and they can adjust for it manually if needed (since both 1:30's are valid).

I've never encountered any use case where moving an invalid time from the spring-forward gap backwards an hour is desired. Not saying they don't exist - they're just not something I've seen.

@maggiepint
Copy link
Member

maggiepint commented Jan 24, 2017

I"m aware that the date constructor is specd for local time. I think I didn't phrase it well, but fundamentally all I am trying to convey is that the DST transition amount itself is temporal. Some countries do DST for a few years, then stop doing it, then start again. Lord Howe Island Australia does a 30 minute DST transition NOW, but I would be entirely unsurprised if they changed to one hour later. I'm not sure how this is handled right now, but I may not be reading close enough.

@jungshik jungshik force-pushed the jungshik:issue725alt branch from 765e39a to d5ed9f8 Jan 26, 2017
@jungshik
Copy link
Contributor Author

jungshik commented Jan 26, 2017

Addressed the review comments. Can you take another look, @mj1856 and @maggiepint ? Thanks !

@ediosyncratic
Copy link

ediosyncratic commented Jan 26, 2017

@mj1856: "I've never encountered any use case where moving an invalid time from the spring-forward gap backwards an hour is desired." Suppose you have a flight early the morning after a transition and a moderately long journey to get to the airport. Subtract travel time, needed to reach airport, from required arrival time. If that lands you in the missing hour, you need to move backwards a further hour to get when to set off in good time to catch your flight. Similar scenario with a fall-back can be a reason to want the second time round. (Where I live, in CEST, the missing-or-repeated hour is 2 to 3; I believe there are places where it's 3 to 4. Catching an early flight can indeed involve getting up that early.)

"The only scenario I've ever personally encountered that might want the second of two instances is when a business such as a bar or movie theater might close at 1:30 AM, but for whatever reason wants to remain open until the second occurrence. Even then - they are already thinking about this edge case and they can adjust for it manually if needed (since both 1:30's are valid)." They're going to be paying their staff for another hour's work (possibly at elevated rates; after all, it is Sunday) and they may well have to get local licensing magistrates (or similar authorities) to approve their staying open an hour longer than normal; yup, they're going to know they're doing this.

I'm not a fan of making an existing numeric global variable, LocalTZA, into a function taking two parameters. Surely that's going to require a lot of folk to change code; this cannot be good. Web pages updated to work with the new spec shall break for users of older browsers; web pages still compatible with old browsers shall break in newer browsers. The earlier spec, adding a new function that Does The Right Thing, looked better to me - albeit I never got round to reviewing it in detail (I've been busy, sorry). I'll give this one a look next.

Copy link

ediosyncratic left a comment

Changing the meaning of old names would break existing code; furthermore, names with "adjustment" in them, actually meaning "offset", are worth retiring in favour of better names.

spec.html Outdated
<p>An implementation of ECMAScript is expected to determine the local time zone adjustment with respect to UTC using the best available information on time zones.</p>
<p>When _isUTC_ is true, <dfn>LocalTZA( _t_, true )</dfn> is the offset of the local time zone from UTC measured in milliseconds at time represented by time value _t_ (UTC). When it is added to _t_ (UTC), it yields the local time.</p>
<p>When _isUTC_ is false, <dfn>LocalTZA( _t_, false )</dfn> is the offset of the local time zone from UTC measured in milliseconds at local time represented by time value <dfn><emu-eqn>_t_<sub>local</sub> = _t_</emu-eqn></dfn>. When it is subtracted from the local time <emu-eqn>_t_<sub>local</sub>, it yields the corresponding UTC.</p>
<p>When <emu-eqn>_t_<sub>local</sub></emu-eqn> represents local time repeating multiple times at a negative time zone transition (e.g. when the daylight saving time ends or the time zone adjustment is increased) or skipped local time at a positive time zone transitions (e.g. when the daylight saving time starts or the time zone adjustment is decreased), <emu-eqn>_t_<sub>local</sub></emu-eqn> must be interpreted with the time zone adjustment before the transition.</p>

This comment has been minimized.

Copy link
@ediosyncratic

ediosyncratic Jan 26, 2017

"must be interpreted with the time zone adjustment before the transition." So a spring forward's gap gets read in standard time, resulting in a UTC that corresponds to an actual local time an hour later than indicated; and a fall back's repeated hour gets read in daylight-saving time, which means using the first pass through the repeated hour.
As long as I've decoded that right, it sounds good :-)

spec.html Outdated
@@ -25936,44 +25936,44 @@

<!-- es6num="20.3.1.7" -->
<emu-clause id="sec-local-time-zone-adjustment">
<h1>Local Time Zone Adjustment</h1>
<p>An implementation of ECMAScript is expected to determine the local time zone adjustment. The local time zone adjustment is a value <dfn>LocalTZA</dfn> measured in milliseconds which when added to UTC represents the local <em>standard</em> time. Daylight saving time is <em>not</em> reflected by LocalTZA.</p>
<h1>LocalTimeZoneAdjustment ( _t_, _isUTC_ )</h1>

This comment has been minimized.

Copy link
@ediosyncratic

ediosyncratic Jan 26, 2017

You've changed the section heading into a function name, but then talked about a function with a different name, LocalTZA(,); I suspect some confusion.

Also: I'd generally replace "adjustment" with "offset" - we're talking about local time's offset relative to UTC.
A time-zone adjustment is a transition, be it DST-related or one-off, at which this offset changes.
We do indeed add or subtract this offset to "adjust" a time between UTC and local time, but using "adjust" in this context is a source of potential confusion, as it may mean either kind of adjustment.

This comment has been minimized.

Copy link
@jungshik

jungshik Jan 26, 2017

Author Contributor

I do prefer 'offset' to 'adjustment', too. That's what I did in my earlier PR. However, the sign of Date.prototype.getTimezoneOffset is exactly the opposite of what's more common. That is, getTimeZoneOffset() returns a negative value for east of the primary meridian and a positive value for west of the primary meridian.

And, I want 'offset' to have the same sign in both places, but I find it rather confusing when I use a 'positive' offset change for fall backward or a 'negative' offset change for spring forward. It's still a possibility, but I chose otherwise by sticking to the existing term, 'Local Timezone Adjustment'.

This comment has been minimized.

Copy link
@jungshik

jungshik Jan 26, 2017

Author Contributor

Changing the meaning of old names would break existing code; furthermore, names with "adjustment" in them, actually meaning "offset", are worth retiring in favour of better names.

Well, LocalTZA is not a part of the public API. It's an internal abstract operation. Moreover, the existing code needs to be changed anyway. Spidermonkey and JSC did the exact opposite of what's specified here around a timezone offset change. v8 is broken in timezones where historic offset is different from the present tz offset, etc.

spec.html Outdated
<h1>LocalTimeZoneAdjustment ( _t_, _isUTC_ )</h1>
<p>An implementation of ECMAScript is expected to determine the local time zone adjustment with respect to UTC using the best available information on time zones.</p>
<p>When _isUTC_ is true, <dfn>LocalTZA( _t_, true )</dfn> is the offset of the local time zone from UTC measured in milliseconds at time represented by time value _t_ (UTC). When it is added to _t_ (UTC), it yields the local time.</p>
<p>When _isUTC_ is false, <dfn>LocalTZA( _t_, false )</dfn> is the offset of the local time zone from UTC measured in milliseconds at local time represented by time value <dfn><emu-eqn>_t_<sub>local</sub> = _t_</emu-eqn></dfn>. When it is subtracted from the local time <emu-eqn>_t_<sub>local</sub>, it yields the corresponding UTC.</p>

This comment has been minimized.

Copy link
@ediosyncratic

ediosyncratic Jan 26, 2017

If taking a "time" parameter that's a local time, there should be a boolean flag to disambiguate transitions - both how to sanitize an invalid time and which valid time to use when local time represents two identically. That's only used for three hours of each year, so it's more of a hint than anything else (we'll ignore it otherwise).

Your earlier idea of separate methods then gives: TZOffsetUTC(t) and TZOffsetLocal(t, hint) as the methods to use. The former is entirely sufficient to implement the latter [*]. We can also implement the old LocalTZA and DaylightSavingTA() in terms of it; take the min of TZOffsetUTC(now +i*year/3) for i in (-2, -1, 0, 1, 2) as LocalTZA and redefine DaylightSavingTA(t) = TZOffsetUTC(t) - LocalTZA.. The old API uses "adjustment", the new uses the more apt "offset" to signal the improvement.

[*] See lines 412--444 of src/corelib/tools/qtimezoneprivate.cpp in https://codereview.qt-project.org#/c/162414/26//ALL,unified along with documentation above it.

This comment has been minimized.

Copy link
@jungshik

jungshik Jan 26, 2017

Author Contributor

Adding a option to control the way to handle repeated and skipped time is beyond this PR. Note that my first commit message leaves it up to a consideration in the future, but I removed that responding to the review comment.

Anyway, in the future, it can be still considered.

This PR wants to minimize the change and get rid of the existing ambiguity by specifying the default behavior. Adding a option in the public API (note that this PR does not change the signature of any public API) is not in the scope of this PR.

That's only used for three hours of each year, so it's more of a hint than anything else (we'll ignore it otherwise).

Is it true that it's only used for three hours of each year. As you noted and I also noted before, there are other cases where this issue of repeated or skipped time is important (namely, timezone rules changes as happened to Europe/Moscow, Pacific/Apia, etc.

spec.html Outdated
<emu-note>
<p>It is recommended that implementations use the time zone information of the IANA Time Zone Database <a href="http://www.iana.org/time-zones/">http://www.iana.org/time-zones/</a>.</p>
<p>1:30 AM on November 5, 2017 in America/New_York is repeated twice (fall backward), but it must be interpreted as 1:30 AM UTC-04 instead of 1:30 AM UTC-05. LocalTZA(TimeClip(MakeDate(MakeDay(2017, 10, 5), MakeTime(1, 30, 0, 0))), false) is <emu-eqn>-4 &times; msPerHour</emu-eqn>.</p>
<p>2:30 AM on March 12, 2017 in America/New_York does not exist, but it must be interpreted as 2:30 AM UTC-05. LocalTZA(TimeClip(MakeDate(MakeDay(2017, 2, 12), MakeTime(2, 30, 0, 0))), false) is <emu-eqn>-5 &times; msPerHour</emu-eqn>.</p>.
</emu-note>

This comment has been minimized.

Copy link
@ediosyncratic

ediosyncratic Jan 26, 2017

This </emu-note> seems to be missing its opening

This comment has been minimized.

Copy link
@jungshik

jungshik Jan 26, 2017

Author Contributor

Thanks. I fixed in my 3rd commit.

@maggiepint
Copy link
Member

maggiepint commented Jan 26, 2017

@jungshik
Copy link
Contributor Author

jungshik commented Jan 26, 2017

Surely that's going to require a lot of folk to change code; this cannot be good. Web pages updated to work with the new spec shall break for users of older browsers; web pages still compatible with old browsers shall break in newer browsers. The earlier spec, adding a new function that Does The Right Thing, looked better to me - albeit I never got round to reviewing it in detail (I've been busy, sorry). I'll give this one a look next.

I'm afraid that you overlooked the fact that LocalTZA is NOT a public API. It's an internal abstract operation. So, Ecma262 implementors need to change their codes (they need to anyway because the current spec has an ambiguity and this PR gets rid of that ambiguity), but web page authors does not in general. Well, their workaround (if there's been any) or reliance on the existing implementations need to adjusted.

Given the fact that (according to @mj1856 ) existing implementations changed the behavior over the time and different implementations have different behaviors, I don't think this change is any worse than status quo or alternatives in terms of web compatibility.

@jungshik
Copy link
Contributor Author

jungshik commented Jan 26, 2017

Offset is the preferred terminology from the Moment.js team - for what it's
worth. I know that it's favored in other parts of the DateTime community as
well.

See my comment
#778 (comment) in response to @ediosyncratic. I do prefer offset to adjustment, too, but decided to keep LocalTZA as a path of the least change. Anyway, I'm open to a suggestion regarding the sign of 'offset' here and that of getTimeZoneOffset(). I'm sure we cannot change the latter - getTimeZoneOffset()).

@jungshik
Copy link
Contributor Author

jungshik commented Jan 26, 2017

Let me give more details on the awkwardness (at least as perceived by me) when we use "offset' and get its sign to match that of getTimeZoneOffset() (public API).

The current PR has this (I spotted a mistake in 'increase' and 'decrease' in the previous commit and I fixed it in the latest commit. Thank you ! ).

When <emu-eqn>_t_<sub>local</sub></emu-eqn> represents local time repeating multiple times at a
 negative time zone transition (e.g. when the daylight saving time ends or the time zone adjustment is
 decreased) or skipped local time at a positive time zone transitions (e.g. when the daylight saving time
 starts or the time zone adjustment is increased),

With the sign of offset' to match that of getTimeZoneOffset() (public API), I'd have

 When <emu-eqn>_t_<sub>local</sub></emu-eqn> represents local time repeating multiple times at a
 positive time zone transition (e.g. when the daylight saving time ends or the time zone offset is increased)
 or skipped local time at a negative time zone transitions (e.g. when the daylight saving time starts or the
 time zone offset is decreased),

And, accordingly 'subtracted from' and 'added to' have to be swapped, which is fine.

Hmm... having written it down, it does not seem that bad except that 'DST ends ==> tz offset increase' and 'DST starts ==> tz offset decrease' seems still a bit odd to me.

Anyway, let's figure out which is the best for this term given all things I mentioned so far.

@mj1856
Copy link

mj1856 commented Jan 26, 2017

WRT sign (+/-): Yes, the getTimezoneOffset function is opposite of what most people expect, but that's only because we are now used to working with the ISO8601 standard and its derivatives, which place positive offsets East of GMT.

Quick history lesson: The getTimezoneOffset function, like much of the Date object, was copied from java.util.Date from Java 1.0 - which follows the POSIX standard with regard to time zones and their offsets. If you've seen a POSIX time zone string, like PST8PDT or CET-1CEST,M3.5.0/2,M10.5.0/3 - the first number (8 or -1 in these examples) is the standard-time offset, in hours West of GMT.

Anyway that absolutely cannot be changed in the existing API. It would break all kinds of things.

@mj1856
Copy link

mj1856 commented Jan 26, 2017

@ediosyncratic - Thanks for the feedback - That's a good scenario! I agree that in this case, one would want the 1:30 before the spring-forward transition, and they would also want the second 2:30 in the fall-back transition. However - we are not landing on these times by means of addressing a local time value without offset. We're landing on them through timeline math, as we do the subtraction operation. We know exactly where we're starting on the timeline, and we know the rules for the time zone, so we know exactly where we'll end up.

Using Europe/Oslo as an example. Let's say the flight leaves at 4:30 AM, and we have to depart two hours early to get their on time. In the spring, the local time from 2 AM to 3 AM is skipped.

2017-03-26T04:30:00+02:00  // our flight time
2017-03-26T02:30:00Z       // equivalent UTC
2017-03-26T00:30:00Z       // subtract two hours
2017-03-26T01:30:00+01:00  // equivalent local time

We can demonstrate this directly with the Date object, if our computer's time zone is set for Norway. This works in both Chrome and FireFox - which we've established are currently doing opposite adjustment behaviors:

var d = new Date(2017, 2, 26, 4, 30);
d.setUTCHours(d.getUTCHours() - 2);
console.log(d.toString()); // "Sun Mar 26 2017 01:30:00 GMT+0100 (Central Europe Standard Time)"

Same in the fall, when the local time from 2 AM to 3 AM is repeated.

2017-10-29T04:30:00+01:00  // our flight time
2017-10-29T03:30:00Z       // equivalent UTC
2017-10-29T01:30:00Z       // subtract two hours
2017-10-29T02:30:00+01:00  // equivalent local time

We land at 2:30 CET - the second occurrence. Again, to demonstrate:

var d = new Date(2017, 9, 29, 4, 30);
d.setUTCHours(d.getUTCHours() - 2);
console.log(d.toString()); // "Sun Oct 29 2017 02:30:00 GMT+0100 (Central Europe Standard Time)"

Of course the mistake that is commonly made is that users use getHours and setHours instead of the getUTCHours and setUTCHours functions. The latter are absolutely required when moving on the timeline by a duration (aka "time math" or "timeline math"). Users don't get this easily, which is one reason why we have libraries like Moment.js, and why so many folks (including @maggiepint and myself) think JavaScript needs a new datetime API.

So - long story - but there is no problem with the scenario you describe. It doesn't invoke the condition where we we have to decide which way to adjust. That only happens when a user gives us a input in terms of local time, and that input is ambiguous or invalid. Then we go down the path with the arguments I made earlier.

Cheers. 😄

@mj1856
mj1856 approved these changes Jan 26, 2017
Copy link

mj1856 left a comment

I reviewed the changes, and they look good to me.

spec.html Outdated
<emu-note>
<p>It is recommended that implementations use the time zone information of the IANA Time Zone Database <a href="http://www.iana.org/time-zones/">http://www.iana.org/time-zones/</a>.</p>
<p>1:30 AM on November 5, 2017 in America/New_York is repeated twice (fall backward), but it must be interpreted as 1:30 AM UTC-04 instead of 1:30 AM UTC-05. LocalTZA(TimeClip(MakeDate(MakeDay(2017, 10, 5), MakeTime(1, 30, 0, 0))), false) is <emu-eqn>-4 &times; msPerHour</emu-eqn>.</p>
<p>2:30 AM on March 12, 2017 in America/New_York does not exist, but it must be interpreted as 2:30 AM UTC-05. LocalTZA(TimeClip(MakeDate(MakeDay(2017, 2, 12), MakeTime(2, 30, 0, 0))), false) is <emu-eqn>-5 &times; msPerHour</emu-eqn>.</p>.

This comment has been minimized.

Copy link
@mj1856

mj1856 Jan 26, 2017

It may be useful to call out here that since the result 2:30 AM UTC-5 doesn't exist, that through normalization, the user will view this as 3:30 AM UTC-4, since they both have the same equivalency 7:30 AM UTC.

This comment has been minimized.

Copy link
@jungshik

jungshik Jan 27, 2017

Author Contributor

Thank you for the review/LGTM. I added '(equivalent to 3:30 AM UTC-04)'. Would it suffice?

@mj1856
mj1856 approved these changes Jan 27, 2017
spec.html Outdated
@@ -25944,7 +25944,7 @@
<emu-note>
<p>It is recommended that implementations use the time zone information of the IANA Time Zone Database <a href="http://www.iana.org/time-zones/">http://www.iana.org/time-zones/</a>.</p>
<p>1:30 AM on November 5, 2017 in America/New_York is repeated twice (fall backward), but it must be interpreted as 1:30 AM UTC-04 instead of 1:30 AM UTC-05. LocalTZA(TimeClip(MakeDate(MakeDay(2017, 10, 5), MakeTime(1, 30, 0, 0))), false) is <emu-eqn>-4 &times; msPerHour</emu-eqn>.</p>
<p>2:30 AM on March 12, 2017 in America/New_York does not exist, but it must be interpreted as 2:30 AM UTC-05. LocalTZA(TimeClip(MakeDate(MakeDay(2017, 2, 12), MakeTime(2, 30, 0, 0))), false) is <emu-eqn>-5 &times; msPerHour</emu-eqn>.</p>.
<p>2:30 AM on March 12, 2017 in America/New_York does not exist, but it must be interpreted as 2:30 AM UTC-05 (equivalent to 3:30 AM UTC-04). LocalTZA(TimeClip(MakeDate(MakeDay(2017, 2, 12), MakeTime(2, 30, 0, 0))), false) is <emu-eqn>-5 &times; msPerHour</emu-eqn>.</p>.

This comment has been minimized.

Copy link
@mj1856

mj1856 Jan 27, 2017

Looks good. Thanks.

This comment has been minimized.

Copy link
@jungshik

jungshik Jan 28, 2017

Author Contributor

Thanks a lot for the review and approval !

This comment has been minimized.

Copy link
@jungshik

jungshik Jan 31, 2017

Author Contributor

I wonder what comes next.

@jungshik jungshik force-pushed the jungshik:issue725alt branch from 1e10e77 to d1861cc Feb 6, 2017
@jungshik
Copy link
Contributor Author

jungshik commented Feb 6, 2017

I rebased the PR to the trunk to resolve a conflict.
@bterlson, what'd be next step for this PR?

@bterlson
Copy link
Member

bterlson commented Feb 7, 2017

We'll have to discuss in committee. Any volunteers to bring it up? Otherwise I can.

@maggiepint
Copy link
Member

maggiepint commented Feb 7, 2017

I'll do it if that would be easier for you, and I'm actually there.

@jungshik
Copy link
Contributor Author

jungshik commented Feb 7, 2017

Thank you, @bterlson and @maggiepint !

@jungshik jungshik force-pushed the jungshik:issue725alt branch from d1861cc to 3781832 Feb 7, 2017
jungshik added 6 commits Jan 20, 2017
Currently, LocalTimezoneAdjustment is assumed to be constant across time
zone rule changes and does not take any argument. Daylight Savings Time
Adjustment does take an argument t.

DSTA(t) can 'absorb' the standard time zone offset changes
(e.g. Europe/Moscow changed the standard time zone offset multiple times
in the last decade.). However, the spec stipulates that it should only
reprsent an additional adjustment necessary when DST is in effect.

In practice, at least two implementations (Spidermonkey/Firefox and
JSC/Safari) do not follow the spec to the letter to produce results
expected by users across time zone rule changes.

This PR revises LocalTZA to accept two arguments, 't' and 'isUTC' and
drop DaylightSavingsTimeAdjustment(t). When isUTC is true, LocalTZA
interprets 't' as a time value representing UTC and gives a time zone
offset in effect at time t for the local time zone (for US ET, it'd be
4*msPerHour in summer and 5*msPerHour in winter). When isUTC is false,
't' is interpreted as local time value and gives a time zone offset in
effect at t = t_local.

It's also specified that LocalTZA(t = t_local, false) will return the offset
*before* the transition when t_local is wall time repeating multiple
times at a negative transition (e.g. fall backward) or skipped wall time
at a positive time zone transition (e.g. spring forward). This is to
get rid of an ambiguity in handling a time zone offset transition. Due
to the ambiguity, different implemenations have different behaviors and
some implementations have changed their behavior over the time.

UTC(t) and Localtime(t) are reformulated with LocalTZA(t = t_local, false) and
LocalTZA(t = t_UTC, true).

Will fix #725.

In the future, it might as well be considered to add an option to specify the
behavior to handle skipped or repeated wall time. See  [1] and [2].

[1] ICU Calendar API has skipped wall time option (https://goo.gl/h0bP26) and
repeated wall time option (https://goo.gl/Q1VX3j).

[2] Python proposal to handle skipped/repeated time:
    https://www.python.org/dev/peps/pep-0495/
1. Interpret skipped and repeating local time in the time zone offset
before the transition.

2. s/wall time/local time/

3. Daylight savings time => Daylight saving time.
@jungshik jungshik force-pushed the jungshik:issue725alt branch from df671f2 to 49a970a Nov 11, 2017
@jungshik
Copy link
Contributor Author

jungshik commented Nov 11, 2017

Please, take one more look and merge now that I updated the PR to reflect @allenwb's comment. Thank you !

@jungshik
Copy link
Contributor Author

jungshik commented Dec 22, 2017

What should/can I do to get this merged?

@littledan
Copy link
Member

littledan commented Dec 23, 2017

@bterlson does this seem good to merge to you?

@bterlson
Copy link
Member

bterlson commented Dec 28, 2017

I'm happy to merge this now, but is there still no tests? I'm worried we may never see them :)

@bterlson
Copy link
Member

bterlson commented Dec 28, 2017

I also note some must/should confusion in LocalTimeZoneAdjustment now that I'm reading this again. Can we remove should from its specification text via "LocalTZA should return" --> "LocalTZA returns"? I can just make this change myself in order to get this in asap :)

@littledan
Copy link
Member

littledan commented Dec 28, 2017

@allenwb explained the rationale for "should" in this comment. I explained why it would be hard to write tests in this comment. In test262, we've seen a bunch of tests get filled in for areas that had gaps for a while, so I don't think that concern necessarily true (even if I'm a big fan of blocking on tests for most things).

@bterlson
Copy link
Member

bterlson commented Dec 29, 2017

@littledan must vs should confusion referred to discrepancies between the note saying must and the normative text saying should, but I see now that the note is only talking about the final must requirement not the previous should requirements. It's good, in other words, thanks :)

@bterlson bterlson merged commit 7e94e28 into tc39:master Dec 29, 2017
@jungshik
Copy link
Contributor Author

jungshik commented Jan 5, 2018

Thanks everybody !

kisg pushed a commit to paul99/v8mips that referenced this pull request Mar 7, 2018
icu-timezone-data was enabled before but reverted due to a perf issue.
(sunspider/date-format-totfe regressed; crbug.com/769706 ).

However, my in-Chrome test of the same test [1] shows that there's virtually
no perf difference. See https://goo.gl/GX1jt6 .

This will introduce a new behavior on POSIX(-like) platforms. Timezone
names inside parentheses after GMT offset will not be 3-4 letter
abbreviation any longer. They'll be human-readable names in the current
default locale. This matches the current Windows behavior.

new Date(2017, 5, 22).toString()
new Date(2017, 11, 22).toString()

Current:

Thu Jun 22 2017 00:00:00 GMT-0700 (PDT)
Fri Dec 22 2017 00:00:00 GMT-0800 (PST)

New:

Thu Jun 22 2017 00:00:00 GMT-0700 (Pacific Daylight Time)
Fri Dec 22 2017 00:00:00 GMT-0800 (Pacific Standard Time)

This CL will be followed by
  https://chromium-review.googlesource.com/c/v8/v8/+/572148 to
implement tc39/ecma262#778 .

[1] http://jungshik.github.io/v8/cr769706.html

BUG=v8:6031, v8:2137, v8:6076, chromium:769706
TEST=mjsunit/icu-date-lord-howe.js, mjsunit/icu-date-to-string.js

Change-Id: I22203670c3307a57fbf99e5f0a271dcbfbbef8fd
Reviewed-on: https://chromium-review.googlesource.com/857333
Commit-Queue: Jungshik Shin <jshin@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51791}
kisg pushed a commit to paul99/v8mips that referenced this pull request Apr 3, 2018
tc39/ecma262#778 was recently merged
to Ecma 262.

It changes the way to convert between "local time" and UTC in such
a way that it'd work for all timezones whether or not there has
been any change in the timezone offset of the standard time. For
instance, Europe/Moscow and some parts of US state of Indiana have
changed the standard (non-DST) timezone offset a few times. The
previous spec assumes that the the standard timezone offset is
constant, but the new spec take into account the offset change
history.

In addition, it specifies a new way to calculate the timezone
offset during a timezone transition (either in and
out of DST or timezone offset shift).

During a negative transition (e.g.  fall backward / getting
out of DST), repeated times are to be interpreted as if the
offset before the transition is in effect.

During a positive transition (e.g. spring forward / getting
into DST), skipped times are to be treated similarly. That
is, they are to be interpreted as if the offset before the
transition is in effect.

With icu-timezone-data, v8 is compliant to the new spec for the
past and the future as well as now whether or not the standard
timezone offset of a given timezone has changed over time
(e.g. Europe/Moscow, Pacific/Apia). With icu-timezone-data,
Australia/Lord_Howe (30 minute DST change) also works per spec.

Without icu-timezone-data, it works only for timezones of which
the standard timezone offset is the same as the current offset
(e.g. most North American timezones other than parts of Indiana)
and of which the DST shift is an hour. For instance, it doesn't work
for Europe/Moscow in 2010 when the standard timezone offset was
+4h because the current (2018) standard timezone offset is +3h. Neither
does it for Lord Howe in Australia with the DST shift of 0.5 hr.

This CL used to require one of the two ICU CLs below, but not
any more.

  https://chromium-review.googlesource.com/c/chromium/deps/icu/+/572652
  https://chromium-review.googlesource.com/851265  (a proposed CL to the
  upstream ICU).

Bug: v8:3547,chromium:417640,v8:5714
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Change-Id: Ib162295da5bee31b2390bd0918157014aebd3e33
Reviewed-on: https://chromium-review.googlesource.com/572148
Commit-Queue: Jungshik Shin <jshin@chromium.org>
Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52332}
kisg pushed a commit to paul99/v8mips that referenced this pull request Apr 3, 2018
This reverts commit dbdede0.

Reason for revert: Fails webkit_tests, blocks roll: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064

Original change's description:
> Implement a new spec for timezone offset calculation
> 
> tc39/ecma262#778 was recently merged
> to Ecma 262.
> 
> It changes the way to convert between "local time" and UTC in such
> a way that it'd work for all timezones whether or not there has
> been any change in the timezone offset of the standard time. For
> instance, Europe/Moscow and some parts of US state of Indiana have
> changed the standard (non-DST) timezone offset a few times. The
> previous spec assumes that the the standard timezone offset is
> constant, but the new spec take into account the offset change
> history.
> 
> In addition, it specifies a new way to calculate the timezone
> offset during a timezone transition (either in and
> out of DST or timezone offset shift).
> 
> During a negative transition (e.g.  fall backward / getting
> out of DST), repeated times are to be interpreted as if the
> offset before the transition is in effect.
> 
> During a positive transition (e.g. spring forward / getting
> into DST), skipped times are to be treated similarly. That
> is, they are to be interpreted as if the offset before the
> transition is in effect.
> 
> With icu-timezone-data, v8 is compliant to the new spec for the
> past and the future as well as now whether or not the standard
> timezone offset of a given timezone has changed over time
> (e.g. Europe/Moscow, Pacific/Apia). With icu-timezone-data,
> Australia/Lord_Howe (30 minute DST change) also works per spec.
> 
> Without icu-timezone-data, it works only for timezones of which
> the standard timezone offset is the same as the current offset
> (e.g. most North American timezones other than parts of Indiana)
> and of which the DST shift is an hour. For instance, it doesn't work
> for Europe/Moscow in 2010 when the standard timezone offset was
> +4h because the current (2018) standard timezone offset is +3h. Neither
> does it for Lord Howe in Australia with the DST shift of 0.5 hr.
> 
> This CL used to require one of the two ICU CLs below, but not
> any more.
> 
>   https://chromium-review.googlesource.com/c/chromium/deps/icu/+/572652
>   https://chromium-review.googlesource.com/851265  (a proposed CL to the
>   upstream ICU).
> 
> Bug: v8:3547,chromium:417640,v8:5714
> Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
> Change-Id: Ib162295da5bee31b2390bd0918157014aebd3e33
> Reviewed-on: https://chromium-review.googlesource.com/572148
> Commit-Queue: Jungshik Shin <jshin@chromium.org>
> Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#52332}

TBR=adamk@chromium.org,littledan@chromium.org,mlippautz@chromium.org,jshin@chromium.org

Change-Id: I6b3bf4427c761b106280d565a3912cd8e25cf87e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:3547, chromium:417640, v8:5714
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/994192
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52338}
kisg pushed a commit to paul99/v8mips that referenced this pull request Apr 4, 2018
This is a reland of dbdede0
after a webkit layout test (geolocation-api/timestamp.html) was
fixed by
https://chromium-review.googlesource.com/c/chromium/src/+/994343 .

Original change's description:
> Implement a new spec for timezone offset calculation
>
> tc39/ecma262#778 was recently merged
> to Ecma 262.
>
> It changes the way to convert between "local time" and UTC in such
> a way that it'd work for all timezones whether or not there has
> been any change in the timezone offset of the standard time. For
> instance, Europe/Moscow and some parts of US state of Indiana have
> changed the standard (non-DST) timezone offset a few times. The
> previous spec assumes that the the standard timezone offset is
> constant, but the new spec take into account the offset change
> history.
>
> In addition, it specifies a new way to calculate the timezone
> offset during a timezone transition (either in and
> out of DST or timezone offset shift).
>
> During a negative transition (e.g.  fall backward / getting
> out of DST), repeated times are to be interpreted as if the
> offset before the transition is in effect.
>
> During a positive transition (e.g. spring forward / getting
> into DST), skipped times are to be treated similarly. That
> is, they are to be interpreted as if the offset before the
> transition is in effect.
>
> With icu-timezone-data, v8 is compliant to the new spec for the
> past and the future as well as now whether or not the standard
> timezone offset of a given timezone has changed over time
> (e.g. Europe/Moscow, Pacific/Apia). With icu-timezone-data,
> Australia/Lord_Howe (30 minute DST change) also works per spec.
>
> Without icu-timezone-data, it works only for timezones of which
> the standard timezone offset is the same as the current offset
> (e.g. most North American timezones other than parts of Indiana)
> and of which the DST shift is an hour. For instance, it doesn't work
> for Europe/Moscow in 2010 when the standard timezone offset was
> +4h because the current (2018) standard timezone offset is +3h. Neither
> does it for Lord Howe in Australia with the DST shift of 0.5 hr.
>
> This CL used to require one of the two ICU CLs below, but not
> any more.
>
>   https://chromium-review.googlesource.com/c/chromium/deps/icu/+/572652
>   https://chromium-review.googlesource.com/851265  (a proposed CL to the
>   upstream ICU).
>
> Bug: v8:3547,chromium:417640,v8:5714
> Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng
> Change-Id: Ib162295da5bee31b2390bd0918157014aebd3e33
> Reviewed-on: https://chromium-review.googlesource.com/572148
> Commit-Queue: Jungshik Shin <jshin@chromium.org>
> Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#52332}

Bug: v8:3547, chromium:417640, v8:5714
Change-Id: I47536c111143f75e3cfeecf5d9761c43a98a10f5
Cq-Include-Trybots: luci.v8.try:v8_linux_noi18n_rel_ng;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/995971
Commit-Queue: Jungshik Shin <jshin@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52372}
@fabriciomurta
Copy link

fabriciomurta commented Jun 21, 2018

Issues are raising as it became released on chrome 67. It is changing seconds in the time zones and does not get carried when converted to-and-back UTC.
https://stackoverflow.com/questions/50857187/weird-seconds-offset-in-js-date-object-in-chrome

@jungshik
Copy link
Contributor Author

jungshik commented Jun 24, 2018

does not get carried when converted to-and-back UTC

Not sure what you meant by that. Chrome 67 takes into account the historical time zone offset changes over time as recorded in the IANA time zone database. As explained by one of answers with heading 'Not a bug', some time zones underwent offset/DST changes multiple times even after 2010 (e.g. Europe/Moscow). In case of Sao Paolo, see https://www.timeanddate.com/time/zone/brazil/sao-paulo (select 1900-1924). It switched to UTC-3 from LMT in 1914.

@fabriciomurta
Copy link

fabriciomurta commented Jun 24, 2018

Yes, but if you read around the discussions you'll see the complaint is not really about the -3h or -3h06m (or other in other time zones), it is about a -3h06m28s or 54s.

This is the most likely change in chrome that has introduced the above issue. Converting to-from UTC also results in a different result, e.g. the time converted back from UTC won't match the base time in a local1->UTC->local2 conversion chain. local2 won't match local1 by this seconds difference.

@jungshik
Copy link
Contributor Author

jungshik commented Jun 25, 2018

local1->UTC->local2 conversion chain. local2 won't match local1 by this seconds difference

I'm not sure what you meant by the above:
( please, ignore 'Pacific Standard Time' inside parens. That's not correct and I'm aware of that).

> d2=new Date(1850,0,1)
Tue Jan 01 1850 00:00:00 GMT-0752 (Pacific Standard Time)
> d2.getUTCHours()
7
> d2.getUTCMinutes()
52
> d2.getUTCSeconds()
58     <===   it's not zero, but that does not get in the way of converting back to local time.
> d2.getUTCMilliseconds()
0
> d3=new Date(Date.UTC(d2.getUTCFullYear(), d2.getUTCMonth(), d2.getUTCDate(), 
d2.getUTCHours(), d2.getUTCMinutes(), d2.getUTCSeconds(), d2.getUTCMilliseconds()))
Tue Jan 01 1850 00:00:00 GMT-0752 (Pacific Standard Time)

> d2.getTime() == d3.getTime()
true
ibaoger pushed a commit to ibaoger/pdfium that referenced this pull request Aug 27, 2018
https://chromium.googlesource.com/v8/v8.git/+log/9cf8abb7ce7e..ff6b34b468c1

This updates V8 to 6.8.44.

There has been changes to how JS handles timezones. [1] Update the test
expectations to match the new behavior, even though it deviates from
Acrobat, which still has the old behavior.

BUG=pdfium:1075

[1] tc39/ecma262#778

Change-Id: I63f0df9cd423ceee5b8d1008ba12a47ca84bbd6d
Reviewed-on: https://pdfium-review.googlesource.com/41450
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.