Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#39692 closed enhancement (fixed)

Fix missing assignment of menus on theme switch

Reported by: melchoyce's profile melchoyce Owned by: obenland's profile obenland
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-patch needs-testing commit
Focuses: Cc:

Description

Switching themes will cause menus to be "lost" in theme switch. See this post for examples.

Change History (71)

#1 @melchoyce
7 years ago

Also related: #39693

#2 follow-up: @lukecavanagh
7 years ago

@melchoyce 

So if a similar menu names exists on the new theme, then the memu connection would remain?

Would there be an option for mapping the existing menus names, to the new menu names in the new theme easily?

#3 @westonruter
7 years ago

  • Milestone changed from Awaiting Review to 4.8

#4 in reply to: ↑ 2 @melchoyce
7 years ago

Replying to lukecavanagh:

@melchoyce 

So if a similar menu names exists on the new theme, then the memu connection would remain?

Would there be an option for mapping the existing menus names, to the new menu names in the new theme easily?

I think there's a couple things we could do:

  1. If your old theme only has one menu and your new theme only has one menu, just map them automatically
  2. If your old theme and new theme share a similar IDs or names, map them automatically
  3. If your old theme has more than one menu, or your new theme has more than one menu, map the menu in the first menu location in your old theme to the first menu location in the new theme.

FWIW I hate menu locations :)

#5 follow-up: @lukecavanagh
7 years ago

@melchoyce 

What would be a better name or term than menu locations then?

#6 in reply to: ↑ 5 @melchoyce
7 years ago

Replying to lukecavanagh:

@melchoyce 

What would be a better name or term than menu locations then?

I don't hate the name, I hate the entire concept. It brings an extra layer of complexity that stumps pretty much every new user I've ever seen work with WordPress, both in person and via user testing.

#7 @folletto
7 years ago

  1. If your old theme only has one menu and your new theme only has one menu, just map them automatically
  2. If your old theme and new theme share a similar IDs or names, map them automatically
  3. If your old theme has more than one menu, or your new theme has more than one menu, map the menu in the first menu location in your old theme to the first menu location in the new theme.

This sounds accurate to me.

I don't hate the name, I hate the entire concept. It brings an extra layer of complexity that stumps pretty much every new user I've ever seen work with WordPress, both in person and via user testing.

Same. I think it's clear why it happened that way, but it's incredibly confusing for users.

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


7 years ago

#9 @westonruter
7 years ago

  • Owner set to jonathanbardo
  • Status changed from new to assigned

#10 @melchoyce
7 years ago

Hey @jonathanbardo, I see you've been assigned to this ticket, along with #39693. Any idea when you'll have a chance to start working on some patches?

#11 follow-up: @jonathanbardo
7 years ago

Hey @melchoyce,

