Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#32841 closed defect (bug) (fixed)

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

Reported by: greenshady Owned by: westonruter
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: javascript Cc:

Description

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

Attachments (7)

menu-customizer-saga.png (298.3 KB) - added by greenshady 7 years ago.
32841.diff (3.1 KB) - added by westonruter 7 years ago.
https://github.com/xwp/wordpress-develop/pull/96
32841.2.diff (5.0 KB) - added by westonruter 7 years ago.
Additional changes: https://github.com/xwp/wordpress-develop/compare/a07767d...dd73dcf
32841.3.diff (5.1 KB) - added by westonruter 7 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/c94de06d352f7a103e7f9af7918bc9c4df64a9bc
32841.4.diff (5.1 KB) - added by westonruter 7 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/c75bdcfb6b805ec520fcd254a10f3c980b9017ea
32841.4.with-32894.diff (7.1 KB) - added by westonruter 7 years ago.
Combining latest patch with patch for #32894.
32841.5.diff (6.6 KB) - added by westonruter 7 years ago.

Download all attachments as: .zip

Change History (28)

#1 @westonruter
7 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Owner set to westonruter
  • Status changed from new to accepted

#2 @westonruter
7 years ago

  • Focuses javascript added
  • Keywords has-patch needs-testing added

@greenshady: Please try 32841.diff:

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

#3 @westonruter
7 years ago

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.

#4 follow-up: @greenshady
7 years ago

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.

#5 in reply to: ↑ 4 @westonruter
7 years ago

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.

#6 @westonruter
7 years ago

  • Keywords reporter-feedback added; needs-testing removed

#7 follow-up: @greenshady
7 years ago

  • Keywords reporter-feedback removed

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.

#8 in reply to: ↑ 7 @westonruter
7 years ago

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

    wp.customize.menusPreview = ( function( $, api ) { 
    228228                        previousContainer.replaceWith( container );
    229229                        eventParam = {
    230230                                instanceNumber: instanceNumber,
    231                                 wpNavArgs: wpNavArgs
     231                                wpNavArgs: wpNavArgs,
     232                                oldContainer: previousContainer,
     233                                newContainer: container
    232234                        };
    233235                        container.removeClass( 'customize-partial-refreshing' );
    234236                        $( document ).trigger( 'customize-preview-menu-refreshed', [ eventParam ] );

#9 follow-up: @greenshady
7 years ago

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

#10 in reply to: ↑ 9 ; follow-up: @westonruter
7 years ago

  • Keywords needs-testing added

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.

#11 @greenshady
7 years ago

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

Last edited 7 years ago by greenshady (previous) (diff)

#12 @westonruter
7 years ago

  • Keywords needs-testing removed

Thanks!

@ocean90 could you do a code review for me?

#13 in reply to: ↑ 10 ; follow-up: @tywayne
7 years ago

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

#14 in reply to: ↑ 13 ; follow-up: @westonruter
7 years ago

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.

#15 in reply to: ↑ 14 @tywayne
7 years ago

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.

Last edited 7 years ago by tywayne (previous) (diff)

@westonruter
7 years ago

Combining latest patch with patch for #32894.

#16 @westonruter
7 years ago

  • Keywords needs-testing added

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.

#17 @tywayne
7 years ago

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

#18 @westonruter
7 years ago

  • Keywords commit added; needs-testing removed

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

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


7 years ago

@westonruter
7 years ago

#21 @westonruter
7 years ago

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

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.

Note: See TracTickets for help on using tickets.