#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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (45)
#2
@
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
#4
@
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
@
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
@
9 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#7
@
8 years ago
- Keywords good-first-bug needs-testing added
Good First Bug: Attempt to verify example is still valid on latest stable
#8
@
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:
- 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
- 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
#10
@
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.
#11
@
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
#14
@
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.
#15
@
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
@
11 months ago
- Keywords needs-testing added
As per today's new contributors meeting, self assigning for final testing/review/commit.
@
11 months ago
Before patch: class added when using the depth
attribute, even if the element doesn't have any child.
#20
@
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.
#21
@
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
This ticket was mentioned in PR #3357 on WordPress/wordpress-develop by audrasjb.
8 months ago
#23
Trac ticket: https://core.trac.wordpress.org/ticket/28620
#24
@
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
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?
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.
8 months ago
#33
committed in https://core.trac.wordpress.org/changeset/54478
#34
@
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
Introduced in [25602].