Improve timezones selector with optgroups

As discussed (and worked on together with John Cook) here is a patch that will render the zimezones grouped in OPTGROUPs. This makes it much easier to work with a screenreader and has the beneficial side-effect of meaning we can finally just type the city name of the zimezone and have it automatically selected.

The screenshot shows how the zimezone regions are made into OPTGROUPs.

This is an amazing improvement. Would love to see this in core!
I found one bug, see screenshot (UTC).

Screenshot, not patch.

Yeah - I was waiting for someone spotting that ;-)

This should be a little better.

Forgot to set as Needs Review - again!

Given the patch a once-over and the code looks good, just two other issues.

1. +++ b/core/modules/system/src/Form/RegionalForm.php
   @@ -65,7 +65,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    
   
   +++ b/core/modules/system/system.module
   @@ -1313,8 +1313,10 @@ function system_mail($key, &$message, $params) {
   +function system_time_zones($blank = NULL, $grouped = FALSE) {
   

   According to api.drupal.org there are 8 call sites, of which 2 are changed in this patch. I think it would be good to comment the other sites that this new interface is available, so it is clearer in the patch and/or comment in the issue why the other call sites do not need change.

   See: https://api.drupal.org/api/drupal/core%21modules%21system%21system.modul...

2. +++ b/core/modules/system/system.module
   @@ -1327,6 +1329,26 @@ function system_time_zones($blank = NULL) {
   +  if ($grouped) {
   

   Is system_time_zones included in a unit test suite? It would be good to extend test coverage for the new functionality.

Thanks for the review.

1. I initially added the calls to the function for the specific use-cases of installing the site's region setting and then subsequently changing the configured region. As it happens, it seems that the same improvement also works in the other locations in drupal/core where this function is used, so I have enabled it there, too. I have made one exception, though, for \Drupal\user\Entity\User::getAllowedTimezones as it is just collecting array keys and we would actually be helping there. See new patch.
2. There doesn't appear to be test coverage on this bit (or, at least, no tests calling this function, anyway!). I'm inclined to feel that adding tests is a good thing to do but probably as an aside to this issue.

Thanks this is a great improvement, because now typing the name works, and the list is much easier to scan.

It's also a nice example on how an accessibility issue improves usability in general.

It works perfectly on the Regional settings page, however, the timezone is initially set during installation and that form throws a warning when this patch is applied:

Warning: asort() expects parameter 1 to be array, object given in system_time_zones() (line 1347 of core/modules/system/system.module).
system_time_zones(NULL, 1) (Line: 207)
Drupal\Core\Installer\Form\SiteConfigureForm->buildForm(Array, Object, Array)
call_user_func_array(Array, Array) (Line: 514)
Drupal\Core\Form\FormBuilder->retrieveForm('install_configure_form', Object) (Line: 271)
Drupal\Core\Form\FormBuilder->buildForm('Drupal\Core\Installer\Form\SiteConfigureForm', Object) (Line: 893)
install_get_form('Drupal\Core\Installer\Form\SiteConfigureForm', Array) (Line: 583)
install_run_task(Array, Array) (Line: 539)
install_run_tasks(Array) (Line: 116)
install_drupal(Object) (Line: 44)

hmm - that's interesting - I didn't see that.

Ah! The UTF8 is a single text entry, not an array, so we probably shouldn't sort it! Oops.

Awesome, that fixed it. RTBC.

I think it is time we had some Accessibility and Usability maintainers take a look at this. It seems, as far as actually doing what was requested, we are there.

Adding tags & additional tasks...

While following along, I noticed two small coding standard errors

1) missing param type
2) space between function call and brackets

otherwise looks good to me.

Tested patch with NVDA screen reader on windows; tab through to Default time zones - screen reader reads: "Default time zone box list collapsed Berlin"

Can either type name in directly; or hit enter and list expands. Time zone regions display grouped. Can type and name is selected. Letters typed are read out by screen reader and in most cases the city/region name is read out; not always though which is odd for example "Darwin" but assume this is screen reader as many others tested work fine and entire name of city/region was read out. Can then tab out of field as expected.

Below before patch:

Compare to before patch; No screenreader information read out when navigating options, no region grouping. Function not able to be completed with screenreader.

A seemingly small change that solves a usability and accessibility issue - great work.

tl;dr - I'm happy to sign this off for the accessibility gate :)

@JohnCook, @rachel_norfolk and myself spent quite a lot of time playing with this at the Sheffield (UK) meeting during Global Sprint Weekend.

I tested this with:

• Chrome browser + ChromeVox screen reader (on Kubuntu 14.10 Linux)
• Android v6.x + latest Chrome (with and without a bluetooth keyboard).
• Android v6.x + latest Chrome + Talkback screen reader (with and without a bluetooth keyboard).
• iOS v10.2 + Safari screen reader (with and without a bluetooth keyboard).
• iOS v10.2 + Safari + VoiceOver screen reader (with and without a bluetooth keyboard).

Plus @ok_lyndsey tested with Windows + NVDA + Chrome in comment #18.

Keyboard users benefit from the general usability win of being able to type the first 2-3 letters of a city name, without first having to quickly type the entire name of a continent (and a slash).

The most obvious change to the screen reader experience is that the continent names are not read out before every city. On the whole this feels like an improvement; you no longer have to listen to the entire phrase "America" before confirming that you have scrolled to "America/Los Angeles". This could make it drastically quicker for screen reader users who are going through all the options instead of typing the name.

Note that screen readers do not yet give any special treatment to HTML <optgroup> elements. If you scroll so that an optgroup is in focus, it will be announced, but AFAIK no current screen-reader lets you drill down by selecting an HTML optgroup first.

I can think of one potentially confusing scenario. Say a user has found Macau, but was looking for Madrid. These ought to be near each other alphabetically, but they are in separate optgroups. Europe and Asia are such large groups that it's possible to open the select widget and not see any optgroup names in the viewport. On the whole, I feel the benefits of typing ahead probably outweigh this scenario. It is mitigated in a few ways however:

• Briefly scrolling will soon reveal the optgroups visually.
• Screen readers typically announce the position and total number of options, so a user may figure out that they haven't reviewed the whole list yet.
• The up, down, page-up, page-down, home, and end keys allow granular levels of scrolling inside a select element.

The original list of timezones includes some three-level strings, such as "America/Argentina/Cordoba". This patch removes the middle level, and HTML optgroups only support 2-levels. So a user will not find the Argentinian cities grouped together. Since users will very likely to know the major city names in their own country, type ahead and alphabetical order will mitigate this for many users.

From an accessibility point of view, we need to ask:

• Does this introduce an accessibility regression? NO, I'm fairly confident it doesn't introduce any serious problems.
• Does it offer any new advantages? Not as such, because assistive tech doesn't make use of HTML optgroups. The general usability gains still apply: type-ahead city names, and no tedious repetition of continent names.

Removing the "needs accessibility review" tag. Please add it back if the approach changes, or we discover something new relating to accessibility.

minor issue summary updates, tick off some tasks.

Adding final screenshots to Issue Summary

bizarre that the screenshot was not appearing...

Updating issue title....

"Group timezones by region" - well, they already are. The "Europe/London" style does that. So how about "Improve timezones selector with optgroups."

Patch from comment #12. This is an API addition, hopefully non-breaking and backwards compatible. Adding it to the issue summary, tagging for approval.

  * @param $blank
  *   If evaluates true, prepend an empty time zone option to the array.
+ * @param $grouped
+ *   If evaluates true, groups the timezones regions together in array.
  */
-function system_time_zones($blank = NULL) {
+function system_time_zones($blank = NULL, $grouped = FALSE) {

No data model changes, updating summary.

Improving alt text for screenshots in issue summary

This looks like a great UX improvement. Good job, everyone.

Small nitpick:

+ * @param bool $grouped
+ *   (optional) If evaluates true, groups the timezones regions together in
+ *   array.

We never say "If evaluates true", we say "If TRUE". I suggest the following wording "(optional) Whether the timezones should be grouped by region."

I'm never afraid of some nitpicking - it's how I learn!!

Improvements like this should now be targeted against 8.4.x. Thanks!

Easy Timezone Select is a related D7 contrib moudle which does something similar.

People here may be interested in:
#2530306: America/Montreal and other linked timezones missing from Default time zone setting

Yes please, this looks like a no-brainer improvement. Being able to type the city name to find it is useful but especially the visual grouping makes the whole list a lot easer to read/scan. We tested this in a UX meeting and it works as intended. Nice one!

+++ b/core/modules/system/system.module
@@ -1313,8 +1313,10 @@ function system_mail($key, &$message, $params) {
  * @param $blank
  *   If evaluates true, prepend an empty time zone option to the array.
+ * @param $grouped
+ *   (optional) Whether the timezones should be grouped by region.

Nitpick: The types of arguments should be documented.

It's a good nitpick Gábor!

Question: Should I add the type to both arguments? The first one might be considered "out of scope" of the issue, given we have not changed it.

IMHO add to both. I reviewed the code otherwise and it look fine other than this nit.

Keeping progress up to date in Issue Summary...

Now - we need to approve a non-breaking API change. Hmm - how to do that...

+++ b/core/modules/system/system.module
@@ -1355,10 +1355,15 @@ function system_mail($key, &$message, $params) {
- * @param $blank
+ * @param mixed $blank
...
  */
-function system_time_zones($blank = NULL) {
+function system_time_zones($blank = NULL, $grouped = FALSE) {

I looked at whether bool|NULL would be the correct type here, but |NULL only seems to be used once in core, so probably not.

Otherwise looks good. IMHO a committer will need to decide on the backwards compatible API addition question.

The original list of timezones includes some three-level strings, such as "America/Argentina/Cordoba". This patch removes the middle level, and HTML optgroups only support 2-levels. So a user will not find the Argentinian cities grouped together. Since users will very likely to know the major city names in their own country, type ahead and alphabetical order will mitigate this for many users.

I think this is a slight regression if you want to select such a timezone. I attached some screenshots to make the difference clear.

Before the patch the three timezones from North Dakota are nicely grouped together.

With the patch they are appear along with the rest of the American time zones.

I think this could be fixed fairly simply by changing the

$split = explode ('/', $value);

line into

$split = explode ('/', $value, 2);

Marking back to "needs review" at least to see what others think. We could either ignore this issue altogether, change it now, or change it in a follow-up. I don't feel strongly either way, but wanted to bring it up, as I don't think it received much attention after @andrewmacpherson brought it up above.

Edit: Actually embed the correct images. (Also make them a appear a bit smaller, sorry for the size)

It is true that @andrewmacpherson raised the query about threee level timezones. We did discuss it but failed to document that discussion very well.

We felt that the threee level timezones were something of a historical quirk of their creation and in many ways were inconsistent with the rest of the timezones, both continent and world wide. People wanting to choose those timezones would be more than aware of their existence and capable of choosing from a list formatted as {continent}/{city}.

If there was a more widespread use of threee level timezones, I think the change would have been justified. I think, in this case, it would be fine to set back to RTBC as is.

Re #44: Well as I suggest we could also just keep the "/" separator for those timezones so I do think it's possible to solve this, but #43 is fine with me.

The three-part timezones have been nagging at my mind, so I did some appraisal of them.

1. Are there any duplicate city names being disambiguated this way? NO. (Good, because that would have been a blocker.)
2. How many 3-part names? 25 out of 423 total (roughly 1 in 17)
3. How many unique middle parts? 4 - Argentina, Indiana, Kentucky, North Dakota. One of these is a country, the others are US states. This means only 2 out of 196 actual countries (approx 1%) are are affected, and only 3 of 50 US states.

Re: #45

Well as I suggest we could also just keep the "/" separator for those timezones

If we kept the middle parts (e.g. "Argentina/Catamarca") those cities would be difficult to find by type-ahead. We'd be singling out a specific country (Argentina) for a poorer UX, which I don't think is very fair at all.

My method was to run this, via devel/php:

$zonelist = timezone_identifiers_list();
dpm($zonelist, '$zonelist');

// Are there any duplicate city names (i.e. final part).
// Expect same count as before.
$zonelist_parts = [];
foreach ($zonelist as $key => $zone) {
  $parts = explode('/', $zone);
  $city = end($parts);
  $zonelist_parts[$city] = $parts;
}
ksort($zonelist_parts);
dpm($zonelist_parts, '$zonelist_parts');

// How many 3-part names?
$zonelist_three_parts = array_filter($zonelist_parts, function($v, $k) {
  return count($v) == 3;
});
dpm($zonelist_three_parts, '$zonelist_three_parts');

// How many unique middle parts?
$unique_middle_parts = [];
foreach ($zonelist_three_parts as $zone) {
 $middle = $zone[1];
  $unique_middle_parts[$middle]++;
}
dpm($unique_middle_parts, '$unique_middle_parts');

Lets also add a graphical time-zone picker ?
1. I like the Ubuntu 14.04 time-zone setter. The location typing field loads beautifully and accurately
2. The map click also works great. Refer to the attached screen-shot
Also found the Jquery Timezone picker - http://timezonepicker.com/

Oops - forgot to attach the Ubutu 14.04 time-zone setting screen-shot

Also found relevant drupal.org projects:

https://www.drupal.org/project/timezone_picker
https://www.drupal.org/project/tzfield

That would need it's own issue @prabhu9484, this issue is about improving what is there now. Feel free to create a separate issue for creating a graphical time zone picker though!

Many thanks @yoroy - created https://www.drupal.org/node/2853386 - please review

So my first reaction on reading the title of this issue was "Bojhan says optgroups are not a good pattern" but I see this issue has usability review from the UX team and signoff from @yoroy, so looks like they were useful here. Thanks!

I am concerned about #42 though. If I want to select a timezone for North Dakota, I would look for North Dakota. Why don't we make it so that it shows "North Dakota/Beluah" instead, listed under the America optgroup?

@xjm - did you see the more detailed analysis done by @andrewmacpherson in #46?

The trouble is that North Dakota is an exception. By displaying it, we create something inconsistent. Same could be said for Argentina - different to rest of time zones in continent.

Unless you know that North Dakota is an exception, you wouldn't look for "North Dakota" but for Beluah because before you come to N there is no indication that this one US state is different then the rest of the countries in the list.

So I would agree with #46: We are now keeping a bad UI for the sake of a few exceptions.

Tried to review this, but latest patch doesn't apply.

I think the only proper thing to do is the split just on the first /, and display the remaining portion, eg Indiana/Indianapolis. zoneinfo is fluid. It would also cause ambiguity if we add in the backward data (the links) where there would be duplicates.

I can raise this issue on the TZ mailing list if desired, I don't really participate but I do read it.

Been following from the sidelines since I reviewed the patch. The list of names presented here is very clearly a list of towns/cities. I see no benefit whatsoever in retaining in this list a few places that include a state or country name. You might as well ask why France/Paris is not listed, or New York/New York!

I thought for a while that splitting on the first /, as @mpdonaldo suggested in #55, was a sane compromise, but I now feel that is not true: It merely muddies waters that are relatively clear until that point. And as also noted above (1), the additional level is apparently a deprecated feature of zoneinfo, so unlikely to be included in any new place, and (2) as @andrewmacpherson found in #46 there are no current duplicate places when the middle level is omitted.

@xjm makes a point about 'looking for a place from North Dakota'. Well, why can I not look up my timezone by looking for United Kingdom then? or @DamienMcKenna look up New Hampshire?

Think of it this way. If the base TZ list hadn't had these anomalies, would people be demanding the additional levels, for these particular places and no others? I would guess not.

The argument thus resolves around the possible desire to not trip up a few people who believe from some past experience that their place can be found via their region name and country name, rather than by their region and city name, despite everyone else not doing so. I do not believe that most people in those areas will think like this: they will look at the first items and deduce the correct behaviour from those.

Please, let us get this approved and committed. If we then get complaints we can perhaps review the decision in the light of that actual data.

The problem is that we are essentially mapping this select to the IANA list, not a list that we are picking.

France/Paris isn't there because the first portion of the identifier is Area (usually continent or ocean) not a country (because geographical lines can change), and the second is Location (usually a city, but sometimes a larger geographic area, typically the name of an island). Location can be compound. Compound locations are here to stay and part of the official database (since at least 2005); they are not deprecated.

Having something like tzselect would be *way* better, but that is not the scope of this issue (but see #2853386: Interactive time-zone picker).

I also think we are misinterpreting the fact that there aren't any duplicates. From the northamerica file

# From Paul Eggert (2007-08-17):
# ...
# Other than Indianapolis, the Indiana place names are so nondescript
# that they would be ambiguous if we left them at the 'America' level.
# So we reluctantly put them all in a subdirectory 'America/Indiana'.

That went into effect in the 2005.10(?) zoneinfo.

So, if we get rid of "Indiana", then we are potentially losing important information, information that the people on the tz mailing list thought was important enough to keep in the database. I looked into the discussion around this, and Paul Eggert told me:

I don't remember much public discussion. That was back in the good old days when we could just do things if they seemed right.

I am not down with the approach of throwing away the middle portion on three-level names. I am somewhat willing to dig in my heels on this...

Fortunately, I think there is an easy solve, to retain the information and to make city lookup easier.

When there is a three-level name, present it as

First Level
-- Third Level (Second Level).

So, 'America/Indiana/Indianapolis' would get rendered out as

<optgroup label="America">
<select value="America/Indiana/Indianapolis">Indianapolis (Indiana)</select>

City/location search would work by first character, and we keep the middle level for reference.

?

Hi, thanks for the detailed reply. Your suggested solution did occur to me as well, and I for one would be happy with it.

@mpdonadio - Thanks for the research.

When there is a three-level name, present it as

First Level
-- Third Level (Second Level).

Yes, this sounds like a good way to deal with these quirks.

# Other than Indianapolis, the Indiana place names are so nondescript
# that they would be ambiguous if we left them at the 'America' level.
# So we reluctantly put them all in a subdirectory 'America/Indiana'.

Oh wow, this is reassuring. I was worried we might be overlooking a critically important technical reason, but it looks like we can rearrange it for presentation.

Review of the patch in #59:

An interdiff wasn't provided, and I got errors when trying to run interdiff, so I manually compared the whole patch with the previous on from comment #38.

The changes which needed a re-roll are all to do with array() syntax which was changed to [].

The change which brings in the third-level presentation is in this part...

Patch from #38:

+      $city = array_pop($split);
+      $region = reset($split);
+      if (!empty($region)) {
+        $grouped_zones[$region][$key] = $city;
+      }

Patch from #59:

+      $city = array_pop($split);
+      $region = array_shift($split);
+      if (!empty($region)) {
+        $grouped_zones[$region][$key] = empty($split) ? $city : $city . ' (' . join('/', $split) . ')';
+      }

This works by taking the last part as the city, the first part as the region (optgroup), and includes all remaining parts in brackets after the city name. Bonus points for being robust enough to deal with any number of parts, just in case the tzdata ever gets some 4 level names :-)

Minor nitpick: join() is an alias of implode(), so we could change this to the preferred function name. Having said that, we are using a mixture of join and implode in core, and our coding standards don't mention it, so I suppose it isn't a blocker here.

Screenshot of the options after patch #59. Amazingly, you can arrange it so Argentine, Indiana, Kentucky, and North Dakota are all in view at once.

Thanks - that looks like a good solution.

Does this change need test coverage, or is there already a test that ensure that the select contains precisely the same list from timezone_identifiers_list()?

ack system_time_zones reveals there are is no test coverage prior to this patch.

Note that system_time_zones() does have a step to filter the list from timezone_identifiers_list(), so they're not guaranteed to be the same. (In practice, they all pass the preg_match, so the counts are the same.)

It might be a good idea to test that the array count from system_time_zones() is the same regardless of whether the new $grouped parameter is true.

1. +++ b/core/modules/system/system.module
   @@ -1371,6 +1376,28 @@ function system_time_zones($blank = NULL) {
   +  // If we are asked to group the zones by region.
   

   This comment looks like a bit redundant

2. +++ b/core/modules/system/system.module
   @@ -1371,6 +1376,28 @@ function system_time_zones($blank = NULL) {
   +      $split = explode ('/', $value);
   

   There's an extra space before (

3. +++ b/core/modules/system/system.module
   @@ -1355,10 +1355,15 @@ function system_mail($key, &$message, $params) {
   + * @param boolean $grouped
   

   s/boolean/bool

4. It would be good to create some test coverage for this.
5. We should create a small change record for the API addition.

Note that system_time_zones() does have a step to filter the list from timezone_identifiers_list(), so they're not guaranteed to be the same. (In practice, they all pass the preg_match, so the counts are the same.)

OK, I see that now. That is baggage from Drupal 7 and the supported PHP versions. In PHP 5.3+, you can filter out the backzone data (they added parameters to that timezone_identifiers_list()). Since Drupal 8 is PHP 5.5+, that is essentially a no-op.

It might be a good idea to test that the array count from system_time_zones() is the same regardless of whether the new $grouped parameter is true.

I think that would be sufficient to check for regressions, or other weirdness that may appear in the future in zoneinfo.

I found some W3C accessibility documentation which supports the proposed UI change!

From the WAI-ARIA Authoring Practices 1.1 (currently a working draft), in the Listbox section:

Sets of options where each option name starts with the same word or phrase can also significantly degrade usability for keyboard and screen reader users. Scrolling through the list to find a specific option becomes inordinately time consuming for a screen reader user who must listen to that word or phrase repeated before hearing what is unique about each option. For example, if a listbox for choosing a city were to contain options where each city name were preceded by a country name, and if many cities were listed for each country, a screen reader user would have to listen to the country name before hearing each city name.

Now, this section is talking about the WAI-ARIA listbox role, rather than HTML <select>, but the point is essentially the same as we're trying to address here.

Just a little update of the Issue Summary

Thanks for the updates to the issue summary, @rachel_norfolk. I think there is work here for a novice, so I will add that tag.

The points in #65 are certainly suitable for a novice, and maybe the tests too.

The "needs tests" part is described in #63, #64, #66.

The change record should describe the new $grouped parameter for system_time_zones()

Working on this with @murrow @DrupalConBaltimore2017.

Submitting patch as requested in #65

The interdiff for #72

This is still Needs Work because #72 fixed things, but we still need test coverage.

Working on adding the test.

Thanks @murrow + @zerbash.

Confirmed that patch #72 fixes the minor issues in #65.

#74 Adding kernel tests for default and grouped use cases in system_time_zones().

I added one test to ensure the existing/default functionality was retained and a second to verify that the grouping override was working as expected.

I am unsure if the test file is in the correct location.

Sorry, I posted the wrong interdiff. Proper interdiff attached.

Thanks for the test. The spacing in the new file isn't per the coding standards. When the test runs, there should be an artifact patch to fix all of this.

1. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,31 @@
   +    public static $modules = [
   +            'system'
   +    ];
   

   Nit. Indentation is wrong, and this should all be one line.

2. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,31 @@
   +      $result = \system_time_zones(NULL, TRUE);
   

   We normally don't preface our own functions like this.

3. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,31 @@
   +      $this->assertEquals(t('Dar es Salaam'), $result['Africa']['Africa/Dar_es_Salaam']);
   +  }
   

   Don't think we need the t().

We also need the test described in #64,

It might be a good idea to test that the array count from system_time_zones() is the same regardless of whether the new $grouped parameter is true

.

That will ensure that this does what we think it does and prevent regressions in the future.

I also think we could add assertions that the three-level zones are handled properly (eg, America/Indiana/Indianapolis).

Thanks for working on tests @murrow!

Some feedback about patch #77 (though I am not a testing expert...):

1.

+  public function testTimezoneFlattening() {
+      $result = \system_time_zones(NULL, TRUE);

+  public function testTimezoneFlatteningDefault() {
+      $result = \system_time_zones();

The name of the new parameter is $grouped, so the test method names would make more sense to use that in the name.
Something like testSystemTimeZoneGrouped() and testSystemTimeZonesUngrouped()? Don't trust my camelCase though, I often get that wrong.

2.
A short docblock comment summarizing what each test method is looking for could help. They are making assertions about array structure and string format. Something like "Tests the grouped output of system_time_zones()."? I'm not sure what our ususal style is, so maybe someone else will suggest something better.

3.
The 2 test methods in #77 are checking the structure of the return array, but we don't have anything to confirm the counts yet.

In #64 and #66 we also suggested calling grouped and ungrouped calls, to confirm they had the same number of options. (i.e. none were being accidentally excluded by the code that handles $grouped).

Oops, cross posted review with mpdonadio, some duplicate feedback.

Thank you. I have updated the tests to include:

1. Additional tests for three-level timezones
2. Additional test to check that the returned array count for both the default and grouped call are equal
3. Renaming the test functions to use the "grouping" term
4. Indentation of static variable has been removed
5. Function prefix (\) has been removed
6. t() has been removed

I've also added some docblock comments, although I'm having difficulty finding suitable examples. Am happy to take pointers…

Loving all this activity!!

Set to Needs Review

Just noticed that @murrow has drafted a Change Record too. I'm running out of time for reviewing stuff today, so I'm linking it here for someone else to look at:

https://www.drupal.org/node/2873857

Awesome progress!

Apologies, I don't think I had the match array test right. We needed to accommodate the nesting and UTC. An update is attached that also cleans out a few errant tabs.

In testSystemTimeZoneMatch() you can tally the $grouped_count more simply with array_walk_recursive() which only acts on the leaves of nested arrays.

Something like this?

$grouped  = system_time_zones(NULL, TRUE);
$grouped_count = 0;
array_walk_recursive($grouped, function ($grouped_count) use (&$grouped_count) {
  $grouped_count++;
})

OK, that #89 looks more efficient. I've updated the test to reflect this change

Is there any chance of having this reviewed soon and completed?

The new patch is applying correctly and providing the expected behaviour during testing via Simplytest.me

I like the new tests - look good structurally.

The test is reporting some code standards errors in the most recent test. They will need resolving but otherwise I'm very happy.

Thanks @rachel_norfolk. I can see I need to read the test report, even when it says pass. I've aligned the spacing with the Drupal coding requirements and I believe that the attached patch is correct now. Many thanks for your quick response.

No worries - and thanks for not losing heart with it all! It’s worth setting up your local dev to allow code standards checking, rather than waiting for the testbot.

Setting to Needs Review. (I forget this ALL THE TIME!)

Tiniest of tiny changes to spacing...

I suggested to @murrow that he put the tests under core/modules/system/tests. Is that the right place for them?

I think we need a follow-up issue to update the documentation. One of the screenshots in 3.4. Running the Installer in the D8 User Guide will need to be updated.

#96, the function is provided by the system module, so it should live with all of the other system KTB tests.

I'll try to do a proper review tonight.

Looks like a testbot failure, now resolved. Back to NR.

I created a follow-up issue for the documentation: #2875672: Update screenshot and text for timezone selector change in Drupal 8.4.x.

The new test file should be a holding place for future system TZ coverage and not just this change.

Test coverage itself looks good, just down to little things now.

1. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   + * Tests system_time_zones().
   

   A bunch of comments are like this.

   Should be more generic, like "Test coverage for time zone handling."

2. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   + * @see https://www.drupal.org/node/2847651
   + */
   

   Not needed.

3. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   +   * Tests to see that the grouping flag creates nested output
   +   *
   +   * @test
   +   */
   

   Needs trailing period. Should be something like "Tests grouping in system_time_zones()." Don't need the @test

4. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   +  /**
   +   * Tests to see that the default call retains its simplier mapping
   +   *
   +   * @test
   

   Mirror the above change, but mention non-grouping or something like that.

5. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   +   * Tests to see that the grouping flag creates nested output for
   +   * timezones with three levels.
   +   *
   +   * @test
   

   Similar to above, but for three level grouping. Remember that the first line in the docblock has to be a single line, not exceeding 80 chars.

6. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   +    $this->assertInternalType('array', $result);
   +    $this->assertNotEmpty($result['America']);
   +    $this->assertNotEmpty($result['America']['America/Indiana/Indianapolis']);
   +    $this->assertEquals('Indianapolis (Indiana)', $result['America']['America/Indiana/Indianapolis']);
   

   Add a negative assertion that "America/Indianapolis" doesn't appear.

7. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   +    $this->assertInternalType('array', $result);
   +    $this->assertNotEmpty($result['America/Indiana/Indianapolis']);
   +    $this->assertEquals('America/Indiana/Indianapolis', $result['America/Indiana/Indianapolis']);
   +  }
   

   Add a negative assertion that "America/Indianapolis" doesn't appear.

8. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   +   * Tests to see both the default and grouped results have a
   +   * matching number of returned items.
   +   *
   +   * @test
   +   */
   

   Single line, not exceeding 80 chars.

9. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,88 @@
   +    $this->assertEquals(count($ungrouped_keys), $grouped_count);
   +  }
   

   Can use assertCount().

OK, I believe I have those changes in place now.

OK, I must be jet lagged. The test is wrong. I am uploading yet another patch that does work.

First, thanks to everyone who was working on this issue. Rather than give feedback on the last patch, I thought it would be a good idea to walk through some additional nitpicks.

The first is that I collapsed this into a single test method. We don't need the isolation that individual methods have, so this makes things much faster.

1. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,64 @@
   +    // Test the default parameters for system_time_zones().
   +    $result = system_time_zones();
   +    $this->assertInternalType('array', $result);
   +    $this->assertArrayHasKey('Africa/Dar_es_Salaam', $result);
   +    $this->assertEquals('Africa/Dar es Salaam', $result['Africa/Dar_es_Salaam']);
   

   OK, here one of the individual methods is folded into the single method, with a comment about what follows. I also updated the assertion to use one of the helpers.

2. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,64 @@
   +    // Tests time zone grouping.
   +    $result = system_time_zones(NULL, TRUE);
   

   The assertions that follow can all use the same result set.

3. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,64 @@
   +    // Check a two-level time zone.
   +    $this->assertInternalType('array', $result);
   +    $this->assertArrayHasKey('Africa', $result);
   +    $this->assertArrayHasKey('Africa/Dar_es_Salaam', $result['Africa']);
   +    $this->assertEquals('Dar es Salaam', $result['Africa']['Africa/Dar_es_Salaam']);
   

   OK, this is largely unchanged, just to use the new helpers.

4. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,64 @@
   +    // Check a three level time zone.
   +    $this->assertArrayHasKey('America', $result);
   +    $this->assertArrayHasKey('America/Indiana/Indianapolis', $result['America']);
   +    $this->assertEquals('Indianapolis (Indiana)', $result['America']['America/Indiana/Indianapolis']);
   +
   

   Largely unchanged, but some things have been moved around. See below.

5. +++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
   @@ -0,0 +1,64 @@
   +    // Make sure grouping hasn't erroneously created an entry with just the
   +    // first and second levels.
   +    $this->assertArrayNotHasKey('America/Indiana', $result['America']);
   +
   +    // Make sure grouping hasn't duplicated an entry with just the first and
   +    // third levels.
   +    $this->assertArrayNotHasKey('America/Indianapolis', $result['America']);
   +
   +    // Make sure that a grouped item isn't duplicated at the top level of the
   +    // results array.
   +    $this->assertArrayNotHasKey('America/Indiana/Indianapolis', $result);
   

   I placed three negative assertions here together explaining what they are doing. These are largely here to make sure that if someone changes system_time_zones() in the future, and get fails here, they will know what was being covered. This partially to make sure that any work in #2530306: America/Montreal and other linked timezones missing from Default time zone setting and #2853386: Interactive time-zone picker doesn't break what we did here.

I am adding #2678010: Time zone selection improvement (performance/UX) as a related issue. I am not sure whether that issue should be closed as a duplicate of this one. I do not think this issue addresses caching for better performance.

I was going to add a follow-up issue to review all calls to system_time_zones in core, but maybe that has already been done as part of this issue:

$ grep -rc system_time_zones core | grep -v :0$
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php:1
core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php:1
core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php:1
core/modules/system/src/Form/RegionalForm.php:1
core/modules/system/system.module:2
core/modules/user/src/Entity/User.php:1
core/modules/user/src/Plugin/migrate/process/d6/UserUpdate7002.php:1
core/modules/views/src/Plugin/views/field/Date.php:1

So "system_time_zones" appears twice in system.module and once in each of the other files. It looks as though all uses have been updated except UserUpdate7002.php and User.php. After looking at those for a minute, I think we want to leave them alone.

@benjifisher: thanks for finding the other issue. I made a comparison of the two proposed approaches over at comment #21 of #2678010: Time zone selection improvement (performance/UX).

It's not clear that the issues duplicate each other, but it's clear we can't do both :-) The other issue discusses a few formats, but hasn't got answers to them all yet. On the whole, I think this one is further developed, and type-ahead is a great accessibility win.

It looks as though all uses have been updated except UserUpdate7002.php and User.php. After looking at those for a minute, I think we want to leave them alone.

Agreed, those aren't about presenting a UI widget, they deal with validation.

Patch still applies cleanly to 8.4.x. Functional testing indicates it's still working. See attached screenshots before and after applied patch. Latest round of automated tests pass. It looks like the last thing to do is for someone to code review the automated tests, so removing the novice tag.

For what it's worth this also applies cleanly to 8.3.x.

Retesting simply to see any code style comments - I seem to have broken phpcs on my machine. Again.

No tests code style comments logged \o/

+++ b/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php
@@ -0,0 +1,64 @@
+    $this->assertArrayHasKey('Africa/Dar_es_Salaam', $result);
+    $this->assertEquals('Africa/Dar es Salaam', $result['Africa/Dar_es_Salaam']);

I suppose there will always be a risk that, if Dar es Salaam is somehow removed from PHP, our tests will fail (same goes for Indiana). Of course, I like to think this is very unlikely!

I've updated the Issue Summary to link to the proposed CR, which shows we are only adding to the API, and I'm going to set this to RTBC. Will get it in the right hands.

I suppose there will always be a risk that, if Dar es Salaam is somehow removed from PHP, our tests will fail (same goes for Indiana). Of course, I like to think this is very unlikely!

I monitor the TZ mailing list. We'll have advance warning about this if it ever happens, and we'll see it in the PHP-dev branch testing before we ever have a failure in stable bots. I also think that we won't have a problem with this. https://github.com/eggert/tz/blob/master/Theory#L48-L169 has more background about this.

Looks neater but not better in functionality.

1. When I scroll it, it's very easy to bypass the Group Heading. If I looking it alphabetically, I will not find my target. And for the most of sites, there're no grouped. They will press keyboard to locate to prefix first. The old style I still knew where I am.
(I won't use down and up arrow to select it but use Mouse hold and scroll or Mouse Wheel.)

2. I think many people don't know which Oceans they're..

and if its only aim to timezone, I think we should add these style to the list. (either before or after the list items)

I’m not sure I understand point 1 but point 2 is interesting and something we certainly could do, alothiugh id like to see the time zone offset AFTER the city name, so as to preserve the ability to type–ahead the city name.

I’m liking the final example.

As long as the city name comes first, we still get the benefits of type-ahead. I can see how adding the timezone will act as a good indicator for where in the list someone is.

I imagine this will have an effect on our tests. #hint

I like the idea of the short codes and will update the tests tomorrow (I'm now at 23:16 NZT myself).

Or as simple as this:

+++ b/core/modules/system/system.module
@@ -1371,6 +1376,27 @@ function system_time_zones($blank = NULL) {
   asort($zones);

[#2873857]
and I thought we could add 3rd arg to accept a callback function for "List sort order". If my site targeted to Asia, I want it shown first. Or in a different language, that will not sort by A-Z.
EDITED: But of course, we still able to sort it again after function calls now.

#116, that isn't really correct. The zoneinfo maintainers are moving away from assigning zone names to places that historically haven't had them, and they have started to go away in newer versions of the database (we had a critical testing bug as a result).

The biggest problem with this is that the function will be time dependant and it returns an array, not a render array. So, the only way to make this work reliably is to add a max-age=0 everywhere it gets used, and I think this would also potentially cause BC problems with contrib.

Adding the offset before / after the name may help (it's somewhat common elsewhere), but I would worry about the screen reader clutter and we also have the cache problem, and the fact that people will have to remember whether they are currently in DST or not.

People should also not really being using the manual, explicitly offsets (other than UTC itself) unless they really know what they are doing, even if they know they are somewhere that doesn't observe DST because laws change.

Bottom line is that no matter what we do, we are going to have a sub-optimal solution. The root cause of this problem is that huge selects have horrible UX no matter how much lipstick you put on the pig. We are trying to fix this with the data, but the problem is the UI element itself. JS solutions like Chosen and Select2 help a lot with this. (I really think Chosen should be baked into core...).

The zoneinfo output list is not really supposed to be used on its own as the sole way for users to choose their time zone. People on unixy systems can run tzselect to see one way that to guide people through this.

I think for the scope of this issue we commit the current patch that we RTBCed, and then figure out a interactive solution in #2853386: Interactive time-zone picker. That will be better for all in the long run.

I think for the scope of this issue we commit the current patch that we RTBCed, and

the scope assumed everyone using the keyboard to select. And if so, why not sorted by A-Z (by default). A-Z implied 26 grounded sections

There are no multiple-level dropdowns, it's easier to locate A-Z 26 grounded sections than 7 continents.

If your City does not on the list, you still have to go back to the basic.

#120 GIF, I actually select the item that way and usually that fast until to range I looking for.

The scope assumed everyone using the keyboard to select. And if so, why not sorted by A-Z (by default). A-Z implied 26 grounded sections

Are you saying alpha by "location (location), area", eg "Indianapolis (Indiana), America"? I'm not sure if that really improves things when the location (usually city) isn't where you live. I live in Philadelphia, USA; that's not in the list. With that, I would have to scroll/arrow around both up and down looking for other cities near me (Baltimore, Washington, New York, Boston), but I would have the non-America zones intermingled. At least with the grouping, I don't have to search as much.

Updating issue credits - adding
-benjifisher for doing detective work and issue triage
-martin107 for a CS reroll
-yoroy for a UX review
-laurii for a review asking for more tests

I think for the scope of this issue we commit the current patch that we RTBCed, and then figure out a interactive solution in #2853386: Interactive time-zone picker. That will be better for all in the long run.

In terms of meeting to original Problem / Motivation, the current patch is the best

I’m setting back to RTBC - I’m happy for work to take place on incorporating offset times directly or graphical timezone pickers to take place in follow-up issues but it’s really out of scope here.

Also, specifically stating that in the Issue Summary. I should have done that earlier.

This should be mentioned in the 8.4 release notes if it gets committed in time.

@mpdonadio in #121:

Bottom line is that no matter what we do, we are going to have a sub-optimal solution. The root cause of this problem is that huge selects have horrible UX no matter how much lipstick you put on the pig. We are trying to fix this with the data, but the problem is the UI element itself.

This is exactly right. As long as we're changing things within the current ui element of a select list, we're only making slightly different trade-offs.

What I think is important to keep in mind here is that in both cases where this element is presented (installer and configuration pages), there is already a default value present.

In my case it shows Berlin. Amsterdam would have been closer since I'm in NL. When I open the select list to change it to Amsterdam, the amount of "travel" I have to do to find it is the same in both current situtation and in the proposed patch here. But the patch version is much more easy to read and understand.

I don't think that losing context (which region you're in) is as big a risk or flaw than the hard to read list we have now. That the patch makes it easier to type and find is a relatively small win. Not many people know this, but maybe that percentage is a bit higher in the group that actually installs and configures Drupal sites :)

All in all, I think this is ok to commit, with the understanding that it is a relatively small optimization. A better time zone picker design would have to break out of the single select list. Oh, look!

Thanks @yoroy for another UX review - well articulated.

Committed as 9868bf9 and pushed to 8.4.x.

Thanks everyone for all the efforts on this, I realise it's been a long time in the making.

Published the change record.

See everyone over in #2853386: Interactive time-zone picker - let's keep up the momentum.

I changed the follow-up issue for documentation from Postponed to Active: #2875672: Update screenshot and text for timezone selector change in Drupal 8.4.x.

I think this has created a new test failure on PHP 7.1

Fatal error: Cannot use lexical variable $grouped_count as a parameter name in /var/www/html/core/modules/system/tests/src/Kernel/Timezone/TimezoneTest.php on line 57

should be reverted?

See https://www.drupal.org/pift-ci-job/722568

Sent message to @larowlan about this; think this should be a quick fix.

Looks like quite a few php projects hitting the same thing with php71

Thanks for taking this up, mpdonadio

Since this is causing test environment fails in non-dev versions, this is a Critical until it is reverted. Starting on a fix now.

Pretty sure this will work, https://3v4l.org/8Pgsp

Patch is against HEAD. Once we revert, it's a small change to #104.

LGTM

Thanks @mpdonadio for the quick turnaround.

I didn't get your message though? (Just saw this issue pop up in email).

Restoring the original priority. Thanks for the hotfix!