Twenty Twenty-One: Primary menu toggle filter adds toggles to third party menu locations

[domainsupport]

The code in /inc/menu-functions.php ...

<?php
function twenty_twenty_one_add_sub_menu_toggle( $output, $item, $depth, $args ) {
        if ( 0 === $depth && in_array( 'menu-item-has-children', $item->classes, true ) ) {

                // Add toggle button.
                $output .= '<button class="sub-menu-toggle" aria-expanded="false" onClick="twentytwentyoneExpandSubMenu(this)">';
                $output .= '<span class="icon-plus">' . twenty_twenty_one_get_icon_svg( 'ui', 'plus', 18 ) . '</span>';
                $output .= '<span class="icon-minus">' . twenty_twenty_one_get_icon_svg( 'ui', 'minus', 18 ) . '</span>';
                $output .= '<span class="screen-reader-text">' . esc_html__( 'Open menu', 'twentytwentyone' ) . '</span>';
                $output .= '</button>';
        }
        return $output;
}
add_filter( 'walker_nav_menu_start_el', 'twenty_twenty_one_add_sub_menu_toggle', 10, 4 );

... should be updated so it doesn't affect third party menu locations.

Perhaps by using ...

<?php
if (isset($args->theme_location) && $args->theme_location === 'primary') {
...
}

Thank you.

Oliver

[mukesh27]

I like the approach of @domainsupport. We can marge theme location condition in if statement like below.

if ( isset( $args->theme_location ) && 'primary' === $args->theme_location && 0 === $depth && in_array( 'menu-item-has-children', $item->classes, true ) ) {

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

[mukesh27]

Ping @poena for thoughts and progress for this ticket.

[poena]

Hi @mukesh27, do you want to create a new pull request?

[domainsupport]

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

The easiest way is to use a legacy Navigation Menu widget as follows ...

Steps to Reproduce

1. Install WordPress.
2. Activate Twenty Twenty-One theme
3. Install and activate "Classic Widgets" plugin
4. Create a new Menu with a parent menu item and child menu item
5. Add a legacy "Navigation Menu" widget to the Footer widget area and select your menu
6. View the front page and examine the footer widget HTML
7. 🐞 You will see there is a button element that shouldn't be there ...

<button class="sub-menu-toggle" aria-expanded="false" onclick="twentytwentyoneExpandSubMenu(this)"><span class="icon-plus"><svg class="svg-icon" width="18" height="18" aria-hidden="true" role="img" focusable="false" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg"><path fill-rule="evenodd" clip-rule="evenodd" d="M18 11.2h-5.2V6h-1.6v5.2H6v1.6h5.2V18h1.6v-5.2H18z" fill="currentColor"></path></svg></span><span class="icon-minus"><svg class="svg-icon" width="18" height="18" aria-hidden="true" role="img" focusable="false" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg"><path fill-rule="evenodd" clip-rule="evenodd" d="M6 11h12v2H6z" fill="currentColor"></path></svg></span><span class="screen-reader-text">Open menu</span></button>

Expected Results

When testing a patch to validate it works as expected:

• ✅ No button element should be appended to the menu outside of the primary menu location

When reproducing a bug:

• ❌ button element appears

Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.

[karmatosed]

Looks to me based on testing we need a patch refresh if I am not mistaken? I am going to put that keyword on for now.

[domainsupport]

Replying to karmatosed:

Looks to me based on testing we need a patch refresh if I am not mistaken? I am going to put that keyword on for now.

I've just re-created the patch and re-uploaded. Is that what you wanted?

[karmatosed]

@domainsupport It is, thank you. I have now removed refresh and we need to get it tested.

[shailu25]

Patch Updated

[shailu25]

Updated Patch. (Removed Extra space in previous patch)

[poena]

Between the three patches, I only see changes to the spacing. What change was expected from the requested refresh?

I have tested https://core.trac.wordpress.org/attachment/ticket/52694/52694.patch
on WordPress 6.6-alpha-57778-src and it solves the problem.

Personally, I prefer wrapping this long line in two, it is easier for me to read. But I don't consider it a blocker unless the line length is against the coding standards.

[SergeyBiryukov]

Replying to poena:

Personally, I prefer wrapping this long line in two, it is easier for me to read.

Yes, there's no official rule about line length as far as I remember, but I prefer shorter lines too, otherwise it increases cognitive load for me.

There was a “Complex control structure conditions” section in ​this proposal:

A control structure statement with multiple conditions can make for very long lines making code harder to read.

It is encouraged (but not enforced) that these long conditions are split into multiple lines.

When a long condition statement is split over multiple lines, the following (new) rules are proposed in addition to the existing rules for control structure formatting:

1. The first condition should be on the same line as the control structure keyword.
2. The closing parenthesis for the control structure should be on a new line, indented the same as the control structure keyword and followed by a space and the opening curly brace of the control structure.
3. When splitting the conditions into multiple lines, the boolean/logical operators separating the statements should always be placed at the start of the new line. This makes for smaller change-sets and improves readability.

Note: even with conditions spread out over multiple lines, multiple conditions per line will still be allowed as long as the condition grouping makes sense.

So I would rewrite the code in question like this:

if ( isset( $args->theme_location ) && 'primary' === $args->theme_location
	&& 0 === $depth && in_array( 'menu-item-has-children', $item->classes, true )
) {

[SergeyBiryukov]

Replying to SergeyBiryukov:

So I would rewrite the code in question like this

It appears that the isset( $args->theme_location ) check is redundant here, as theme_location is one of the defaults in wp_nav_menu() and would always be present. All of the filters attached to walker_nav_menu_start_el in other bundled themes just check $args->theme_location directly, so the same should be enough here:

if ( 'primary' === $args->theme_location
	&& 0 === $depth && in_array( 'menu-item-has-children', $item->classes, true )
) {

[SergeyBiryukov]

In 58293:

Twenty Twenty-One: Only add the sub-menu toggle button to the primary menu.

Follow-up to [49216].

Props domainsupport, mukesh27, poena, karmatosed, shailu25, SergeyBiryukov.
Fixes #52694.