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

Scheduled Post time has mismatched between time shown and hint #25256

Closed
mkaz opened this issue Sep 11, 2020 · 9 comments
Closed

Scheduled Post time has mismatched between time shown and hint #25256

mkaz opened this issue Sep 11, 2020 · 9 comments
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@mkaz
Copy link
Member

mkaz commented Sep 11, 2020

Describe the bug

When displaying the time in the calendar, a hint was added in #23400 so the user would know what timezone the post was being schedule in.

The issue is it now appears that the time being selected is the user's time and not the server time.

To reproduce
Steps to reproduce the behavior:

  1. Set the site time to a different timezone, this example site set to EDT
  2. Create a new post and schedule the post time
  3. Notice the hint shown is EDT, but the time picked is in PDT

Expected behavior

The time shown should match the hint of what time zone it is being set to.

Screenshots

date-time

Related trac ticket: https://core.trac.wordpress.org/ticket/50624

@mkaz mkaz mentioned this issue Sep 11, 2020
6 tasks
@mkaz mkaz added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Sep 11, 2020
@vcanales
Copy link
Member

I think the problem comes from the fact that the date given to the frontend by the site is already offset, but the frontend formatting functions works assuming that this is not the case.

In order to print "September 20, 2020 5:00pm", dateI18n is called, which takes the dateValue (in this case, what the date picker shows—02:00pm) and applies the timezone offset in the frontend.

Something else to note: the difference between EDT and PDT is 3 hours; that is the offset that the library is applying to the date, and not necessarily the actual PDT offset of UTC-7—if it were a matter of the time being picked in another timezone, it would look like.

PDT (UTC-7) EDT (UTC-4) UTC
11:00am 02:00pm 06:00pm

In my system, the offset is applied based on my timezone (GMT-3) vs the timezone of the site—for instance, GMT-5 would render displayed dates like with an offset of -2 hours:

Kapture 2020-09-15 at 19 10 35

This is because of how moment works and how the dateI18n function is implemented based on it: the local time of the user is what will be taken into consideration when performing the offset—the function assumes that the date being passed to it is in the user's timezone and applies the system timezone offset to it.

There currently isn't an option for dateI18n to not perform any transformations at all, so I think another way would be to transform the time we get from the server into UTC so that the timezone applied by dateI18n ends up being the correct one. I'm not sure I like the idea of transforming the date in order to bring it back too much. I've tested adding a "do not perform timezone operations" to dateI18n and it works well, I like that approach best.

Any thoughts?

@vcanales
Copy link
Member

Another thought that comes to mind: the real base of the problem is that the site is providing the frontend with an already formatted time, which makes it harder to manipulate, as the new variable of 'timezone' is introduced. It would perhaps be best if the system reported all times as UTC.

If we take a look at the tests on @wordpress/dates for instance, they all rely on UTC times being provided—ie. they're all suffixed with Z.

@vcanales
Copy link
Member

This turned out to be a deeper issue that it seems, in my opinion.

When handling dates on the frontend, the multiple possible sources of truth must be kept in mind in order to avoid inconsistencies with the “actual” dates. The WordPress entity’s “date” and “date_gmt” attributes should be considered the source of truth when handling WordPress entities.

The happy path.

The user’s Browser timezone is UTC-3. The user sets the site’s time to UTC-3 as well. The user creates a post; the following timestamps are set in the corresponding attributes. date is the site’s local time, date_gmt its UTC(GMT/Zulu Time) equivalent.

date date_gmt
2020-09-23T17:18:00 2020-09-23T20:18:00

Gutenberg’s dateI18n function is called, in order to display the post’s formatted date on the editor—the date parameter is used

> dateI18n('F j, Y g:i a', '2020-09-23T17:18:00')
< September 23, 2020 17:18 pm

This scenario is well tested, and it works correctly!

The not so happy path

The user’s Browser timezone is still UTC-3, but they’ve set the site’s time to UTC-5. At 5:18pm UTC-5, the user creates a post; the following timestamps are set:

date date_gmt
2020-09-23T17:18:00 2020-09-22T22:18:00

