Navigation: Explore using Walker_Nav_Menu

[obenland]

Description

Exploratory PR to see what's needed in order to be able to use Walker_Nav_Menu to render site navigation menu blocks.

See #17194.

How has this been tested?

So far only locally, by setting up a menu block and loading it in the front-end.

Types of changes

New feature.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[obenland]

Have we considered saving navigation block data in the proper post types? Is that something Gutenberg is even capable of?

It seems like re-using Core's built-in menu post types would alleviate many of these workarounds.

[obenland]

Without using Core's nav_menu_item post type, I think this as good as it'll get. I tried using wp_nav_menu() and filtering the supplier functions to render the right menu items, but that does not seem to work like we'd need it to.

[retrofox]

super minor: can you change that trash that I left there?

// Background color - has background color CSS class.

[retrofox]

We need to define the name of the attribute used to store the menu item link.

[draganescu]

hi @obenland !

about this I clearly remember a lot of discussion about how the menu block saves its menus and that we don't want to use custom post types for that. Instead we save it as all the other blocks are saved.

[draganescu]

hi :) could you make this PR or another strictly about the walker without the beautification of the customisation code? :)

[frontdevde]

Would it be possible to add a class to the top-level menu items when they have children? For the Frontend default styles suggested by Shaun (see #18094) we need it for the indicator arrows that show to the right of the menu. In the reference theme Varia the class menu-item-has-children is added (in the PR I named it has-sub-menu for now).

[obenland]

could you make this PR or another strictly about the walker without the beautification of the customisation code?

There's no beautification, the data format needs adjustment to fit with walk_nav_menu_tree() and I updated the keys to closer align with the nomenclature of wp_setup_nav_menu_item().

Would it be possible to add a class to the top-level menu items when they have children? […] In the reference theme Varia the class menu-item-has-children is added

Sure, but really what we should be doing is use wp_nav_menu() (where this class is being added in Core), which brings us back to #18071 (comment)

I clearly remember a lot of discussion about how the menu block saves its menus and that we don't want to use custom post types for that.

Is there a record of this discussion somewhere that I can read, so I can better understand the rationale behind that decision?

[retrofox]

The current implementation doesn't respect the menu hierarchy, doesn't pay attention to the nesting.
The following pictures show how the same menu is rendered on master (correct) and how it is done on this PR.

master

try/nav-menu-walker

If I'm not mistaken, it's done through a recursive call.

[draganescu]

@obenland sorry about the beautification comment I didn't pay attention to the effect of the changes 💯 👍
I'll see asap if I can direct you to places discussing saving menus.

[draganescu]

On using a WP_Walker for the navigation menu

One of the roles of the WP_Walker is to allow themes to customise the HTML and classes for the WordPress menus.

Is this navigation menu block going to be used to replace the current admin menu building page? If so, then a WP_Walker will be needed to maintain compatibility to old themes.

If not, then in a FSE environment there is no reason to have a Walker for anything, as the HTML structure that the block outputs is something that the FSE environment needs to count on, and not be available for change by themes at run time.

Just to be very clear, I don't think using a walker to render the navigation menu block is bad. In fact that is how I originally implemented the server side rendering of the navigation block. But then I realised that it is only needed if we'll use it to replace how all the menus are created in WordPress, which might not be the case at all.

Also, for FSE ready themes, I believe bringing in CSS frameworks won't be a possibility (another reason for the ability to customize the block's HTML), as both the FSE system and the CSS framework expect a certain HTML structure, and they will conflict.

Plus, the navigation block would be the only block that allows interference with its HTML because of rendering with a walker.

So just checking what is the right path here, @mtias

[retrofox]

Although it changes how the styles are applied in the markup we will need to change asap according to what we decide but in another issue/PR.
Just FYI, I leave two screenshots (again :-) )

master

try/nav-menu-walker

[obenland]

Yeah, unfortunately Walker_Nav_Menu only allows us to set an id or class on the <li>, no other attributes. Together with the styles on the <nav> level, that should still work though?

In my testing I've not experienced any padding/margin changes, could those come from #18094?

[noisysocks]

Is this navigation menu block going to be used to replace the current admin menu building page? If so, then a WP_Walker will be needed to maintain compatibility to old themes.

If not, then in a FSE environment there is no reason to have a Walker for anything, as the HTML structure that the block outputs is something that the FSE environment needs to count on, and not be available for change by themes at run time.

Adding Needs Decision here—I think we need an answer to this before proceeding.

[obenland]

I think this experiment is as close as it gets without using Core's menu item post type. That is what we should really be exploring, though. I'm not sure using the Walker makes too much sense without it.

I'm going to close this for now, hoping we'll get a chance to add an enhancement layer for WordPress uses of Gutenberg, to better integrate with the existing system.