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

[pickers][DateRangePicker] timezones handled incorrectly with dayjs when timezone extensions are enabled #13290

Closed
oskarkk opened this issue May 29, 2024 · 14 comments · Fixed by #13362
Assignees
Labels
bug 🐛 Something doesn't work component: date range picker This is the name of the generic UI component, not the React module! component: pickers This is the name of the generic UI component, not the React module!

Comments

@oskarkk
Copy link

oskarkk commented May 29, 2024

Steps to reproduce

Link to live example: https://codesandbox.io/p/sandbox/mui-daterangepicker-error-zphn44

Steps:

  1. In the date "04/29/2024" change "29" to e.g. "1" or "2" using keyboard (not clicking in calendar)
  2. Error: "MUI X: The timezone of the start and the end date should be the same."

Sometimes it works, maybe depending on the initial value.

Current behavior

The error appears only when using dayjs timezone extensions (dayjs.extend(timezone); dayjs.extend(utc);).

When I put a breakpoint on this line of code (rangeValueManager.getTimezone) I can see that one date has a timezone "system" and the other "Europe/Warsaw" (my system timezone), and that raises an exception.

Expected behavior

No response

Context

No response

Your environment

x-date-pickers-pro 7.5.1
material 5.15.14
(see codesandbox)

Search keywords: DateRangePicker timezone dayjs

@oskarkk oskarkk added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 29, 2024
@michelengelen michelengelen added component: pickers This is the name of the generic UI component, not the React module! component: date range picker This is the name of the generic UI component, not the React module! labels May 29, 2024
@michelengelen michelengelen changed the title [pickers] DateRangePicker handles timezones incorrectly with dayjs when timezone extensions are enabled [pickers][DateRangePicker] timezones handled incorrectly with dayjs when timezone extensions are enabled May 29, 2024
@michelengelen michelengelen added the bug 🐛 Something doesn't work label May 29, 2024
@michelengelen
Copy link
Member

Hey @oskarkk and thanks for raising this issue.
I can confirm this bug on my local instance.

Let me have a look at what might be going wrong here.

@michelengelen
Copy link
Member

Ok, this happens because the value will be updated with a new timezone on change.
@LukasTy I will have a deeper look tomorrow to fix this. 👍🏼

@michelengelen michelengelen self-assigned this May 29, 2024
@michelengelen
Copy link
Member

OK, I found the issue after a bit of a search around the valueManagement and state.
The problem is with this method:

getTimezone: (utils, value) => {
const timezoneStart =
value[0] == null || !utils.isValid(value[0]) ? null : utils.getTimezone(value[0]);
const timezoneEnd =
value[1] == null || !utils.isValid(value[1]) ? null : utils.getTimezone(value[1]);
if (timezoneStart != null && timezoneEnd != null && timezoneStart !== timezoneEnd) {
throw new Error('MUI X: The timezone of the start and the end date should be the same.');
}
return timezoneStart ?? timezoneEnd;
},

When we change one value it gets set to the local timezone (using value.local()) and with that it receives the new timezone. As I see it we have 2 options:

  1. allow 2 different timezones in the values
  2. when changing one value we need to update the timezone on the second value as well.

IMHO we should go for the 1. option, since it will allow for ranges that span multiple timezones. Not a very widely used case, but it can still happen (e.g. in planning software)

WDYT @mui/explore ?

@michelengelen michelengelen removed their assignment May 31, 2024
@michelengelen michelengelen removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 31, 2024
@LukasTy
Copy link
Member

LukasTy commented May 31, 2024

@oskarkk could you elaborate a bit more about your use case?
I see that you are using timezone and utc plugins, but the dayjs call to create days is the regular one, not dayjs.utc or dayjs.tz as documented here.

As I see it, everything is handled correctly, when the bound values are created with appropriate functions.
If you have a concrete example for a situation, that @michelengelen described, we can discuss supporting multiple timezones for values, but that's not a bug, but a feature request. 🤔

@LukasTy LukasTy added status: waiting for author Issue with insufficient information and removed bug 🐛 Something doesn't work labels May 31, 2024
@oskarkk
Copy link
Author

oskarkk commented May 31, 2024

I would say that the timezones are not different, they're the same timezone, as they are (in my case) "Europe/Warsaw" and "system", and my system timezone is "Europe/Warsaw".

My use case was that I had some fields that worked in user timezone, so I didn't have timezone and utc extensions for dayjs. Then I needed some additional fields that would be in a specific timezone, so I added timezone and utc extensions and made some datetime fields with timezone prop. Then the other fields without timezone prop started throwing errors.

I see that you are using timezone and utc plugins, but the dayjs call to create days is the regular one, not dayjs.utc or dayjs.tz as documented here.

You don't have to set any initial value to reproduce this issue, you can reproduce it with just this (this time with DateTimeRangePicker):

dayjs.extend(timezone);
dayjs.extend(utc);

export default function App() {
  return (
      <LocalizationProvider dateAdapter={AdapterDayjs}>
        <DateTimeRangePicker slots={{ field: SingleInputDateTimeRangeField }} />
      </LocalizationProvider>
  );
}

