Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#51130 closed enhancement (fixed)

Events displayed in venue timezone instead of user's

Reported by: iandunn's profile iandunn Owned by: iandunn's profile iandunn
Milestone: 5.5.2 Priority: high
Severity: normal Version:
Component: Date/Time Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

#meta4480 describes how the Events API returns data as a "WP timestamp" (Unix timestamp + UTC offset), rather than a true Unix timestamp, which is always UTC.

Because of that, events are displayed in the venue's timezone, rather than the user's. That's usually accurate for in-person events, but usually inaccurate for online events, where attendees can be located in any timezone.

Once the API starts returning a proper Unix timestamp, Core should update the widget to display events in the current user's timezone.

cc @rarst, @dd32

Attachments (3)

Screen Shot 2020-09-19 at 7.47.52 AM.png (170.9 KB) - added by iandunn 4 years ago.
Shows UTC offset next to the city name
Screen Shot 2020-09-19 at 7.49.09 AM.png (171.9 KB) - added by iandunn 4 years ago.
Shows UTC offset and timezone ID next to the event time
51130.diff (438 bytes) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (58)

#1 @Rarst
4 years ago

  • Component changed from Administration to Date/Time

Per that ticket current API return is local event date and time without time zone, not WP timestamp.

#2 @iandunn
4 years ago

Ah, ok, I couldn't find much info on them beyond just "Unix timestamp plus UTC offset". That how we convert Meetup.com API timestamps are converted to local time. What's the distinction?

#3 @Rarst
4 years ago

No, I mean your understanding of WP timestamp is correct, but at the time I opened the meta issue timestamps weren't returned, the API return was "date": "2019-06-18 19:00:00". I don't know if that's still the state of it, assumed so.

#4 @iandunn
4 years ago

Ah, that's right. The patch on #meta4480 will add new fields, but hasn't been committed yet.

This ticket was mentioned in PR #535 on WordPress/wordpress-develop by iandunn.


4 years ago
#5

  • Keywords has-patch has-unit-tests added

[...] events are displayed in the venue's timezone, rather than the user's. That's usually accurate for in-person events, but usually inaccurate for online events, where attendees can be located in any timezone.

See https://core.trac.wordpress.org/ticket/51130
See https://meta.trac.wordpress.org/ticket/4480

ocean90 commented on PR #535:


4 years ago
#6

Please use your own fork for PRs because custom branches will get deleted due to the mirror syncing.

iandunn commented on PR #535:


4 years ago
#7

Ah, I misread the handbook, thanks.

This ticket was mentioned in PR #539 on WordPress/wordpress-develop by iandunn.


4 years ago
#8

[...] events are displayed in the venue's timezone, rather than the user's. That's usually accurate for in-person events, but usually inaccurate for online events, where attendees can be located in any timezone.

See https://core.trac.wordpress.org/ticket/51130
See https://meta.trac.wordpress.org/ticket/4480

#9 @hlashbrooke
4 years ago

This is an excellent idea - with all meetups being online, it makes a lot more sense for the time to show in the user's timezone. Could we indicate that at the top of the widget and state which timezone it is? So for me it would say "Times shown in local timezone (UTC+2)". Your patch may already do that, but I can't test locally at the moment.

@iandunn
4 years ago

Shows UTC offset next to the city name

@iandunn
4 years ago

Shows UTC offset and timezone ID next to the event time

#10 follow-up: @iandunn
4 years ago

That's an interesting idea 🤔

I'd thought about adding it next to the event time, but it felt too cluttered. Putting it next to the city time would mitigate that. I uploaded some screenshots above that show examples of those.

