WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 4 months ago

#39665 new defect (bug)

Customize: nav menu fallbacks don't get edit shortcuts

Reported by: celloexpressions Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch needs-testing
Focuses: Cc:

Description

wp_nav_menu() defaults to showing a menu of pages when no menu is set in a location. This is usually the case when a user first switches to a new theme. Since content is shown in the menu, I would expect to see an edit shortcut to be able to change what menu is displayed. But there isn't one.

This is likely a bit of a technical challenge, but should be possible since the menu location is still there. There is a full refresh when switching a location to have an actual menu, so it's likely a matter of adding selective refresh support for menu fallbacks.

Attachments (4)

39665.diff (1.3 KB) - added by dlh 6 months ago.
39665.2.diff (2.1 KB) - added by westonruter 4 months ago.
39665.3.diff (2.3 KB) - added by westonruter 4 months ago.
39665.4.diff (2.9 KB) - added by dlh 4 months ago.

Download all attachments as: .zip

Change History (23)

#1 @celloexpressions
8 months ago

  • Milestone changed from Future Release to 4.7.3

Moving to 4.7.x as this is an issue with functionality in 4.7. I don't have any specific implementation ideas but if anyone does please start exploring and patching.

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


7 months ago

#3 @westonruter
7 months ago

  • Milestone changed from 4.7.3 to 4.7.4

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


6 months ago

@dlh
6 months ago

#5 @dlh
6 months ago

39665.diff is an approach that would apply to only wp_page_menu(): It applies the same filter to that function's markup as is applied to wp_nav_menu().

In my testing with the patch, the edit shortcut appears, and a partial refresh occurs when switching from no-menu to actual-menu, but a full refresh occurs when the opposite happens.

The patch illustrates the idea, but it isn't ready for primetime. @celloexpressions, if you think this is a direction worth pursuing, I can try to flesh it out.

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


6 months ago

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


5 months ago

#8 @swissspidy
5 months ago

  • Milestone changed from 4.7.4 to 4.7.5

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


5 months ago

#10 @desrosj
5 months ago

  • Milestone changed from 4.7.5 to 4.8

@westonruter
4 months ago

#11 @westonruter
4 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

@dlh this seems to work quite well. The only thing I noticed was that if I have a nav menu assigned to a location and then I remove the nav menu assignment, that this was resulting in a fallback refresh. I fixed that in 39665.2.diff.

Is there anything else about it that made you think it wasn't ready for primetime?

#12 @dlh
4 months ago

@westonruter Primarily, that it didn't attempt to provide an edit shortcut for a custom fallback_cb.

I also wasn't sure whether a separate filter_wp_page_menu() callback should be added, even if it did only $args = (object) $args; and then passed everything to filter_wp_nav_menu().

@westonruter
4 months ago

#13 @westonruter
4 months ago

@dlh I don't know, I think the solution you came up is pretty awesome. I don't assume that fallback_cb is specified that frequently, but I could be very wrong.

In any case, I think I have a general solution to this in 39665.3.diff. By wrapping the fallback_cb in another function call, we can then call the logic that injects the selective refresh HTML attributes regardless of whether there is a filter we can hook into. The caveat being that it is currently using a closure which would have to be refactored for PHP 5.2 compat.

Anything I'm missing in this approach?

#14 @dlh
4 months ago

I'll take a look at 39665.3.diff. I should be able to test and post feedback in the next day or so.

#15 follow-up: @dlh
4 months ago

I tested 39665.3.diff and found it generally works as expected. I came up with a few comments and questions:

  • If the fallback_cb doesn't output HTML, then the additional attributes in filter_wp_nav_menu() aren't added to the output.
  • As a UX matter, do you think it makes sense to a user for the edit link to take them somewhere where they weren't editing the thing in the preview? The user would have to understand that they're "editing" the fact that there isn't a menu assigned to the location, as opposed to being able to edit the list of links output by wp_page_menu() or editing whatever the fallback_cb was. While the current nav menu edit links go to the same place as the new ones would, there's at least an "Edit Menu" link.
  • Presumably it's rare, but if someone already happened to provide support for edit links in their fallback_cb, then I think there would be two edit links.

#16 in reply to: ↑ 15 @westonruter
4 months ago

Replying to dlh:

  • If the fallback_cb doesn't output HTML, then the additional attributes in filter_wp_nav_menu() aren't added to the output.

If it doesn't output HTML, then this should still work as expected. If you're talking about returning nothing, then essentially empty out the container. Otherwise, if it is outputting something I understand that the callback provided for fallback_cb needs to accept all of the arguments that wp_nav_menu would accept, since it is in fact passed all of those arguments. So I think it needs to be a compatible function in how it behaves. As a failsafe, I suppose we could modify \WP_Customize_Nav_Menus::filter_wp_nav_menu() to make sure that it begins with an HTML start tag.

  • As a UX matter, do you think it makes sense to a user for the edit link to take them somewhere where they weren't editing the thing in the preview? The user would have to understand that they're "editing" the fact that there isn't a menu assigned to the location, as opposed to being able to edit the list of links output by wp_page_menu() or editing whatever the fallback_cb was. While the current nav menu edit links go to the same place as the new ones would, there's at least an "Edit Menu" link.

The edit link will take them to the related nav menu location control, right? It would take them to a select dropdown that is unselected. The user would then see that they'd need to supply a menu. If no menus exist yet, then I think that's where #40104 comes into play.

  • Presumably it's rare, but if someone already happened to provide support for edit links in their fallback_cb, then I think there would be two edit links.

I think that would be rare indeed.

Otherwise, if all that checks out, the next thing needed is to refactor the patch to be compatible with PHP 5.2.

@dlh
4 months ago

#17 @dlh
4 months ago

39665.4.diff refactors the previous patch so that it doesn't use a closure.

One observation about the change to customize-preview-nav-menus.js: It seems to cause the fallback to potentially display in the preview when it wouldn't otherwise. You can see this by previewing Twenty Seventeen after applying the patch.

In Twenty Seventeen, when no menu is assigned to the Top Menu location, the fallback doesn't display because header.php checks has_nav_menu() before calling wp_nav_menu(). But with the change to the JS refresh() function, the fallback still displays in the Customizer.

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


4 months ago

#19 @obenland
4 months ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.