WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 7 days ago

Last modified 5 days ago

#51130 closed enhancement (fixed)

Events displayed in venue timezone instead of user's

Reported by: iandunn Owned by: 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 6 weeks 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 6 weeks ago.
Shows UTC offset and timezone ID next to the event time
51130.diff (438 bytes) - added by SergeyBiryukov 7 days ago.

Download all attachments as: .zip

Change History (58)

#1 @Rarst
2 months 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
2 months 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
2 months 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
2 months 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.


6 weeks ago

  • 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

#6 @prbot
6 weeks ago

ocean90 commented on PR #535:

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

#7 @prbot
6 weeks ago

iandunn commented on PR #535:

Ah, I misread the handbook, thanks.

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


6 weeks ago

[...] 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
6 weeks 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
6 weeks ago

Shows UTC offset next to the city name

@iandunn
6 weeks ago

Shows UTC offset and timezone ID next to the event time

#10 follow-up: @iandunn
6 weeks 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
6 weeks ago

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

#12 @audrasjb
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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
6 weeks 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.

#19 @prbot
5 weeks ago

iandunn commented on PR #539:

@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
4 weeks 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

#21 @prbot
3 weeks ago

iandunn commented on PR #539:

The test failure is unrelated, see #core51446.

#22 @prbot
3 weeks ago

iandunn commented on PR #539:

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.

#23 @prbot
3 weeks ago

Rarst commented on PR #539:

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

#24 @prbot
3 weeks ago

iandunn commented on PR #539:

👍🏻 , thanks.

#25 @prbot
3 weeks ago

iandunn commented on PR #539:

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.

#26 @prbot
3 weeks ago

iandunn commented on PR #539:

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

#27 @prbot
3 weeks ago

iandunn commented on PR #539:

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.

#28 @prbot
3 weeks ago

iandunn commented on PR #539:

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

#29 @prbot
3 weeks ago

iandunn commented on PR #539:

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

#30 @prbot
3 weeks ago

ocean90 commented on PR #539:

@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.

#31 @prbot
3 weeks ago

iandunn commented on PR #539:

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

#32 @prbot
2 weeks ago

iandunn commented on PR #539:

Awesome, thanks!

#33 @iandunn
2 weeks ago

  • Milestone changed from 5.5.2 to 5.6

#34 @iandunn
2 weeks 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
2 weeks 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
2 weeks 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
2 weeks 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
2 weeks 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
2 weeks ago

Great! 👍

#40 @iandunn
2 weeks 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.

#41 @prbot
2 weeks ago

iandunn commented on PR #539:

Merged in r49145, r49146, r49147.

#42 @iandunn
2 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#43 @SergeyBiryukov
12 days 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.


10 days ago

#45 @tellyworth
9 days 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.


9 days ago

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


9 days ago

#48 @iandunn
8 days 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.


8 days ago

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.


8 days ago

#51 @SergeyBiryukov
8 days 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
7 days ago

#52 @SergeyBiryukov
7 days 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.

#53 @prbot
7 days ago

SergeyBiryukov commented on PR #651:

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
7 days 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.

#55 @prbot
5 days ago

iandunn commented on PR #651:

Ah, that makes sense, thanks!

Note: See TracTickets for help on using tickets.