It also shows the difference between using the UTC offset, and the timezone abbreviation. I think I'm leaning towards the timezone abbreviation, since that's shorter, and would be more familiar for most people (I'm assuming).

If we go with the timezone abbreviation, then maybe it would be better to place it next to the event time? That'd be more directly connected to the time, and more likely to be seen.

There may be some technical challenges getting either of them, though, though. Date().getTimezoneOffset() and new Date().toLocaleTimeString('en-us',{timeZoneName:'short'}).split(' ')[2] have various issues with browser support. moment().tz("America/Los_Angeles").zoneAbbr() might not be worth the performance hit for the enqueue. I guess we could try the two native methods, and fallback to an empty string if neither are available?

#11 @iandunn
4 years ago

xref ticket:47798#comment:31 for discussion about internationalizing the start/end month in format_event_data_time().

#12 @audrasjb
4 years ago

As soon as they are not displayed and not passed to any hook, those strings don't need to be prepared for i18n.

#13 in reply to: ↑ 10 @sippis
4 years ago

This sounds like a great improvement! For example user in Stockholm does not have any events coming up in Sweden, but we are having a WordCamp Finland Online later this year. Sweden is UTC+2 and we are living UTC+3 in Finland, so user currently would see wrong time if it would be meetup instead of WordCamp. And our Helsinki group might have online meetup which would interest them. Stockholm and Helsinki are so close to each other, that events in one of those cities will most likely be visible in both cities at the moment at least.

Replying to iandunn:

I'd thought about adding it next to the event time, but it felt too cluttered. Putting it next to the city time would mitigate that. I uploaded some screenshots above that show examples of those.

I think it's enough to show the timezone next to the city. Putting it next to event time makes the view too cluttered indeed. Though in that case, the user might think the event times are still displayed in the venue/local time rather than in their own timezone and add more confusion.

Some visual indication that tells how times are shown, as @hlashbrooke suggested, would be really good addition.

It also shows the difference between using the UTC offset, and the timezone abbreviation. I think I'm leaning towards the timezone abbreviation, since that's shorter, and would be more familiar for most people (I'm assuming).

Personally I'm leaning towards showing UTC offset rather than timezone abbreviation. It really depends on the location of user to which they are used to. In my mind timezone abbreviations are something that is used in US and UK, but not in central Europe for example and I've grown to use UTC offset since the elementary school :D

#14 follow-up: @iandunn
4 years ago

In my mind timezone abbreviations are something that is used in US and UK, but not in central Europe for example

Ah, great point. Maybe we should use an I18n'd string to let translators pick which one, then? e.g.,

/* translators: Do not translate. Set either `abbreviation` or `offset`, in English. This determines if the Events Widget shows timezone abbreviations or offsets. Pick whichever is most common in your locale. */
if ( 'abbreviation' === __( 'events-widget-timezone-display' ) ) {
    // show the timezone abbreviation
} else {
    // show the UTC offset
}

#15 in reply to: ↑ 14 @sippis
4 years ago

Sounds a bit hacky, but it works! :D If translators do not translate or translate it incorrectly, using UTC offset as a fallback +1

Actually it could be usable if core would handle the same in bit more elegant way in future - but I guess that's not going to happen in timespan which this events widget change is wanted to go out.

#16 @iandunn
4 years ago

Yeah, I've seen Core do it in other places, but I agree it'd be nice if there were a more elegant way.

I don't think there's any way to get it via the locale/tz info available via JavaScript. There may be some other canonical source, though. I'll see what I can find.

Or we could just use the UTC offset for simplicity, if that's what's most common around the world.

#17 @iandunn
4 years ago

Er, nevermind, it does look like toLocaleTimeString() will do that. #pr539 is already using it, but w/ the locale hardcoded. I wanted to remove that anyway, though. Leaving that param empty should default to the user's (browser/BCP47) locale. As long as we can still parse it out reliably, that should work.

#18 @iandunn
4 years ago

I've updated the PR to show the localized abbreviation 👍🏻


Note to self:

To manually test abbreviations and timezones on OS X, open the browser with overridden environment variables:

TZ="Europe/Helsinki" open /Applications/FirefoxDeveloperEdition.app

To verify: console.log( Intl.DateTimeFormat().resolvedOptions().timeZone );

You don't need to change the OS/browser locale, because (I'm guessing) the timezone shortname is pulled from tzdata, rather than locale data.

iandunn commented on PR #539:


4 years ago
#19

@rarst, @ryelle, @dd32 - This is still a WIP, but it's roughly working for me. If you see any big problems with the general approach, please let me know.

#20 @iandunn
3 years ago

  • Milestone changed from Awaiting Review to 5.5.2
  • Owner set to iandunn
  • Priority changed from normal to high
  • Status changed from new to assigned

iandunn commented on PR #539:


3 years ago
#21

The test failure is unrelated, see #core51446.

iandunn commented on PR #539:


3 years ago
#22

I think this is ready for more eyes.

I requested review from Kelly and Dion. @Rarst, GitHub won't let me request it from you formally, but I'd love to hear any feedback you have as well.

Rarst commented on PR #539:


3 years ago
#23

Nothing jumps out at me in PHP, not really familiar with date stuff in JS so no opinion there. :)

iandunn commented on PR #539:


3 years ago
#24

👍🏻 , thanks.

iandunn commented on PR #539:


3 years ago
#25

Oh, it looks like the notStrictEqual( daylightSavingAbbreviation, standardAbbreviation ); assertion _is_ failing in Travis, unrelated to the CORS error.

That's odd, since it passes locally, and in a Win7/IE11 VM. Looking into it.

iandunn commented on PR #539:


3 years ago
#26

Ah, I think it's because Travis is running with a UTC timezone, so there is no difference. Stubbing it will probably fix.

iandunn commented on PR #539:


3 years ago
#27

I rewrote that test to stub the Date functions, but at that point it's not testing anything that the other tests aren't. So, I just removed it.

I think this is ready for review, despite the Travis errors. I'm still digging into those, but they appear to be unrelated. Possibly related to #core51466.

iandunn commented on PR #539:


3 years ago
#28

Made a few changes, and the Travis issues are resolved now.

iandunn commented on PR #539:


3 years ago
#29

Do you think any more changes are needed, @ocean90 ?

ocean90 commented on PR #539:


3 years ago
#30

@iandunn From an i18n perspective this is looking good. Created #51498 to use wp.i18n for the remaining strings which I will commit once this has landed.

I'd probably update the @since tags to use 5.6.0 since there doesn't seem to be a 5.5.2 in the near future.

iandunn commented on PR #539:


3 years ago
#31

I'd probably update the @since tags to use 5.6.0 since there doesn't seem to be a 5.5.2 in the near future.

👍🏻 updated in f48e101d9258bfc6e3fb86b637ef372122e92101

iandunn commented on PR #539:


3 years ago
#32

Awesome, thanks!

#33 @iandunn
3 years ago

  • Milestone changed from 5.5.2 to 5.6

#34 @iandunn
3 years ago

In 49145:

Community Events: Trim events by Unix timestamp for accuracy.

The date and end_date fields are WP timestamps representing the venue's local time. As of meta:changeset:10270 (#meta4480), new start_unix_timestamp and end_unix_timestamp values are available, providing a proper Unix timestamp in the UTC timezone. Using those is more precise, and removes the time window where the event has expired but still appears in the Events Widget.

To simplify the function, it now only accepts and returns the events themselves, rather than the entire response body.

See #51130
See #meta4480
Related: https://make.wordpress.org/core/2019/09/23/date-time-improvements-wp-5-3/

#35 @iandunn
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 49146:

Community Events: Display dates and times in the user's time zone.

Fixes #51130
Props sippis, hlashbrooke, audrasjb, Rarst, iandunn

#36 @audrasjb
3 years ago

@iandunn Hi! I saw you moved it to milestone 5.6. While we're preparing 5.5.2, I just wanted to add that if it's a high priority, I'm all for backporting this to 5.5.2 which will probably ship in two weeks or so :)

#37 @iandunn
3 years ago

In 49147:

Community Events: Apply coding standards.

The previous commits intentionally didn't include these, because it would have added an unreasonable amount of diff noise.

See #51130

#38 @iandunn
3 years ago

  • Milestone changed from 5.6 to 5.5.2

Ah, I didn't realize it was going to happen after all, thanks!

I'll update the @since tags and backport 👍🏻

#39 @audrasjb
3 years ago

Great! 👍

#40 @iandunn
3 years ago

In 49152:

