WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#38992 closed defect (bug) (fixed)

Twenty Seventeen: Menu toggle doesn't work after adding a new menu item in customizer

Reported by: ocean90 Owned by: davidakennedy
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-patch commit dev-reviewed
Focuses: javascript Cc:

Description

If you add a new menu item the mobile "Menu" button doesn't toggle the menu anymore. See attached screencast.

I found this in Twenty Fifteen, see [33491] and [32807], but not in Twenty Sixteen where it's working too.

Attachments (2)

38992-menu-toggle.gif (994.9 KB) - added by ocean90 4 months ago.
38992.patch (1.1 KB) - added by laurelfulford 4 months ago.

Download all attachments as: .zip

Change History (11)

#1 @westonruter
4 months ago

@ocean90 it works in Twenty Sixteen without that JS snippet because Twenty Sixteen handles the dropdowns with pure CSS. There's no need to re-attach any event listeners.

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


4 months ago

#3 @ocean90
4 months ago

  • Keywords needs-patch added; has-patch removed

#4 @afercia
4 months ago

About this line in navigation.js:

$( siteNavigation.closest( '.main-navigation' ), this ).toggleClass( 'toggled-on' );
  1. not sure why this is passed as context, since this is the clicked button
  2. I'd just declare and get the main navigation element earlier in the function

Couple line below:

$( this )
	.add( siteNavigation )
	.attr( 'aria-expanded', $( this ).add( siteNavigation ).attr( 'aria-expanded' ) === 'false' ? 'true' : 'false' );

I'm not sure there's the need to set aria-expanded also on the siteNavigation, that's the menu <ul> element that is not focusable or directly available to assistive technologies so aria-expanded seems a bit pointless to me.

As a general consideration for navigation.js, not sure why there are two IIFEs nested within the main one.

#5 @laurelfulford
4 months ago

  • Keywords has-patch added; needs-patch removed

In 38992.patch, I applied @afercia's suggestion of simplifying the line $( siteNavigation.closest( '.main-navigation' ), this ).toggleClass( 'toggled-on' );, which also appears to fix the reported issue with the menu toggle.

#6 @davidakennedy
4 months ago

  • Keywords commit added

I tested this, and it works well and fixes the issue. Should be good to go.

#7 @helen
4 months ago

  • Keywords dev-reviewed added

#8 @davidakennedy
4 months ago

  • Owner set to davidakennedy
  • Resolution set to fixed
  • Status changed from new to closed

In 39419:

Twenty Seventeen: Fix broken menu toggle in Customizer after menu items are added

This simplifies the line $( siteNavigation.closest( '.main-navigation' ), this ).toggleClass( 'toggled-on' ); to $( siteNavContain ).toggleClass( 'toggled-on' );, since this is the clicked button, so the extra context isn't needed.

Props afercia, laurelfulford.

Fixes #38992.

#9 @helen
4 months ago

In 39423:

Twenty Seventeen: Fix broken menu toggle in Customizer after menu items are added

This simplifies the line $( siteNavigation.closest( '.main-navigation' ), this ).toggleClass( 'toggled-on' ); to $( siteNavContain ).toggleClass( 'toggled-on' );, since this is the clicked button, so the extra context isn't needed.

Props afercia, laurelfulford.

Fixes #38992 for the 4.7 branch.

Note: See TracTickets for help on using tickets.