Somehow without default values the error show less frequently, I have to change the date more times before it breaks (changing month seems to fail faster). It seems to work without SingleInputDateTimeRangeField (but maybe it's even rarer then).

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels May 31, 2024
@michelengelen
Copy link
Member

@LukasTy on another note:
using it like this breaks it immediately as well

const [value, setValue] = useState<[Dayjs | null, Dayjs | null]>([null, null]);

it tries to get the timezone form value.$x?.timezone, but value is null in this case.
I changed the first value via keyboard and the second value is still null.

Maybe we need to somehow update both values at the same time when using the single input range field. 🤷🏼

@LukasTy
Copy link
Member

LukasTy commented May 31, 2024

You don't have to set any initial value to reproduce this issue, you can reproduce it with just this (this time with DateTimeRangePicker):

@oskarkk could you provide clear steps to reproduce the issue without having any value bound? 🤔
I tried various ways, but couldn't get it to throw an error... 🤷

The root cause of the problem is that we are trying to "guess" the timezone of the value, when a timezone plugin is present. It forces the whole application to behave in the same way, either working with or without timezone.

LukasTy on another note:
using it like this breaks it immediately as well

@michelengelen could provide step by step guide on how to cause the error given your example?
I tried it and couldn't reproduce the error. 🙈 🤷

@LukasTy LukasTy added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels May 31, 2024
@michelengelen
Copy link
Member

michelengelen commented May 31, 2024

@LukasTy using this code:

import * as React from 'react';
import { Box } from '@mui/material';
import {
  DateRangePicker,
  LocalizationProvider,
  SingleInputDateRangeField,
} from '@mui/x-date-pickers-pro';
import { AdapterDayjs } from '@mui/x-date-pickers-pro/AdapterDayjs';
import dayjs, { Dayjs } from 'dayjs';
import timezone from 'dayjs/plugin/timezone';
import utc from 'dayjs/plugin/utc';

dayjs.extend(timezone);
dayjs.extend(utc);

export default function App() {
  const [value, setValue] = React.useState<[Dayjs | null, Dayjs | null]>([null, null]);

  return (
    <Box justifyContent={'center'} sx={{ width: '100%' }}>
      <LocalizationProvider dateAdapter={AdapterDayjs}>
        <DateRangePicker
          value={value}
          onChange={setValue}
          slots={{ field: SingleInputDateRangeField }}
          sx={{ '& .MuiInputBase-root': { width: '250px' } }}
        />
      </LocalizationProvider>
    </Box>
  );
}

and then changing one of the values in the inputfield via keyboard will trigger that error.

Screen.Recording.2024-05-31.at.16.07.36.mov

@LukasTy
Copy link
Member

LukasTy commented May 31, 2024

@michelengelen I have tried the same code before writing a comment and it's strange that for me locally I do not get any error... 🤷 🙈

@michelengelen
Copy link
Member

thats strange ... let me fetch master and try again!

@michelengelen
Copy link
Member

My bad ... I had some logging in place that caused that error. Sry! 🙇🏼

@oskarkk
Copy link
Author

oskarkk commented May 31, 2024

You don't have to set any initial value to reproduce this issue, you can reproduce it with just this (this time with DateTimeRangePicker):

@oskarkk could you provide clear steps to reproduce the issue without having any value bound? 🤔 I tried various ways, but couldn't get it to throw an error... 🤷

@LukasTy
https://codesandbox.io/p/sandbox/mui-daterangepicker-error-forked-dg32xj
Click on first MM, select May 12 on calendar, select time 12 00 AM, select May 14 (still clicking calendar), then OK. Click on "05" in the first date and press down arrow on keyboard two times - error.

The root cause of the problem is that we are trying to "guess" the timezone of the value, when a timezone plugin is present. It forces the whole application to behave in the same way, either working with or without timezone.

Without timezone plugin the value doesn't have timezone, but will be presented in users timezone. So if I set value to dayjs("2024-05-31T00:00:00Z") I'll see 2024-05-31 02:00 on the page (the same with dayjs("2024-05-02T02:00:00+02:00")). The objects don't have timezone on their own and convert to UTC when using .toISOString() or .toJSON(), but with .format() you can get string with users timezone. Then after enabling plugins, I would expect this to stay the same, enabling a plugin for dayjs shouldn't change the behavior of the existing code. Plugins have these different types of objects (dayjs.utc and dayjs.tz), so I see no reason for change in the behavior of datetime fields with plain dayjs datetimes and no timezone prop.

Anyway, overall the field seems to work as I expect, but when entering by keyboard it somehow sets the timezone of one date to user's timezone (with continent/city, like Europe/Warsaw), and the other to "system", which I think is essentially the same, but maybe there are two places that set the timezone differently, but with the same intent of setting user's timezone - the same behavior as without plugins.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels May 31, 2024
@LukasTy LukasTy self-assigned this Jun 3, 2024
@LukasTy LukasTy removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 4, 2024
@LukasTy LukasTy added the bug 🐛 Something doesn't work label Jun 4, 2024
@LukasTy
Copy link
Member

LukasTy commented Jun 4, 2024

@oskarkk Thank you for providing a detailed guide to reproduce the problem. 🙏
I can confirm that this behavior does not seem appropriate.
The dayjs is a bit picky with timezone/utc management, because the setup is global, while moment and luxon provide a more elegant API, especially for cases like yours. 🙈

Copy link

github-actions bot commented Jun 4, 2024

⚠️ This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@oskarkk: How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: date range picker This is the name of the generic UI component, not the React module! component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants