Rename the menu regions

I took a shot at this. I followed previous patterns here by renaming the regions to header_first, _second, and _third, though my prefernce would have been to rename them to header_top, _main, _bottom. But judging from previous discussions, that might not be neutral enough.

The failing tests assume the region 'header' is defined. As far as I know, the tests use Stark which has no region definitions, but pick up the default region definitions from ThemeHandler.php. There are several possible solutions:
Change Themehandler to continue to specify 'header' as a default region. But this means bartik will have a region of 'header' as well as 'header_second', since it inherits from ThemeHandler.php.
Change all the tests to specify 'header-second', but this makes the tests bartik centric.
Leave region 'header' as is, but change the menu regions to 'header_top' and 'header_bottom'.
Use 'header_main' instead of 'header_second' and change all the tests.

A question I have is why does the system page.html twig even have the menu blocks specified and ThemeHandler's default regions have the menu regions, when this really a theme specific thing? Shouldn't core themes specify their own required regions for the blocks they expect to display? It seems some assumptions about core themes are built into the System files and ThemeHandler.php without any backward documentation in core themes.

I know these are "system" menus, but they are theme objects and should be only defined there. My 2 cents, and will fix as recommended or pick my own favorite if no quick response.

I decided to leave 'header' as is and renamed the regions to header_top, _bottom. I think some of the conceptual problems mentioned have more to do with the block ui than region names. I never understand why drupal didn't have a region structure ui instead of a block ui. Regions are the container, after all.

Ok, I'll redo it but still leave 'header' defined in ThemeHandler.php as it seems to want to mimic the semantic elements, but not in Bartik. That also means the block tests won't have to be modified. 'Header_second' might be a good target region for #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo).

Since there many, many references to region 'header' in #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) I redid this with regions header, header_first, and header_second. There are patch conflicts with latest #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) so which ever one goes in 2nd will need a reroll.

Doh

Personally I think that using "Header" and "Header first" will be ambiguous for users since they won't know which is meant to be the primary region from select boxes. Perhaps rename to "header", "navigation top", "navigation bottom"?

As per the issue summary, the region names chosen seemed to cause confusion as people seemed to think they could move the region around when all you can do is move blocks. This is a problem with the block ui, which has many issues against it. And there has been a trend in drupal core themes to make region names semantically relevant but don't provide functional or layout definition.

In drupal the primary and secondary menus are special menus with a special 'use' case that a great many people are aware of and look for. They are a carry over from previous drupal versions and the names serve as a mnemonic for those who look for them and the justification of the original region name. That is why I didn't change any of the file names, just the internal labels. We still have primary-/secondary-/menu.css. Maybe one day, support for that use case will go away or be generalized.

Maybe header_sidebar1, -2 would be more acceptable and in keeping with core theme design.

Hmm, thought about it a bit more, and really don't think header first/second is the best option for a couple reasons.

1. Using numbers also implies a common layout between regions, like a row or column, and reading the label also allows you to know where it is. For example, the labels featured bottom first/second/third and footer first/second/third/fourth, imply that they are all lined up. This is not true for header first, header second or footer fifth.
2. Also consider scenarios such as deciding where to place a new block. Header first and header second, won't give any context. For example, even with existing site builders, they know one menu/block is for site navigation and the other is for account links, and where they should be placed on the layout. When attempting to place the site navigation block though, the header first and header second labels aren't helpful as they don't relate to the layout in any meaningful way.

You could just move both menus into header and use css to place them correctly.

I don't think the right issue is defined here. As far as the conceptual issues with the block ui I don't think changing the design of a specific theme really addresses that problem. If instead, bartik's current design doesn't reflect best practices, then that is something else.

+++ b/core/themes/bartik/templates/page.html.twig
@@ -48,10 +48,10 @@
+ * - page.header_first: Region for items like the user account menu.
  * - page.header: Items for the header region.
+ * - page.header_second: Region for items like the main menu.

This does seem confusing. Ordinal naming is fine, but it doesn't make sense when keeping one region just called "header", with first before it and second after it.

I'd so like to keep this as "primary_menu" and "secondary_menu" region keys. Can we just suffix the regions with " Region" to avoid confusion? Or just rename the labels slightly?

-1 to header_first/header_second and renaming the regions in code at this time

Can we look for a cosmetic fix here?

Adding a suffix of _region seems redundant in the context in which they are displayed. We could adopt pre_header and post_header or we could adopt a triplet of header_first/_content/_last, or header_top/_content/_bottom. None of these indicates what is in those regions while hinting their layout. It will just make the patch slightly larger. And I'm slightly against having region names the same as the semantic unit they belong to(region 'header' inside <header>).
One of the issues is because the block ui lists regions in the order they are defined in info.yml, not in their order in the theme layout. How would users react if they saw the block ui page in the attached image? We could recommend that regions be defined in info.yml in the order they appear in the theme layout but none of the current core themes follow that practice.
I also think the conceptual issue reported in the issue summary wasn't specific to primary and secondary menu. They just happened to be first on the page. The names may violate best practice, but it would be nice to keep them.

Just a nit. I've always disliked the block ui because the table displayed is really indexed by region though the label above the first column says BLOCK and the column labeled REGION contains a selector for the assigned region. I understand the table but found the column labeling confusing the 1st time I saw it. You could change the BLOCK label to REGION >> BLOCK.

Is this a re-roll or did you make changes?

This is a minimalist change. Internally the region name remains primary and secondary menu but the label displayed is 'Pre header' and 'Post header'. It amounts to a 2 line change.

Opps. If this is accepted I'll have to change the default region list also. 4 lines.

A decision should be made soon, it doesn't really help to drag this out. My understanding of status as of now:

We should leave 'header' region as is. A change here would have impact on other issues currently in process and require changes to at least 4 tests. Not sure it is worth it.

3 possible solutions:
1) Do nothing and rely on changes elsewhere to improve concerns raised in summary.
2) Rename primary_menu and secondary_menu regions thru out bartik code. API impact because of sites currently using these region names.
3) Leave the machine names for primary and secondary menus the same but change the human readable name to something that reflects best practices. This is really just a cosmetic change and won't entail api change.

So far suggested names for the 2 regions include:
header_first/top/start/begin
header_second/bottom/end/last
navigation_top/bottom
begin/start/top/pre/ante_header
end/finish/bottom/post_header

Honestly, I think emma.maria should be the one making the call on region names and I don't see any comments from her yet. I'm tagging for maintainer review.

#22 is the direction I am hoping for, thanks for considering it @dcrocks. The names may not be right but it doesn't cause too much fuss around renaming all the things and less disruptive of a patch in general if it will be enough?

To me "Navigation" is a nice catch all for menus, pagers, etc and may help.

So throwing in "Primary Navigation" and "Secondary Navigation" into the mix to help distinguish the Menu block from the Region.

I'd leave the call to @emma.maria though.

@LewisNyman 'header' - It is doable but is an api change for current themes. I had considered header_content or header_main. Do you have a suggestion?

@LewisNyman good points... can we do without a new region then for these blocks? AKA remove them?

The only other region currently available to place blocks is 'header'. And bartik's current page.html.twig is hardcoded to add system logo, slogan, and site name. The menus are supposed to sandwich that code so it would be very difficult to lay that out properly by placing them in a region that is rendered after that code. That will change after #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) but I think that issue's 'branding' block will be placed in 'header' as its region.

Because I had time. This is strictly cosmetic for bartik only. But for the long term, all 3 regions should be renamed.

:( can't win one way can't win the other...

menu regions are renamed now. check screenshots.

Renamed where?

I tried to see if this could be done with css alone and it doesn't look simple. The styling for these menus is tied to them being in these regions. Put them in another region and they only have default menu styling. Put other menus in these regions and they will acquire styling written for the primary and secondary menus. I tried both cases and they result in unsatisfactory output.
I may be wrong but I thought the styling on blocks is supposed to be region agnostic. It isn't on these 2 blocks. Since it is apparent in bartik that these 2 regions are targeted to have these 2 menus, and these 2 only, why is it wrong to have the region name indicate the content it is targeted for?
Lastly, I thought the purpose of regions was to create layout and to optionally provide common styling attributes for their content. In bartik they are treated as components, which seems odd to me.
And why is a class name(site-footer) treated as a component?

I think the regions should be called.

header_first: 'Header first'
header_second: 'Header second'
header_third: 'Header third'

This follows the same patterns used for other regions. The region names should not hint at what content they have, as they in theory could contain anything. The only thing the names need to convey is that they are all found within the header section of the markup with a number hierarchy to distinguish them from each other.

Also each region needs to be assigned with first, second etc, we can't have one that is just called 'header' on it's own.

The names in #33 are not immediately logical to the user and break the patterns of other grouped regions in Bartik.

Please can we have a patch with Header first, header second, header third.

If tests break because we are renaming regions we will have to fix them. We should not implement a half fix or just a cosmetic fix as we are trying to fix usability here.

I'll start work on this but I want to point out the 'featured-top' and 'featured-bottom' also break the pattern. If that pattern is allowed might we consider the top/main/bottom pattern for header?

ps. and we also have page top/bottom

Added Header regions as per feedback by Emma in #42, See if it breaking structure or not ;)

This patch is probably incomplete. It:

Does region name rename only
It ignores changes to these same files currently being worked on in other issues.
It doesn't change maintenance page files or css
Doc changes should include modules/help/help.php.api

Another try. I had forgotten Classy. The problems are in some of the block tests but not sure yet what the problem is.

Know what it is, fix tomorrow.

@dcrocks:
Featured top and bottom do not sit next to each and are supplementary regions to the Main content. Featured bottom also consists of three regions so the Top | Bottom naming convention was needed.

Bartik has one header that consists of three regions that come after each other, so the first | second | third naming convention works well here.

Also @dcrock's can you merge your patch with @Manjit.Singh's patch in #36 and carry on with the work. You did not review his patch before posting your own and his work had a correct start to the solution, the same code in your patch has whitespace at the start of the lines.

Thanks.

#36 doesn't have a patch. @manjit.Singh's last patch was in #44, and only changes bartik.info.yml, which has been done in all my patches. And all the region definitions in bartik.info.yml have whitespace at the start of the line.
I am hoping this patch will fly.

bump

We'll have to fix all the placed blocks.

I was worried about migration issues. Where is that done? I am also still worried about the system maintenance page.

#52: OK sure. I noticed the following from your patch in dreditor and I got mixed up.

@emma.maria I'm not sure why the patch looks like that but if you look at the generated code everything is fine. Maybe has to do with git and tabs?

What's there works, but needs review.

Working on the upgrade path

The patch needs to be tested for visual regressions and the code reviewed, even though it is mostly substitutions..

Maybe it would be good idea to add the changed region names to the IS

Is update

We also need to update the issue to contain other themes and core because they seem to have the same regions too.

Posting my progress. Broken upgrade path which cannot handle themes which this shouldn't affect etc. We also need tests for that.

Ups empty patch

@lewisnyman: I fixed that already in my patch

IS update.

Nothing to review

+++ b/core/themes/bartik/templates/page.html.twig
@@ -48,10 +48,10 @@
+ * - page.header_first: Items for the secondary menu region.
+ * - page.header_second: Items for the header region.
+ * - page.header_third: Items for the primary menu region.

Comments don't match the region names.

What do we need to do about the upgrade path? For Bartik it should be (reasonably) straightforward, but what about a custom theme that extends Classy? If they have overridden page.html.twig don't they need to update their template to match?

We have to load all blocks using the old region names and resale them with new ones.

+function system_update_8001() {
+  /* @var \Drupal\block\BlockInterface[] $blocks */
+  $blocks = entity_load_multiple_by_properties('block', ['region' => 'header']);
+  foreach ($blocks as $block) {
+    $block->setRegion('header_first');
+    $block->save();
+  }

That is what the update hook does in the latest patch, but this does this blindly for all themes. We can limit this to Bartik or Classy by adding an extra property condition, but we also need to consider subthemes of Classy, but only if they relied on the default regions and didn't provide their own custom "header" region in the theme info file. And if they used the defaults, we will still break their theme if they have a custom page.tpl.php that refers to the old region names.

Read the IS. That is exactly what is expected if this issue is committed.

@Lewis, we can change templates and regions. The problem is we now need upgrade hooks to move anything that was in the region we removed/renamed to put those blocks into the new regions, so the blocks aren't orphaned. If we don't, when someone updates their site they'll get those "block set to region that doesn't exist..." errors, or whatever it says.

In this patch I fixed the comments, moved the update hook to block.install, and started writing a test. But the test fails, I think because block_rebuild() fires before the update hook is run, so the blocks are already disabled by the time we try to move them.

You missed the comments in modules/system/templates/page.html.twig. I mapped the regions to reflect the order in which they appeared in bartik's output, you used their original order in comments. 6 of one, half dozen the other.

The update places the blocks as

'header' in 'header_first'
'secondary_menu' in 'header_second'
'primary_menu' in 'header_third'

.
But the test assumes 'secondary_menu' is in 'header_first'. Need to decide what placement order is desired as inconsistent now. Perhaps follow the order define in ThemeHandler.php?

'header' in 'header_first'
'primary_menu' in 'header_second'
'secondary_menu' in 'header_third'

RE #79 yes, templates are not frozen so we can make changes to them. We should have change records for changes to page template because of the significance of the template, and adding or removing variables/regions from the default template is pretty significant.

What we need an update hook for is the blocks. Changing the block regions is like a schema change, since the values are stored in the database. It won't affect a theme that defines its own regions (nor would the page template changes) but a theme using the defaults will have a problem.

Adding change record tag. We need one for the region changes.

I made the small changes I indicated in #83 and #84.

@dcrocks from your comment #84 I just need to check the following is being set.

With the ordering of Bartik's current regions, the region changes should be:

Secondary menu -> Header first
Header -> Header second
Primary menu -> Header third.

@dcrocks: could you please post an interdiff while you change something to make it easier to follow what has been changed between patches. Yoi can find the documentation about how its done here.

@emma.marie That has been the pattern I've been following but the patch in #80 suggested a possible different choice. Thanx for the direction.

@laurii Sorry, I'm not adept with Git, but I'll try to follow best practices from here on.

@dcrocks: Here are some instructions on making an interdiff. Fairly similar to making a patch. You just need to branch off 8.0.x to commit the last patch, and then you can diff between the branch you are working on and that up-to-date last-patch branch, in order to show the changes from one patch to another. It really helps to see what work someone is doing when posting a patch. When you are doing a reroll, an interdiff isn't necessary because there are too many changes. At all other times, interdiffs make life easier! Thanks!

Helps if I post the link: https://www.drupal.org/documentation/git/interdiff

Thanx for the pointers.

I am trying to get my head around this problem with the update. If @longwave is right the blocks are already disabled when the update fires and that's why the test fails. The impact should be that the user sees a bunch of standard blocks in their 'disabled' blocks list and a empty header region in Bartik. If the toolbar is not turned off, it is relatively easy to fix things up. But if not, and for other profiles than the standard one, things aren't quite so simple. So is there any place else to handle this, perhaps in theme handling or info file processing?

Rebased #86 as block_update_8001() is now taken. If I remove block_rebuild() entirely then the failing test passes, but that is obviously not the right solution - but I am also not sure what the correct thing actually is.

According to test bot it seems to be opposite :P

Neither patch actually includes the HeaderRegionRenameUpdateTest.php, so no surprise the first passed.

There is a long discussion which has been happened when this was done: #1869476: Convert global menus (primary links, secondary links) into blocks so this wasn't done by a mistake which I was assuming in the first place. The reason it was done in the first place was that we used to have an UI for selecting primary menu and secondary menu which are being outputted in the variables and we didn't want to remove. To support that we created them as separated regions so that themes could theme their menus according what's inside that region and site builders could change the menu inside the region. Apparently people don't know how it work. It was super confusing and even I couldn't figure out how I'm supposed to use those regions before reading the original issue. I think we either need to add a help text there or rename the regions at least on the UI so that its clear what they are there for.

That's what I get for trying to post patches in a hurry before going to sleep. This is #95 with the missing test added back.

#1869476-187: Convert global menus (primary links, secondary links) into blocks and #1869476-188: Convert global menus (primary links, secondary links) into blocks certainly do make good arguments for the way things currently are, and after scanning that entire issue there seems to be no single perfect solution that will please everyone. Maybe the current setup is the least worst option, and adding help text to explain the use of Bartik's regions would improve things? Header first/second/third may remove some of the confusion but that does not follow that it will necessarily make the blocks page any more usable.

#2502045: Replace the "Primary menu" component with a reusable component in Bartik. and #2502049: Replace the "Secondary menu" component with a reusable component in Bartik. are meant to address part of @LewisNyman's comments in #102. This issue was supposed to just address the region renaming.

This has been sitting for a while. I don't think I quite understand the update process but if we simply update the block config but the theme definition hasn't been updated we will still have a mismatch between the block region designation and the theme's region definitions. What I would expect that for any block configured for a region that doesn't exist in a given theme the block would be placed in the theme's default region, which, I think, is the 1st region defined in that theme's info.yml. And unless the theme has the old region names, we can't mess with it. Even then we aren't really sure what the consequences of that change would be on the theme. So, is it actually possible to have an update mechanism for this change?

It seems to me that we can't do any updates to custom themes as we don't know the themer's intent. All we can update is the bartik theme, and even there we might step on some local modifications. And the test added only tests the bartik theme anyways. So I modified the update to just do a forced update of the bartik account menu and main menu blocks.

duh

Small typo, big fail

Why didn't we get this earlier? Just duplicate dpattern of other updates.

duh again

Need to finish. There will be multiple patches affected by #2555183: Fix the filled update tests, they are broken.

another typo, but I'm concerned about all the other errors.

+++ b/core/modules/block/block.install
@@ -142,5 +142,18 @@ function block_update_8002() {
+  $block = entity_load('block', 'bartik_account_menu');
+    $block->setRegion('header_first');
+    $block->save();

a) The other two update hooks use the config objects directly, bypassing the entity system. Why doesn't this do the same?

b) If you continue to load the entity, please do it like

Block::load('bartik_account_menu')
  ->setRegion('header_first')
  ->save();

Also, a lot the "needs" tags can be resolved without the intervention of others (like BE and IS update), and will actually help others review this.

just a nit

+++ b/core/modules/block/block.install
@@ -142,5 +142,18 @@ function block_update_8002() {
+  $block = entity_load('block', 'bartik_account_menu');
+    $block->setRegion('header_first');
+    $block->save();
+  $block = entity_load('block', 'bartik_main_menu');
+    $block->setRegion('header_third');
+    $block->save();

indent wrong, also better to use Block::load()

This is a reroll after #507488: Convert page elements (local tasks, actions) into blocks and for the recommendations in #118. I would like to take a stab at using config instead of the entity system but I wanted to try this first.

Know that will fail, so trying again.

If we can get it in, #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) will affect what content is in the header. That's RTBC, so a heads-up.

This is not the only one impacted. It would make sense for #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) to go in first.

I don't know the original decision to use the entity system came about but what would be the advantage of using config objects over the entity system?

$entity->save() dispatches events and invokes hooks. Those can be unsafe during update.

Using config instead of entity

There are a *lot* of "needs" tags on this issue...

Fixing the coding style of the last patch.

Sorry. I was planning on doing that when this needs a re-roll, as I expect #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) to go in first. Should probably remove "use Drupal\block\Entity\Block;" from block.install as well. I can remove tags but I didn't add them so I'm leery about changing them.

This is a re-roll after #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo). Some images included.

This won't fix the fail, just doing a little clean up.

Ok, now needs review and change record.

1. +++ b/core/modules/system/templates/page.html.twig
   @@ -58,11 +58,11 @@
      <header role="banner">
   -    {{ page.header }}
   +    {{ page.header_second }}
      </header>
    
   -  {{ page.primary_menu }}
   -  {{ page.secondary_menu }}
   +  {{ page.header_third }}
   +  {{ page.header_first }}
   

   We're naming these regions numerically, but the order is 2, 3, then 1?

   It seems like the region number should match the source order.

2. +++ b/core/themes/classy/templates/layout/page.html.twig
   @@ -56,11 +56,11 @@
      <header role="banner">
   -    {{ page.header }}
   +    {{ page.header_second }}
      </header>
    
   -  {{ page.primary_menu }}
   -  {{ page.secondary_menu }}
   +  {{ page.header_third }}
   +  {{ page.header_second }}
   

   header_second is used twice here. Should the first one be header_first?

This is the order in Bartik and matches the placement of the blocks in Bartik. It is not the same order in page.html.twig files in system or classy. It is the order that @emma.maria had asked for back in #42.

mdrummond's comment wasn't about the name of the regions, but the order and placement in the template. It indeed looks wrong. They should be in order and not duplicated.

When I 1st set it up I was thinking in terms of the content more than the region. This patch changes all core page.html.twig templated to have the same order,

@dcrocks should not the order be same for bartik as well ?

+++ b/core/modules/system/templates/page.html.twig
@@ -58,11 +58,11 @@
   <header role="banner">
-    {{ page.header_second }}
+    {{ page.header_first }}
   </header>
 
+  {{ page.header_second }}
   {{ page.header_third }}
-  {{ page.header_first }}
 
   {{ page.breadcrumb }}
 

The header regions appear in the same order in all of the core page.html.twig files; system. classy, and bartik.

Spent too much time looking at wrong ThemeTest.php.

Small change but faster thn waiting for 151 to finish

Well I know that will blow up because somebody else grabbed update 8003

Quick fix

Something to think about:

- You may want to check the block configs for the minimal and standard profiles to check which regions each block is being placed into: those may need to change so they still show up in the right spot.
- We'll probably need an upgrade path for this. We can't do much for custom themes, but we can check for Stark, Seven, Classy and Bartik to move blocks in regions whose names are changing to the corresponding new regions that take up the same place in the layout.

Note that seven has only one header region and that wasn't touched by this patch. Classy and stark inherit the default theme definitions in ThemeHandler.php.
Using a git clone of 8.0.x and applying the patch, after install:

On a minimal install:
stark is the default theme and its blocks are assigned regions as configured.
All the other themes are in the 'uninstalled' state.
When installed they will inherit blocks from whatever blocks are in the current default them plus 'Site Branding' which is placed by system.install.

On a standard install:
Bartik, Classy, and seven are in the 'installed state' and blocks are assigned as configured.
Classy has no blocks assigned to any region, which is as designed.
Stark is in the 'uninstalled' state and when installed will pick up whatever blocks are assigned to the current default theme in the corresponding regions, if it exists.

The only possible additions I can see at this time is to have additional blocks, such as primary or secondary menus for stark, placed by system install.
There is a minimalist upgrade path and I'm not sure how it can be added to.

Re-roll to include changes from #2503755: Switch from user login block to login menu link and search block in standard profile and some before and after shots.

I just noticed that RC1 has arrived. Given that, this patch will be much more disruptive since the number of users will be much higher than the number on the betas, and also probably less experienced. Much as I hate to say it, it might be better to postpone this.

Unfortunately, this does not appear to meet the allowed changes criteria for 8.0.x now, so I agree this must be postponed until 8.1.x.

You don't have to actually postpone it. It can still be worked on, just not committed. The change in version is the clear indicator that it would not get committed to 8.0 and have to wait to be added to 8.1.x-dev.

Though, admittedly, I'm not sure this could even go in 8.1. Changing the default regions is a change that we can only provide limited backwards compatibility for. We might have to leave that change for 9. All the changes to Bartik and Seven we can make though to some version of 8 after release.

Doesn't look like 8.1.x is synced with RC so can't work on that branch at all and as long as issue assigned to 8.1.x can't work on it as 8.0.x either.

I think/hope we can do this in 8.2.x. The plan though is that we can't change classy or stable. But all other themes/core is fair game.

This should be a good test of how much we can test in the name of UX for the themes we want to improve and for core and still giving backwards compatibility for 8.x

Here's a reroll of #161

Another reroll of 161

@dcrocks Thanks that re-roll looks better than mine.

@dcrocks Could you remove the classy and stable bits of this patch?

So you want classy and stable to maintain the old region names? Neither classy nor stable specify regions in their info.yml but they have templates that do. I don't expect issues but I guess we will have to wait and see.

This will probably not fix everything. I'd like to add that this would be easier if non-stable themes were not used in testing unless they themselves were being tested.

This has turned into a real mess. Reverting classy to use the old region names has created all kinds of conflicts. It made tests that use classy now fail because the page.html.twig file has the old names and I have to find and revert all those tests. This also affects tests that use stark because it inherits from classy. But that is not the hard problem. Since the existing block yml files are actually written for bartik, if they are run in tests that use classy(or stark) as the theme, the tests fail. I'd like to restart and make this a bartik only change but for the problem with the block yml files. If #2642590: Move Bartik block config into the Bartik theme and its brethern were in, it would make any solution here safer for the long run.

So, bartik, classy, stable, and system all carry these region definitions around in page.html.twig, plus some core block yml files that are designed for bartik(eg. system branding) but used in tests with other themes. Honestly, I'd prefer they all used the new names because they are more generic and not bartik centric.

I know we can't modify classy and stable, and we probably shouldn't touch any templates in system module as well. I could redo this and focus just on bartik. That means bartik would look very different from default region setups in the rest of drupal core, including the default region definitions in ThemeHandler.php. Maybe that's for the better, maybe it will just create confusion, and maybe it's not worth the effort.
The original issue scope really was to change this in all of Drupal. If that is still the intent, then maybe this has to be moved to 9.x.

ps. If this is done in 8.x I would like to change the tests impacted by this change to NOT use bartik unless they actually have to. Its a pain to have to fix tests because they chose a moving target for a theme and don't really see a functional change.

To be clear, I am looking for input as to whether to continue with this or not.

I am going to mark this as postponed and for 9.x for several reasons:

The old region names are already embedded in existing 8.x sites.

The original issue posed this as a core wide change. Having both the old and bartik specific regions names will probably lead to some confusion(and cursing). If this is done for Bartik only, then themes that think they are inheriting bartik's menu blocks will find them disabled because the block's menu regions don't exist in the new theme's layout.

This is still worth trying in a minor release, moving back for now.

See no reasons for this to be postponed.