Make WordPress Core

Opened 9 years ago

Closed 8 months ago

Last modified 6 months ago

#28620 closed defect (bug) (fixed)

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

Reported by: slobodanmanic's profile slobodanmanic Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.7
Component: Menus Keywords: good-first-bug has-patch has-unit-tests commit
Focuses: ui Cc:

Description

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.

Attachments (7)

28620.diff (943 bytes) - added by iCaspar 8 years ago.
28620.2.diff (680 bytes) - added by mdgl 7 years ago.
28620-alt.diff (1.8 KB) - added by mdgl 7 years ago.
Alternative (preferable?) patch
28620-alt.2.diff (1.8 KB) - added by petitphp 11 months ago.
Refresh of 28620-alt.diff
before-patch.png (328.5 KB) - added by audrasjb 11 months ago.
Before patch: class added when using the depth attribute, even if the element doesn't have any child.
after-patch.png (340.1 KB) - added by audrasjb 11 months ago.
After patch: works fine
28620-alt.3.diff (6.0 KB) - added by petitphp 10 months ago.
Add unit tests for the change

Download all attachments as: .zip

Change History (45)

#1 @slobodanmanic
9 years ago

  • Version changed from trunk to 3.7

#2 @slobodanmanic
9 years ago

  • Summary changed from When $depth argument is used in wp_nav_menu last level menu items still have .menu-item-has-children class to 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

#3 @SergeyBiryukov
9 years ago

Introduced in [25602].

#4 @slobodanmanic
9 years ago

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

#5 @kucrut
9 years ago

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.

#6 @wonderboymusic
9 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#7 @chriscct7
8 years ago

  • Keywords good-first-bug needs-testing added

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

@iCaspar
8 years ago

#8 @iCaspar
8 years ago

  • Keywords has-patch added; needs-patch removed

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

Last edited 8 years ago by iCaspar (previous) (diff)

#9 @chriscct7
8 years ago

  • Owner set to chriscct7
  • Status changed from new to reviewing

@mdgl
7 years ago

#10 @mdgl
7 years ago

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
7 years ago

Alternative (preferable?) patch

#11 @mdgl
7 years ago

  • Type changed from enhancement to defect (bug)

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".

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


12 months ago

#13 @SergeyBiryukov
12 months ago

  • Milestone set to Future Release

#14 @SergeyBiryukov
12 months ago

  • Milestone changed from Future Release to 6.1

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

@petitphp
11 months ago

Refresh of 28620-alt.diff

#15 @petitphp
11 months ago

  • Keywords needs-testing removed

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.

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


11 months ago

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


11 months ago

#18 @audrasjb
11 months ago

  • Keywords needs-testing added

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

#19 @audrasjb
11 months ago

  • Owner changed from chriscct7 to audrasjb

@audrasjb
11 months ago

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

@audrasjb
11 months ago

After patch: works fine

#20 @audrasjb
11 months ago

  • Keywords needs-unit-tests added; needs-testing removed

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
10 months ago

Add unit tests for the change

#21 @petitphp
10 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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

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


9 months ago

#24 @audrasjb
8 months ago

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 ✌️

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


8 months ago

audrasjb commented on PR #3357:


8 months ago
#26

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?

petitphp commented on PR #3357:


8 months ago
#27

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

This ticket was mentioned in PR #3435 on WordPress/wordpress-develop by petitphp.


8 months ago
#28

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.

petitphp commented on PR #3357:


8 months ago
#29

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

#30 @audrasjb
8 months ago

  • Keywords commit added

Looks great, thanks for handling this @petitphp ✌️

audrasjb commented on PR #3357:


8 months ago
#31

closing in favor of #3435

#32 @audrasjb
8 months ago

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

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.

#34 @antonvlasenko
7 months ago

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

Last edited 7 months ago by antonvlasenko (previous) (diff)

#35 @peterwilsoncc
7 months ago

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.

#36 @desrosj
7 months ago

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.

#37 @peterwilsoncc
6 months ago

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.

#38 @peterwilsoncc
6 months ago

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.

Note: See TracTickets for help on using tickets.