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] Digital Clock with DST time zone generates inaccurate timeOptions for the "fall back" and "spring forward" days #10783

Open
sbriceland opened this issue Oct 23, 2023 · 4 comments · May be fixed by #10793
Assignees
Labels
bug 🐛 Something doesn't work component: pickers This is the name of the generic UI component, not the React module! time-zone Issues about time zone management

Comments

@sbriceland
Copy link

sbriceland commented Oct 23, 2023

Steps to reproduce

Link to live example: https://codesandbox.io/s/pensive-boyd-n6kq3f?file=/src/demo.tsx

Steps:

  1. Open up "Fall-back DST Picker". Scroll through the list of available times
  • Additional Note: 01:00 AM and 01:30 PM appear twice. This is a much smaller issue, but would be nice to address as well.
  1. Open up "Spring-forward DST Picker". Scroll through the list of available times
  • Additional note: 02:00 AM & 02:30 AM do no appear. However, one could argue that this is 100% correct. Works for our scenario, but just wanted to point it out for others to make that decision.

Current behavior

When opening the "Fall-back" picker set to November 5th, 2023, the time options stop at 10:30 PM. 11:00 PM & 11:30PM are missing.

When opening the "Spring-forward" picker set to March 10th, 2024, the time options shows extra options for which are technically for the next day: 12:00 AM & 12:30 AM at the end of the time options list are both for November 11th.

Expected behavior

When opening the "Fall-back" picker set to November 5th, 2023, all time steps of the day should be available to select.

  • Nice to have: don't show duplicate time steps (IE: 01:00 AM & 01:30 AM` in demo)

When opening the "Spring-forward" picker set to November 5th, 2023, only time steps for the selected day should be available.

Context

The bug appears to be in the creation of timeOptions:

  const timeOptions = React.useMemo(() => {
    const startOfDay = utils.startOfDay(valueOrReferenceDate);
    return [
      startOfDay,
      ...Array.from({ length: Math.ceil((24 * 60) / timeStep) - 1 }, (_, index) =>
        utils.addMinutes(startOfDay, timeStep * (index + 1)),
      ),
    ];
  }, [valueOrReferenceDate, timeStep, utils]);

Not completely related and

Your environment

npx @mui/envinfo
Please see the codesandbox demo. Bug does not appear to be attributed to any browser.

Search keywords: DigitalClock timezone DST daylight luxon

@sbriceland sbriceland added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 23, 2023
@sbriceland sbriceland changed the title Digital Clock with DST time zone generates inaccurate timeOptions for the "fall back" and "spring forward" days [pickers] Digital Clock with DST time zone generates inaccurate timeOptions for the "fall back" and "spring forward" days Oct 23, 2023
@LukasTy
Copy link
Member

LukasTy commented Oct 24, 2023

Thank you for creating this issue! 🙏
I can confirm that the problem is evident, it's also an issue with moment-timezone.
However, dayjs adapter seems to be working correctly as seen in this demo.
Could you confirm if you must use Luxon in your application?
I'll try investigating the issue in more depth to understand if it could be some issue on our end, or if is it a problem on both Luxon and Moment adapters. 🤷

@LukasTy LukasTy added status: waiting for author Issue with insufficient information component: pickers This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 24, 2023
@LukasTy
Copy link
Member

LukasTy commented Oct 24, 2023

Confirming that this seems to be the expected behavior for Luxon.
It's a known limitation that is documented.
Here are some relevant issues: moment/luxon#1089, moment/luxon#1278.

I've found a workaround for DigitalClock, which should avoid the problem of DST reliance.
The fix is somewhat logical because both MultiSectionDigitalClock and TimeClock do not have such a problem, because of how they are implemented. 👍

@LukasTy LukasTy added bug 🐛 Something doesn't work time-zone Issues about time zone management and removed status: waiting for author Issue with insufficient information labels Oct 24, 2023
@LukasTy LukasTy linked a pull request Oct 24, 2023 that will close this issue
@sbriceland
Copy link
Author

@LukasTy thank you so much for the response and taking the time to whip up such a nice demo of the adapters!

However, I still believe the issue is not specific to any of the Adapters. From your demo, the bug is still evident on both "Fall-back" and "Spring-forward" pickers for Dayjs.

Fall Back ends at 22:30:

Screenshot 2023-10-24 at 9 04 05 AM
Screenshot 2023-10-24 at 9 04 28 AM

Spring Forward includes 00:00 & 00:30 for the next day:

Screenshot 2023-10-24 at 9 04 47 AM

The Problem (I think)

The Code in DigitalClock that creates the timeOptions (similar to your demo TimeIn30Increments) expects there to be 24 hours in every day. This is correct for all dates in a year, except when dates use a timezone that respects Daylight Savings. When a DST timezone is used, the following two days of the year do not have 24 hours:

  • The date in which clocks "fall back", one hour is added to the day. Therefore, the Fall Back Date respecting DST actually has 25 hours. This year that happens on November 5th (every year the exact date is different).
  • The date in which clocks "spring forward", one hour is removed from the day. Therefore, the Spring Forward Date respecting DST actually has 23 hours. Next year that happens on March 10th (every year the exact date is different).

My belief that this is the problem is from how I see timeOptions being used in DigitalClock. Here's my brief understanding:

  • timeOptions is an array of Adapter-Specific Dates
  • The timeOptions array is created with the startOfDay (midnight AM of the valueOrReferenceDate) as the first item in the array
  • Then for every time step minus 1 (since startOfDay is already in the array) in a 24 hour day, a new date is added to the array.
  • The important part likely causing the bug is 24 * 60.
  • So from what I can see here, timeOptions will always be a list of every timeStep starting from startOfDay to 24 hours later
  • Later in the component, timeOptions (AKA "24 hours of time steps") is used in a map to render each time option.

Possible solution

Rather than using 24 as the "golden rule" for how many hours there are in a day, we could try something like the following:

const timeOptions = React.useMemo(() => {
  let timeStepOptions = [];
  const startOfDay = utils.startOfDay(valueOrReferenceDate);
  let nextTimeStepOption = startOfDay;
  while (utils.isSameDay(valueOrReferenceDate, nextTimeStepOption)) {
    timeStepOptions.push(nextTimeStepOption);
    nextTimeStepOption = utils.addMinutes(nextTimeStepOption, timeStep);
  }
  return timeStepOptions;
}, [valueOrReferenceDate, timeStep, utils]);

I have added the following updates to your awesome Demo to reflect this:

Screenshot of updated Demo:
Screenshot 2023-10-24 at 11 32 27 AM

Note that the pickers themselves still show the bug (because the bug originates inside of Digital Clock). The displayed "TimeIn30Increments" does now show the expected times for each adapter.

Also, apologies for my primitive code! I'm sure there is a more optimal and concise way to do that. I mainly wanted to illustrate an option.

@flaviendelangle
Copy link
Member

@LukasTy I added it to the board
If you think it's a non trivial topic to close, I would be in favor of estimating it and prioritizing it quickly given how old it is 👍

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: pickers This is the name of the generic UI component, not the React module! time-zone Issues about time zone management
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants