-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DatePicker: Fix day's labels for shifting by 1 in specific usr and website timezone combination #67482
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
DatePicker: Fix day's labels for shifting by 1 in specific usr and website timezone combination #67482
Conversation
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @ThirdEyeRose. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @MaksVeter! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
…sVeter/gutenberg into fix/datepicket-day-labels-uk-dts-bug
|
@ntsekouras can you help resolve this here? |
|
@tyxla can you also help review this from a components point of view? Thanks for your first contribution @MaksVeter! Always a delight to se. |
| onKeyDown={ onKeyDown } | ||
| > | ||
| { dateI18n( 'j', day, -day.getTimezoneOffset() ) } | ||
| { dateI18n( 'j', shiftedDay, -shiftedDay.getTimezoneOffset() ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go with this approach we'd need to also update the aria-label above.
| }, [ isFocusable ] ); | ||
|
|
||
| const shiftedDay = new Date( day ); | ||
| shiftedDay.setHours( 15 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a bit more about this technique? Dates have many nuances and would like to understand more about it.
Would similar changes be needed elsewhere or this could resolve this issue for the whole component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm a bit torn about doing this without any conditions. Shouldn't we be doing it only if we can verify a DST discrepancy is occurring?
| }, [ isFocusable ] ); | ||
|
|
||
| const shiftedDay = new Date( day ); | ||
| shiftedDay.setHours( 15 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this sets the hours to 15:00:00, assuming that this will keep us in the same day, regardless of which timezone we are in (let me know if that's correct). However, I'm on the right track, this assumption seems flawed: it rests on the assumption that timezones won't spread more than 9 hours in the future or 15 hours in the past. However, AFAIK, timezones can range from UTC−12:00 to UTC+14:00, and that would contradict the initial assumption, and in some cases, this could still be a bug.
I'm also concerned this might create more problems, especially as we reuse time and date components and reuse them in more complex scenarios like DateRange picker (see #60971). The shifted date could then end up being used for time, which, as you can expect, would lead to unexpected results.
|
Maybe this is a naive question, but have we considered delegating some (most) of the logic directly to |
|
Has there been any further movement on this issue? We're still seeing this bug in Oct/Nov 2025, and it seems to impact more than just the date selected on the calendar. Looks like it may also impact day of the week order, moving from a Mon->Sun schedule to a Sun->Sat schedule. |
|
Closing in favor of #73444 |
What?
Fixes the days labels become wrong after the DST time when user is in UK and Website has US Timezone.
Why?
#67480
How?
Add 15 hours to day before pathing it to
Daycomponent to avoid the DST impact described in a bug (in the similar maner asdate-fnslib do)Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast