When $depth argument is used in wp_nav_menu last level menu items still have .menu-item-has-children class, even though they are not shown

[slobodanmanic]

Example:

• Set wp_nav_menu $depth to 3
• A theme has some CSS to indicate submenus (e.g. Twenty Fourteen)
• If last level (as allowed by $depth argument) items still have children, they will not be shown, but .menu-item-has-children class is still there

I don't think the class itself is the problem, because the menu item does have children, they're just not being displayed. It might be good to add a new class that would help theme developers disable submenu indicator for menu items in which $depth prevents sub-items to be shown.

Maybe something like this (in nav-menu-template.php):

function start_lvl( &$output, $depth = 0, $args = array() ) {
	$indent = str_repeat("\t", $depth);
	$sub_menu_class = ( $depth + 2 == $args->depth ? 'submenu menu-last-level-parent' : 'submenu' );
	$output .= "\n$indent<ul class=\"$sub_menu_class\">\n";
}

This would allow .menu-last-level-parent li selector to disable dropdown indicator, since the indicator is not needed when sub-items are not shown.

$depth +2 feels hacky to me, but this is by far the easiest way to add the class. Ideally, class would be added to last level <li> elements, but I think that would require updates to Walker class.

[SergeyBiryukov]

Introduced in [25602].

[slobodanmanic]

Worth noting that this can be achieved by using .menu li li li li type selectors, but last level class would be cleaner.

[kucrut]

Here's one approach:

public function start_el( &$output, $item, $depth = 0, $args = array(), $id = 0 ) {
	$indent = ( $depth ) ? str_repeat( "\t", $depth ) : '';

	$classes = empty( $item->classes ) ? array() : (array) $item->classes;
	$classes[] = 'menu-item-' . $item->ID;

	if ( ( -1 === $args->depth ) || ( $args->depth > 0 && ( ( $depth + 1 ) === $args->depth ) ) ) {
		$_classes = array_fill_keys( $classes, '' );
		unset( $_classes['menu-item-has-children'] );
		$classes = array_keys( $_classes );
	}

	// ...

If only nav_menu_css_class is passing the current $depth then all this could be done with a simple filter callback.

I'll be happy to format this as a patch if it's considered the right approach.

[chriscct7]

Good First Bug: Attempt to verify example is still valid on latest stable

[iCaspar]

Here's the patch based on @kucrut's suggestion above. It seems that removing the .menu-item-has-children route is the better one for two reasons:

1. Seems more backwards-compatible, as themes won't have to update their menu CSS to take into account the addition of a new class, and
2. Even though the menu item does have children in terms of the menu walker, because the depth has been limited, they won't ever see the light of day on the front end. At that point there's no use in having a CSS class that says an item on the page has children when it doesn't.

#wcus

[mdgl]

Patch refreshed. This should perhaps even be categorised as a "bug" rather than an "enhancement". Note that a similar issue exists within class Walker_Page and likely other "walker" classes. These remain unpatched.

[mdgl]

Alternative (preferable?) patch

[mdgl]

Alternative patch added, which avoids adding the incorrect class names at source. This might be considered preferable. It will use a little more time/space but this shouldn't be a problem as most menus will be (relatively) short and shallow. Also re-categorised ticket as a "bug".

[SergeyBiryukov]

Moving to 6.1, as there was some interest in testing the patches during the latest new core contributors meeting.

[petitphp]

Refresh of 28620-alt.diff

[petitphp]

The alternative patch looks good to me, I've refreshed it against trunk in 28620-alt.2.diff.

Using a simple menu with two level of links and displaying it with twentytwenty theme :

<ul id="menu-main-menu-2" class="menu">
	<li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-has-children menu-item-5">
		<a href="">Sample Page</a>
		<span class="icon"></span>
		<ul class="sub-menu">
			<li class="menu-item menu-item-type-post_type menu-item-object-post menu-item-has-children menu-item-6">
				<a href="">Hello world!</a>
				<span class="icon"></span>
				<ul class="sub-menu">
					<li class="menu-item menu-item-type-taxonomy menu-item-object-category menu-item-8">
						<a href="">Uncategorized</a>
					</li>
				</ul>
			</li>
		</ul>
	</li>
</ul>

When displaying the same menu with a custom depth argument :

Before the patch

The last element displayed has the menu-item-has-children class.

<div class="menu-main-menu-container">
	<ul id="menu-main-menu-3" class="menu">
		<li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-has-children menu-item-5">
			<a href="">Sample Page</a><span class="icon"></span>
			<ul class="sub-menu">
				<li class="menu-item menu-item-type-post_type menu-item-object-post **menu-item-has-children** menu-item-6">
					<a href="">Hello world!</a>
					<span class="icon"></span>
				</li>
			</ul>
		</li>
	</ul>
</div>

After the patch

The class menu-item-has-children is not present anymore

<div class="menu-main-menu-container">
	<ul id="menu-main-menu-3" class="menu">
		<li class="menu-item menu-item-type-post_type menu-item-object-page menu-item-has-children menu-item-5">
			<a href="">Sample Page</a>
			<span class="icon"></span>
			<ul class="sub-menu">
				<li class="menu-item menu-item-type-post_type menu-item-object-post menu-item-6">
					<a href="">Hello world!</a>
				/li>
			</ul>
		</li>
	</ul>
</div>

So working as expected.

I've also looked at the navigation block in Gutenberg but it doesn't seem to use the existing code to render menu items so not sure if this patch will affect it.

[audrasjb]

As per today's new contributors meeting, self assigning for final testing/review/commit.

[audrasjb]

Before patch: class added when using the depth attribute, even if the element doesn't have any child.

[audrasjb]

After patch: works fine

[audrasjb]

The last patch works fine on my side (see the above before/after screenshots).

As per today's new contributor meeting, adding needs-unit-tests, as recommended by Sergey.

[petitphp]

Add unit tests for the change

[petitphp]

I've created unit tests for this change as discussed during the new contributor meeting.

Trac ticket: https://core.trac.wordpress.org/ticket/28620

[audrasjb]

Thanks @petitphp! I built a PR containing your patch to facilitate peer review.

Pinging @costdev: if you have a chance, it would be wonderful if you could have a look on the unit tests in the above PR ✌️

With 6.1 RC1 approaching, this ticket only has a few hour to get committed (or punted to 6.2). Anyone available to address the last remaining change requested?

@audrasjb Sure, I can take care of the remaining change.

Trac ticket: https://core.trac.wordpress.org/ticket/28620

This PR contain additional changes request on the previous PR #3357 in order to be ready for merging in trunk.

@audrasjb I've made a new PR #3435 containing the remaining changes.

[audrasjb]

Looks great, thanks for handling this @petitphp ✌️

closing in favor of #3435

[audrasjb]

In 54478:

Menus: Remove .menu-item-has-children on wp_nav_menu last level menu items when $depth arg is used.

This changeset prevents wp_nav_menu last level menu items from having the .menu-item-has-children class when the $depth argument is used. It adds a loop in wp_nav_menu() to calculate the depth of each menu item with children to make sure the class is added only when applicable.

Props slobodanmanic, kucrut, iCaspar, mdgl, petitphp, audrasjb, costdev.
Fixes #28620.

committed in https://core.trac.wordpress.org/changeset/54478

[antonvlasenko]

This ​commit caused this error on one of the WordPress websites:

Warning: Undefined array key 3807 in /homepages/24/d4296231672/htdocs/wordpress/wp-includes/nav-menu-template.php on line 210

Please see ​https://wordpress.org/support/topic/undefined-array-key-3807 for additional context.
In my opinion, this ticket must be reopened, and a 6.1.1 milestone should be added because this is a WordPress 6.1 bug.
FYI: @audrasjb @slobodanmanic

[peterwilsoncc]

In 54801:

Menus: Apply menu-item-has-children class in sub-menus.

Ensure the menu-item-has-children class is added to sub-menu items when wp_nav_menu() is called with the depth parameter specified to a non-zero value.

Follow up to [54478].

Props davidvongries, fpodhorsky, hellofromTonya, innovext, larsmqller, LeonidasMilossis, mattkeys, mukesh27, nuvoPoint, ocean90, outrankjames, petitphp, SergeyBiryukov, sippis, webmandesign.
Fixes #56946.
See #28620.

[desrosj]

In 54809:

Menus: Apply menu-item-has-children class in sub-menus.

Ensure the menu-item-has-children class is added to sub-menu items when wp_nav_menu() is called with the depth parameter specified to a non-zero value.

Follow up to [54478].

Props davidvongries, fpodhorsky, hellofromTonya, innovext, larsmqller, LeonidasMilossis, mattkeys, mukesh27, nuvoPoint, ocean90, outrankjames, petitphp, SergeyBiryukov, sippis, webmandesign, peterwilsoncc.
Merges [54801] to the 6.1 branch.
Fixes #56946.
See #28620.

[peterwilsoncc]

In 54999:

Menus: Prevent infinite loop in menus.

This modifies how the menu-item-has-children class is removed from bottom level menu items. Instead of removing the class within wp_nav_menu() a filter is applied to the nav_menu_css_class hook to remove the class as required.

Introduces wp_nav_menu_remove_menu_item_has_children_class() for removing the class.

Reverts source code changes in [54478,54801], the tests are retained.

Props davidbinda, SergeyBiryukov, mhkuu, JeffPaul, jmdodd, priethor, desrosj, hellofromTonya, azaozz, peterwilsoncc.
Fixes #56926.
See #28620.

[peterwilsoncc]

In 55005:

Menus: Account for legacy calls to nav_menu_css_class filter.

Modify wp_nav_menu_remove_menu_item_has_children_class() to account for changes to the nav_menu_css_class filter since it's introduction.

The $args and $depth parameters were added after the filter's introduction so this protects against fatal errors in custom walkers applying the filter in a legacy format.

Without the $args or $depth parameters, wp_nav_menu_remove_menu_item_has_children_class() no longer attempts to remove the menu-item-has-children from the lowest level menu items as these are required to determine the current branch the walker is walking.

Follow up to [54999].

Props dd32, azaozz, peterwilsoncc.
See #56926, #28620.