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

Incorrect decoding of local dates in Safari. #53

Merged
merged 4 commits into from
Jul 22, 2019
Merged

Incorrect decoding of local dates in Safari. #53

merged 4 commits into from
Jul 22, 2019

Conversation

Fil
Copy link
Member

@Fil Fil commented Jul 21, 2019

instead of treating new Date("2018-04-25T11:00") as a local date, Safari parses it as a UTC date and returns it with the tz difference added. This fix detects the situation by checking TWO local dates (one in winter, one in summer) which should return hour 0, and if one of them is not 0 it's the Safari bug. In that case, and if the date string that is passed is local (no "Z"), we shift it accordingly.

I don't know how to write a node test for this, but the following notebook shows the fix on Safari:
https://observablehq.com/d/7e8b78e5050258ae

Fixes #45

instead of treating `new Date("2018-04-25T11:00")` as a local date, Safari parses it as a UTC date and returns it with the tz difference added. This fix detects the situation by checking TWO local dates (one in winter, one in summer) which should return hour 0, and if one of them is not 0 it's the Safari bug. In that case, and if the date string that is passed is local (no "Z"), we shift it accordingly.

I don't know how to write a node test for this, but the following notebook shows the fix on Safari:
https://observablehq.com/d/7e8b78e5050258ae

Fixes #45
@Fil Fil requested a review from mbostock July 21, 2019 14:05
@mbostock
Copy link
Member

Some suggestions:

  • Compute tzFix once on startup rather than every time autoType is invoked.

  • Store the local timezone offset string (e.g., -07:00) rather than a boolean, or null if the workaround is not needed.

  • Add the local timezone offset string to the input value if m[7] is falsey, prior to parsing the date, rather than parsing the input and then offsetting by milliseconds.

if (fixtz && !m[7]) value += fixtz;
value = new Date(value)

@mbostock
Copy link
Member

Oh, and most importantly: don’t add the local timezone offset if the specified input value doesn’t contain a time (T…, m[4] should be truthy, I believe).

if (fixtz && !!m[4] && !m[7]) value += fixtz;
value = new Date(value)

In other words, this should always be parsed as UTC (and should already work as-is in all browsers):

new Date("2018-04-25")

And this would give an invalid date:

new Date("2018-04-25-07:00")

Let’s make sure we have a test for that.

…ked.

Store the local timezone offset string (e.g., -07:00) rather than a boolean, or null if the workaround is not needed.

Add the local timezone offset string to the input value, when m[4] is truthy (there is a time) and m[7] is falsy (there is no TZ information).
@Fil
Copy link
Member Author

Fil commented Jul 21, 2019

I've implemented the suggested changes, but I realize the fix is wrong in winter for summer dates, and vice versa. (Because the offset depends on the daylight savings time at the date we read.) So, more work needed…

By checking on the day we still probably have the 2—3 hours of the early morning of the DST days wrong…?
@mbostock
Copy link
Member

Okay, well here’s potentially a simpler hack to force Safari to parse local dates. Change the format:

new Date("2019/09/02 11:00")

Something like value.replace(/-/g, "/").replace(/T/, " ") should work.

@Fil
Copy link
Member Author

Fil commented Jul 21, 2019

Much simpler! I've updated the test notebook. I still have no idea how to test for it in node (can we monkey patch the Date class?).

@Fil Fil merged commit 4972600 into master Jul 22, 2019
@Fil Fil deleted the safariLocalDate branch July 22, 2019 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Safari incorrectly defaults to UTC for date-time strings.
2 participants