Community Events: Update timezone-related @since tags to 5.5.2.

r49145 and r49146 were originally planned for 5.6 when they were committed, but are now planned for 5.5.2.

See #51130.

iandunn commented on PR #539:


3 years ago
#41

Merged in r49145, r49146, r49147.

#42 @iandunn
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#43 @SergeyBiryukov
3 years ago

In 49201:

Docs: Add a @deprecated note to WP_Community_Events::format_event_data_time().

Follow-up to [49146].

See #51130.

This ticket was mentioned in Slack in #core by iandunn. View the logs.


3 years ago

#45 @tellyworth
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 49275:

Community Events: Display dates and times in the user's time zone.

Fixes #51130
Merges [49145], [49146], [49147], [49152], and [49201] to the 5.5 branch.
Props sippis, hlashbrooke, audrasjb, Rarst, iandunn

This ticket was mentioned in Slack in #community-team by iandunn. View the logs.


3 years ago

This ticket was mentioned in Slack in #community-events by iandunn. View the logs.


3 years ago

#48 @iandunn
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Huh, some of the new QUnit tests are failing in the `5.5` branch, even though they pass in trunk.

>> dashboard > communityEvents.getFormattedDate - multiple day event should use corresponding format
>> Actual: "September 15–17, 2020"
>> Expected: "September 15–17, 2020"
>> at Object.<anonymous> (file:///home/travis/build/WordPress/wordpress-develop/tests/qunit/wp-admin/js/dashboard.js:106:12)

>> dashboard > communityEvents.getFormattedDate - multiple month event should use corresponding format
>> Actual: "September 15 – October 6, 2020"
>> Expected: "September 15 – October 6, 2020"
>> at Object.<anonymous> (file:///home/travis/build/WordPress/wordpress-develop/tests/qunit/wp-admin/js/dashboard.js:117:12)

I saw that happen sometimes when I was writing them, but couldn't reproduce it reliably, and thought copy/pasting might have fixed it. Apparently not :)

I'm gonna take another look.

This ticket was mentioned in PR #651 on WordPress/wordpress-develop by iandunn.


3 years ago
#49

En dashes (8211) and hyphens (45) are getting mixed up, causing some QUnit tests to fail. They all appear to be en dashes in the code itself, but there could be something in dateI18n / Moment that is converting to a hypen.

See 51130#comment:48

021e8ef demonstrates the problem, but isn't necessarily the best solution.

Trac ticket: https://core.trac.wordpress.org/ticket/51130

This ticket was mentioned in Slack in #core by iandunn. View the logs.


3 years ago

#51 @SergeyBiryukov
3 years ago

Looks like some encoding failure. It would be great to find the source, but if it turns out to be too hard to consistently reproduce, it would probably be easier to replace the character in the affected strings with the &#8211; entity:

  • %1$s %2$d&#8211;%3$d, %4$d
  • %1$s %2$d &#8211; %3$s %4$d, %5$d

That would make the strings less readable to translators, but should still work. I think I went with the character in [47124] for readability only.

@SergeyBiryukov
3 years ago

#52 @SergeyBiryukov
3 years ago

Actually, it looks like 51130.diff resolves this:
https://travis-ci.com/github/SergeyBiryukov/wordpress-develop/builds/192442752

<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />

This line already exists in trunk since [49101], but not in the 5.5 branch. That explains the difference.

SergeyBiryukov commented on PR #651:


3 years ago
#53

Commented on the ticket: https://core.trac.wordpress.org/ticket/51130#comment:52

Adding this to qunit/index.html appears to resolve the issue:

<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />

This line already exists in trunk since [49101], but not in the 5.5 branch. That explains the difference.

#54 @SergeyBiryukov
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 49307:

Tests: Declare UTF-8 encoding for QUnit test suite.

This was done for trunk in [49101] as part of other changes, and is now backported to the 5.5 branch.

Follow-up to [49275].

Props iandunn, SergeyBiryukov.
Fixes #51130.

iandunn commented on PR #651:


3 years ago
#55

Ah, that makes sense, thanks!

Note: See TracTickets for help on using tickets.