Sorry for the late reply, I was on vacations in a country with no internet :( I should be able to look into this pretty soon. I think we should rework the original description to better reflect the issue because I don't think this is a regression rather than adding new UI in the customizer to deal with unassigned menu when doing theme switch.

#12 in reply to: ↑ 11 @melchoyce
7 years ago

Replying to jonathanbardo:

I think we should rework the original description to better reflect the issue because I don't think this is a regression rather than adding new UI in the customizer to deal with unassigned menu when doing theme switch.

Can you elaborate on this? I don't think this ticket will require any new UI, based on what I proposed in #4.

#13 follow-up: @jonathanbardo
7 years ago

@melchoyce I'm a bit confused too since when testing changing themes, menus were assign automatically to a menu location if they shared the same id. Maybe I'm not testing correctly?

#14 in reply to: ↑ 13 @melchoyce
7 years ago

Replying to jonathanbardo:

@melchoyce I'm a bit confused too since when testing changing themes, menus were assign automatically to a menu location if they shared the same id. Maybe I'm not testing correctly?

Yup! The issue is when menus don't share the same IDs. We're trying to intelligently re-assign menus that don't match, using these criteria:

  1. If your old theme only has one menu and your new theme only has one menu, just map them automatically
  2. If your old theme and new theme share a similar (not the same) IDs or name, map them automatically (so, primary, main, menu 1, etc. are all similar in concept)
  3. If your old theme has more than one menu, or your new theme has more than one menu, map the menu in the first menu location in your old theme to the first menu location in the new theme.
Last edited 7 years ago by melchoyce (previous) (diff)

#15 @jonathanbardo
7 years ago

So it seems there is 2 components to this. One is to actually fix the switch_theme method and the other one is to be able to fix the customizer theme preview since it changes options at runtime and isn't calling switch_theme() until the user clicks save & activate.

#16 @melchoyce
7 years ago

  • Owner jonathanbardo deleted

#17 @melchoyce
7 years ago

Following up on this ticket after posting 39693#comment:20.

If your old theme and new theme share a similar (not the same) IDs or name, map them automatically (so, primary, main, menu 1, etc. are all similar in concept)

@ipstenu grabbed some data for us from the theme directory. Logs here.

Most common menu area names seem to be:

  1. Main
  2. Primary
  3. Secondary
  4. Footer
  5. Navigation
  6. Subsidiary

Using that info & some more research, I think we can do this:

  • Main, Primary, and Navigation (along with any combination of these, like Main Navigation) can all be mapped to each other
  • Secondary and Subsidiary can be mapped to each other
  • Anything with "Footer" or "Bottom" in it can be mapped, if they don't match
  • Anything with "Social" in it can be mapped, if they don't match
  • Anything with "Top" in it can be mapped, if they don't match

This is in addition to:

  1. If your old theme only has one menu and your new theme only has one menu, just map them automatically.
  2. If your old theme has more than one menu, or your new theme has more than one menu, map the menu in the first menu location in your old theme to the first menu location in the new theme.

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


7 years ago

@obenland
7 years ago

First pass at guessing menu locations

#19 @obenland
7 years ago

  • Keywords has-patch needs-testing added

@melchoyce I added a patch with a first pass.

It maps menus when old and new themes only have one location or locations that are named the same, and also attempts to map locations that share one of the common terms you listed earlier.

I grouped them in primary, secondary, and social menu groups so they only map within that group. 'footer-menu' would map to 'secondary' but not 'main-menu' for example.

The mapping is pretty rudimentary currently, I'm sure that can be optimized. Let me know what you think?

#20 @melchoyce
7 years ago

Is there a way for this menu "memory" to persist across a couple theme changes? I noticed after I tried live previewing a couple themes in the Customizer, it stopped reassigning my menu.

#21 @obenland
7 years ago

Hm, I'm not sure how much of my patch actually applies to live preview since it's hooked into after_switch_theme. I'll have to look into that

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


7 years ago

#23 @westonruter
7 years ago

As with #39693, I've forked the slurper and have written a parser for obtaining the sidebar args for the themes: https://github.com/xwp/WordPress-Theme-Directory-Slurper/tree/master/feature-stats/registered-nav-menu-locations

Here are stats from the parsing: https://docs.google.com/spreadsheets/d/1QCormQoVGlI8rKxgn0ylxEw4JAa71372wdoJwgLvENs/edit#gid=0

The most common number of nav menu locations is 1, followed by 0 and then 2. The max a theme has is 9. Here are the stats:

Location Count Theme Quantity
0 1312
1 1921
2 878
3 291
4 79
5 10
6 2
7 2
9 1

Plotted:


As for the most popular sidebar IDs used, here are the top 30:

Sidebar ID Count
primary 2141
footer 302
social 295
secondary 249
main-menu 132
header-menu 109
footer-menu 94
top-menu 80
top 76
primary-menu 73
main 65
header 49
main-nav 45
main_menu 37
subsidiary 36
custom_menu 33
footer_menu 31
header_menu 30
Header 24
menu-1 24
main-navigation 23
footer-nav 23
mobile 23
top_menu 21
menu 18
main_nav 18
mainmenu 18
primary_nav 17
notfound 16
top-nav 15

There is a degree of normalization that can be done for these, such as replacing “main” with “primary”, removing hyphens, removing “navigation” and “menu” and so on.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

#25 @westonruter
7 years ago

As for the menu groups, from this data I think that header should be included in the primary group.

Also, the string comparisons should be changed to be case-insensitive, and also to explicitly check for false as strpos() can return 0 if the haystack string begins with the needle string. So something like so:

if ( false !== stripos( $location, $slug ) || false !== stripos( $slug, $location ) ) {

Then we need to figure out how to apply these changes up-front when opening the customizer to preview theme switch. When the customizer is used for theme switching, we should presume that the nav menu locations will be assigned during the customization session. So as with starter content, the customizer should open with the nav menu location settings already dirty and populated with the remapped nav menus, and so they shouldn't be changed when the new theme then gets active.

Also, if a theme was previously active, then it will already have nav menu location assignments in its theme mods. In that case, I think this menu remapping switching logic should only be done if the target theme has no stored nav menu location assignments already.

#26 @melchoyce
7 years ago

Aside from adding support for live preview, what else needs to happen here to get something mergeable?

#27 @obenland
7 years ago

I think once #39692:25 was addressed we should be pretty close

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


7 years ago

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


7 years ago

#30 @jbpaul17
7 years ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


7 years ago

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


7 years ago

@obenland
7 years ago

#33 @obenland
7 years ago

In 39692.2.diff:

  • Added header to primary group.
  • Hardened string comparisons to check for false and not be case sensitive.
  • Maps menus on top of existing menu locations if theme was previously active.
  • Added unit tests.

Unit tests helped quite a bit to uncover and fix odd behaviors and make the mapping much more reliable.

Last edited 7 years ago by obenland (previous) (diff)

This ticket was mentioned in Slack in #core-customize by obenland. View the logs.


7 years ago

#35 follow-up: @joyously
7 years ago

Looking at the last patch, I think there are two cases that are strange for theme switch, and apparently this is not handling Customizer previewing at all.

Case 1: the new theme has theme_mods already. Those should be used, untouched.
Case 2: the old theme has no menu selected. Nothing to do for new theme.
The old code (removed) handled these. The new code handles case 2, but not straightforward.

Also, the new function is called _wp_menus_changed(), but it is about changing themes, not menus. Perhaps a little more clarity in the name, so it can be called from both switch_theme() and whatever the Customizer equivalent is?

I also think the call to get the menu locations should be consistent. So in theme.php, instead of

$nav_menu_locations = get_theme_mod( 'nav_menu_locations' ); 

use the same call, like

$nav_menu_locations = get_nav_menu_locations(); 

And since we are talking about a switch, the temp option could be named more clearly. Instead of 'theme_switch_menu_locations', could it be 'theme_switch_old_menu_locations' or 'theme_switch_from_menu_locations'?

I wondered about language. Since Weston shows the most used menu slugs, and they are all English, all the other slugs that didn't make the top 30 list could include words in other languages along with less popular English words. I guess that doesn't matter.

Having looked at over 400 themes for theme review, I might classify the common slugs this way:

$common_slug_groups = array( 
  array( 'header', 'main', 'primary' ), 
  array( 'secondary', 'subsidiary', 'top', 'custom' ), 
  array( 'bottom', 'footer' ), 
); 

#36 in reply to: ↑ 35 @obenland
7 years ago

Replying to joyously:

apparently this is not handling Customizer previewing at all.

It's not handling it yet, like I said.

Case 1: the new theme has theme_mods already. Those should be used, untouched.

They are used, though not untouched. If a menu was changed since the theme was active last, we should account for that.

the new function is called _wp_menus_changed(), but it is about changing themes, not menus. Perhaps a little more clarity in the name, so it can be called from both switch_theme() and whatever the Customizer equivalent is?

That name is modeled after _wp_sidebars_changed(), but I'm open to suggestions. The meat of the function will probably have to be separated out anyway to accommodate the customizer.

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


7 years ago

#38 @jbpaul17
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub.

#39 @melchoyce
7 years ago

Is anyone else available to review @obenland's patch?

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


7 years ago

This ticket was mentioned in Slack in #core-customize by melchoyce. View the logs.


7 years ago

#42 @westonruter
7 years ago

  • Keywords needs-unit-tests added

I've put the patches into a pull request which we can use for review and collaboration going forward: https://github.com/xwp/wordpress-develop/pull/245

#43 @welcher
7 years ago

I reviewed the patch and just have a couple of comments.

  1. I think we should do the check for ! empty( $old_nav_menu_locations ) right away and only do the processing if so. It's a minor thing, but why do any processing if there is nothing to convert from.
  2. The in_array check is not doing a strict check. Technically, nav menus can be registered with a numeric location such as array( 1 => 'First Menu', 2 => 'Second Menu', ) which will produce a false positive and in turn, an undefined offset error.

I have applied the patch and am getting a failure on one of the unit tests:

1) Tests_Nav_Menu_Theme_Change::test_one_location_each

Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
+    0 => 1
 )