The formatting function is called to display the post’s formatted date:

> dateI18n('F j, Y g:i a', '2020-09-23T17:18:00')
< September 23, 2020 3:18 pm

Here we can see that 3:18pm is neither UTC-5 nor -3 for the actual UTC time of
2020-09-22T22:18:00, but rather UTC-7. The formatting function is transforming the given time to UTC—ie. 20:18pm—based on the Browser Settings , and then setting the timezone. 20:18-05:00 = 15:18 or 3:18pm.

Same would happen if we used date_gmt instead, which is actually being used in the ‘latest posts’ and ‘latest comments’ blocks, but is not apparent on first sight as those blocks are only displaying the date but not the time. This creates problems anyway because day changes would be offset by whatever difference in times there is.

A combination of date_gmt + the gmt parameter on the dateI18n function doesn’t work either, because the timestamp is ambiguous ie. doesn’t have timezone information. Moment’s .utc() method, which is invoked when the third parameter on dateI18n is set to true, and in turn calling gmdateI18n, transforms the given date to UTC in this case, again applying the Browsers timezone to it.

Possible Solutions

  1. Adding a timezone or a Zulu Time suffix to the date_gmt timestamp renders the correct result:
> dateI18n( 'F j, Y g:i a', '2020-09-23T22:18:00Z' )
< September 23, 2020 5:18 pm

I think this would be ideally done in the backend.

  1. Patching it on the frontend, every time the function is called. It would mean using moment to let dateI18n know that the date is actually a UTC date...
const dateGMT = "2020-09-23T22:18:00";
const actualGMTDate = moment( dateGMT ).utc( true );

> dateI18n( actualGMTDate );
< September 23, 2020 5:18 pm

...or similarly doing the same when we save the data to the store.
    
  1. Changing the API by always assuming that the date is GMT when using gmdateI18n

ie. changing the API to

export function gmdateI18n( dateFormat, dateValue = new Date() ) {
/**
 * The only change in the next line, where the`true` parameter
 * is added to the `utc()` function.
 */
	const dateMoment = momentLib( dateValue ).utc( true ); 
	dateMoment.locale( settings.l10n.locale );
	return format( dateFormat, dateMoment );
}

This breaks the tests, so I'm hesistant on this being the best solution. The entire library is tested using timestamps that include the Z suffix.

Conclusion/Questions

Adding the Z suffix to the dates that WordPress provides to the editor requires next to no changes on the frontend, other than using the gmt parameter on dateI18n. The impact on the backed is unknown at this time.

This issue is affecting every usage of the dateI18n function that relies on the date provided by either the date or date_gmt parameter of the Post Entity, or any other ambiguous timestamp. Currently, besides the scheduler, that's also the following blocks:

Please let me know your thoughts. I'm particularly interested on the perspective of the non-Gutenberg side of this issue, as I haven't filled the knowledge gap there.

cc. @mkaz

@mkaz
Copy link
Member Author

mkaz commented Sep 21, 2020

Good work digging in and figuring out the scenarios.

I think I'm with you on the first solution — adding the Z would be explicitly setting what I think is assumed that the date is UTC. Also, we should be able to check the incoming date does not have any time zone indicator and only set it if it is missing.

This would be a good one to bring up in the Core JS Meeting, usually there is time in the meeting to bring up items, but might be good to add ahead to the meeting's rolling agenda to give people a chance to prep.

@vcanales
Copy link
Member

I brought this up on the meeting, and as suggested, I'd like to bring this to the attention of @aduth and @davilera—I don't have permissions to add you as reviewers, hence the ping :)

@tellthemachines
Copy link
Contributor

Is this a duplicate of #25003?

@vcanales
Copy link
Member

Is this a duplicate of #25003?

Yes, it’s the same problem.

@david-szabo97
Copy link
Member

This seems to be fixed by now. Not sure which commit though. Probably it's fixed by migrating from moment to date-fns. @mkaz Can you confirm? Thanks!

@mkaz
Copy link
Member Author

mkaz commented Dec 4, 2020

Yep, fixed in #25782

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants