Partial refresh <div> in menu customizer breaks some theme designs

[greenshady]

I originally reported this in #32576. See entire discussion here: https://core.trac.wordpress.org/ticket/32576#comment:17

With one of my themes that has a hidden menu by default, the menu is opened and messes up the theme design with the change (screenshot attached). This happens due to this <div> that gets injected:

<div id="partial-refresh-menu-container-1" class="partial-refresh-menu-container" data-instance-number="1">

Injecting an additional <div> has potential to cause some unintended consequences with theme designs.

It was suggested that we add the attributes directly to the menu container instead of adding the <div>.

[westonruter]

​https://github.com/xwp/wordpress-develop/pull/96

[westonruter]

@greenshady: Please try 32841.diff​:

• Eliminate additional wrapper container around partial-refreshable nav menus.
• Do full refresh of preview when nav menu unassigned.

[westonruter]

Additional changes: ​https://github.com/xwp/wordpress-develop/compare/a07767d...dd73dcf

[westonruter]

Updated patch 32841.2.diff​ fixes a couple other issues:

• Limit customizer settings sent in partial refresh to just those related to menu.
• Fix partial refresh of nav menu widget by resolving args hash problem.

[greenshady]

It's close. It fixes the primary issue I opened this ticket for.

However, the issue with the patch is that it wipes JS-added attributes from the element when adding/removing items. For example, there's a style="display:none;" or style="display:block;" on the element there, which gets wiped. Same with a jQuery-added .open or .close class.

Here's the original:

<div class="wrap partial-refreshable-nav-menu-1 partial-refreshable-nav-menu open" style="display:block;">

After adding/removing an item, it becomes this:

<div class="wrap partial-refreshable-nav-menu-1 partial-refreshable-nav-menu">

The open class and style attribute is gone.

[westonruter]

Replying to greenshady:

It's close. It fixes the primary issue I opened this ticket for.

However, the issue with the patch is that it wipes JS-added attributes from the element when adding/removing items. For example, there's a style="display:none;" or style="display:block;" on the element there, which gets wiped. Same with a jQuery-added .open or .close class.

Here's the original:

<div class="wrap partial-refreshable-nav-menu-1 partial-refreshable-nav-menu open" style="display:block;">

After adding/removing an item, it becomes this:

<div class="wrap partial-refreshable-nav-menu-1 partial-refreshable-nav-menu">

The open class and style attribute is gone.

I don't think there's anything that Core can do to support this other than what has already been done. Take a look at [32807] for a change made to Twenty Fifteen to re-initialize the JS-added behaviors once a menu has been partially-refreshed. So for your theme to support partial refresh, you'd need to include the same:

jQuery( document ).on( 'customize-preview-menu-refreshed', function( e, params ) {
    /* params.wpNavArgs has info on which menu item is being refreshed re-initialize menus */ 
} );

The question then remains as to whether themes should explicitly say they support partial-refresh for menus in the Customizer, or if it can be enabled by default. Up until now it has seemed that the vast majority of themes work fine with it enabled by default. Thoughts?

BTW, I think the properties on this params object may need to be revisited. Like, in addition to the wpNavArgs object it would be handy to have the actual container for the nav menu element that was injected.

[greenshady]

I don't think there's anything that Core can do to support this other than what has already been done. Take a look at [32807] for a change made to Twenty Fifteen to re-initialize the JS-added behaviors once a menu has been partially-refreshed. So for your theme to support partial refresh, you'd need to include the same:

Awesome! Knowing the JS to do that will be helpful. That should be all I need.

The question then remains as to whether themes should explicitly say they support partial-refresh for menus in the Customizer, or if it can be enabled by default. Up until now it has seemed that the vast majority of themes work fine with it enabled by default. Thoughts?

I haven't tested with enough themes to know. But, I'd guess enabled by default would be fine. There should be some docs made available on how to switch to something different though. I'm guessing this is handled something like:

$wp_customize->get_setting( 'the_nav_menu_setting_id' )->transport = 'refresh';

BTW, I think the properties on this params object may need to be revisited. Like, in addition to the wpNavArgs object it would be handy to have the actual container for the nav menu element that was injected.

I'm not familiar enough with the code to reply to this.

[westonruter]

Replying to greenshady:

BTW, I think the properties on this params object may need to be revisited. Like, in addition to the wpNavArgs object it would be handy to have the actual container for the nav menu element that was injected.

I'm not familiar enough with the code to reply to this.

So what this would mean is that your event handler you could reference params.container as the jQuery object for the new menu being inserted into the document. But in your case, it looks like you'd also want to be able to have the old container available too, so you could persist the open state, for instance:

jQuery( document ).on( 'customize-preview-menu-refreshed', function( e, params ) {
    if ( params.oldContainer.hasClass( 'open' ) ) {
        params.newContainer.addClass( 'open' );
    } else if ( params.oldContainer.hasClass( 'close' ) ) {
        params.newContainer.addClass( 'close' );
    }
    params.newContainer.css( 'display', params.oldContainer.css( 'display' ) );
} );

Let me know what you think about that, and how best to facilitate dynamic menus.

The additional change I'm proposing for the triggered event is:

src/wp-includes/js/customize-preview-nav-menus.js

@@ -228 +228 @@
                         previousContainer.replaceWith( container );
                         eventParam = {
                                 instanceNumber: instanceNumber,
-                                wpNavArgs: wpNavArgs
+                                wpNavArgs: wpNavArgs,
+                                oldContainer: previousContainer,
+                                newContainer: container
                         };
                         container.removeClass( 'customize-partial-refreshing' );
                         $( document ).trigger( 'customize-preview-menu-refreshed', [ eventParam ] );

[greenshady]

Gotcha. Yes, that sounds good. I'd probably need access to the old container.

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/c94de06d352f7a103e7f9af7918bc9c4df64a9bc

[westonruter]

Additional change: ​https://github.com/xwp/wordpress-develop/commit/c75bdcfb6b805ec520fcd254a10f3c980b9017ea

[westonruter]

Replying to greenshady:

Gotcha. Yes, that sounds good. I'd probably need access to the old container.

Thanks. Could you give 32841.4.diff​ a spin with your theme?

To summarize the changes in the patch:

• Remove additional wrapper around wp_nav_menu()'s normal menu container, replacing nav menu
• Send newContainer and oldContainer in params sent via the customize-preview-menu-refreshed event.
• Fix partial refresh for custom menu widget (which was failing due to a hash mismatch).
• Harden partial refresh, ensuring a full preview refresh is done when all of the necessary conditions are met.
• Improve performance of partial refresh by only sending settings that are relevant to the given menu.
• Refresh the preview when a partial refresh response returns no data, as this means the menu is no longer assigned to the location and the page may need to be reflowed.

[greenshady]

New patch works beautifully! That plus the JS you posted above in my theme is working great.

[westonruter]

Thanks!

@ocean90 could you do a code review for me?

[tywayne]

Replying to westonruter:

Thanks. Could you give 32841.4.diff​ a spin with your theme?

In testing the .4 patch, I found a situation where the preview refreshes infinitely. Reverting the patch removes the bug. Not sure what exactly is causing it, but does seem to be related to the most recent patch.

It is possible it happens elsewhere, but these are the steps taken to reproduce.

1. Start with no menus assigned to any location.
2. Assign a menu to a location.
3. Add a new item to that menu.

gif of steps here: ​https://cloudup.com/ckOEuyoHiai

[westonruter]

Replying to tywayne:

In testing the .4 patch, I found a situation where the preview refreshes infinitely. Reverting the patch removes the bug. Not sure what exactly is causing it, but does seem to be related to the most recent patch.

I don't think it is unique to this patch. It was reported as a problem on trunk as well in #32894. I've never been able to reproduce the issue myself.

[tywayne]

Replying to westonruter:

I don't think it is unique to this patch. It was reported as a problem on trunk as well in #32894. I've never been able to reproduce the issue myself.

To double/triple check, I tested on two additional sites, with one of them being a fresh checkout. In all instances I encounter the infinite refresh after applying this patch and following the steps listed/shown above. Reverting the patch reverts the issue.

While it is possible that it is not unique to this patch, something about this patch regularly reveals the issue in a new context and should be addressed.

[westonruter]

Combining latest patch with patch for #32894.

[westonruter]

I've figured out the infinite refresh problem and added a patch on #32894. I've merged the changes into the patch for this ticket: please test 32841.4.with-32894.diff​.

[tywayne]

I can confirm the latest patch resolves the infinite refresh issue here, thanks!

[westonruter]

In 33138:

Customizer: Remove additional wrapper element around wp_nav_menu() which broke some theme designs.

Also includes these related changes:

• Export oldContainer and newContainer among the customize-preview-menu-refreshed event params for themes to be able to more easily re-initialize the DOM elements.
• Improve performance for partial refresh by only sending settings related to the menu being previewed.
• Fix previewing of menu assigned to Custom Menu by exporting a menu term_id as opposed to an object, as the former is more stable for comparing in in args hashes.
• Do full refresh of preview when nav menu unassigned so that the layout can be updated.
• Harden conditions for when partial refresh is eligible for a wp_nav_menu() instance.

Fixes #32841.