I've attached a patch with the proposed changes and a new test that demonstrates the in_array error if not using strict.

@welcher
7 years ago

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

#45 @westonruter
7 years ago

@obenland I'm creating a suite of child themes for testing and debugging to confirm that the theme switching is resulting in nav menus being assigned to the new locations as expected: https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations

Right now there is one test for a theme that has nav menu locations (primary, secondary) and another theme that has nav menu locations (top, footer) and it checks to confirm that the menus assigned to locations in the former theme get re-assigned to the locations in the latter theme. So far, so good! ✅ The tests pass: nav_menus_switch_from_nav-menus-primary-secondary_to_nav-menus-top-footer.sh

These tests are a level above the unit tests in 39692.3.diff (which are still needed) and more in the realm of acceptance testing, which is really going to be the only feasible way to test menu switching in the Customizer.

Version 0, edited 7 years ago by westonruter (next)

#46 @obenland
7 years ago

  • Keywords needs-unit-tests removed

Whoa, nice! Thanks for that!

#47 @joyously
7 years ago

I still think the groups are not right being

$common_slug_groups = array( 
   array( 'header', 'main', 'navigation', 'primary', 'top' ), 
   array( 'bottom', 'footer', 'secondary', 'subsidiary' ), 
   array( 'social' ), 
   // TODO: Find a second slug or remove, since locations with same slug are already mapped. 
   );

The themes I have seen will typically have two or more menus at the top of the page and perhaps one at the bottom. So, like I said before, this makes more sense: (top is typically right below the admin bar and not the main menu)

$common_slug_groups = array( 
  array( 'header', 'main', 'primary' ), 
  array( 'secondary', 'subsidiary', 'top', 'custom' ), 
  array( 'bottom', 'footer' ), 
); 

This ticket was mentioned in Slack in #core-customize by obenland. View the logs.


7 years ago

#49 @westonruter
7 years ago

I'm working on fixing the tests and breaking out the _wp_menus_changed() logic into a wp_get_remapped_nav_menu_locations() function which can then be re-used for the Customizer.

#50 @westonruter
7 years ago

Unit tests are fixed now as of 39692.5.diff

Next up: Customizer integration

#51 @westonruter
7 years ago

And 39692.6.diff implements Customizer support for remapping nav menus when theme switching.

Please test.

I'd like to flesh out some more unit tests, like for these Customizer changes.

I'd also like to add Customizer acceptance tests to accompany the non-Customizer acceptance tests in https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations/tree/master/tests/nav-menus

#52 follow-up: @joyously
7 years ago

Would it be possible to test it on the theme preview site?

#53 in reply to: ↑ 52 ; follow-up: @melchoyce
7 years ago

Replying to joyously:

Would it be possible to test it on the theme preview site?

@otto42, do you know if this is possible?

#54 in reply to: ↑ 53 @Otto42
7 years ago

Replying to melchoyce:

@otto42, do you know if this is possible?

The theme previewer is an external tied to trunk. Disconnecting that would be possible, but likely to break things in trying to put it back together later. Help from systems would be needed.

#55 @obenland
7 years ago

Nice work @westonruter, tests out well. I assumed it would be much more involved to add Customizer support.

Can we change wp_get_remapped_nav_menu_locations() to simply wp_map_nav_menu_locations()? And maybe any variable that says remapped to just mapped?

#56 @westonruter
7 years ago

  • Keywords commit added

@obenland ok, 39692.7.diff makes those changes. I've updated the unit tests to make use of the new function as oppose to using the private function and manipulating theme mods and options.

I've also added updated the test runner to test more scenarios, using three methods to switch themes: WP-CLI, themes admin screen, and the Customizer: https://github.com/xwp/wp-theme-nav-menu-widget-sidebar-permutations/blob/master/tests/nav-menus/runner.php

Using headless Chrome to script the activation of a theme via the Customizer and admin screen:

This is good for to you commit from my view, though per above there may be some tweaks to the group slugs.

#57 @westonruter
7 years ago

  • Owner set to obenland
  • Status changed from assigned to reviewing

FYI: The Travis build for the branch is ✅: https://travis-ci.org/xwp/wordpress-develop/builds/262099992

#58 @obenland
7 years ago

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

In 41237:

Map nav menu locations on theme switch

This will send nav menu locations through three levels of mapping:

  1. If both themes have only one location, that gets mapped.
  2. If both themes have locations with the same slug, they get mapped.
  3. Locations that (even partially) match slugs from a similar kind of menu location will get mapped.

Menu locations are mapped for Live Previews in the Customizer and during theme switches.

Props westonruter, obenland, welcher, melchoyce.
Fixes #39692.

#59 @obenland
6 years ago

In 41801:

Menus: Add social as tertiary mapping group

Allows mapping of all social menus that include the term social in their slug.

See #39692.

#60 @obenland
6 years ago

In 41810:

Customizer: Account for legacy menu data

Checks for menu location data from when a previewed theme was previously active.

See #39692.

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


6 years ago

#62 @obenland
6 years ago

In 41995:

Customize: Allow previewed menus to be customized

Fixes a bug where menu assignements couldn't be changed when previewing a theme.
Also removes an unnecessary call to menu mapping after a theme switch from the customizer and makes sure the locations option is always written, for good measure.

Props westonruter.
See #39692.

#63 @obenland
6 years ago

In 42026:

Menus: Add menu-$i slugs to mapping groups

Helps to future proof the feature.
Also orders slugs by popularity to optimize mapping time.

See #39692.

Note: See TracTickets for help on using tickets.