WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 4 weeks ago

#51592 closed defect (bug) (fixed)

Customizer: Create New Menu is Empty

Reported by: boldgrid Owned by: SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: blocker Version: 5.6
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

When attempting to create new menus in the WordPress Customizer, the Create New Menu panel is empty. I am not getting any console errors or php errors.
Steps to Replicate:

  1. Install clean wordpress with latest stable version.
  2. Install beta tester plugin.
  3. Set to bleeding edge nightlies
  4. Dashboard > Updates > update core ( this installed build 5.6-beta1-49262 for me )
  5. Go to Appearence > Customize
  6. Go to Menus > Create New Menu

Attachments (2)

wordpress-beta-5.6.gif (1.5 MB) - added by boldgrid 5 weeks ago.
Gif screencapture demonstration issue.
51592.2.patch (659 bytes) - added by david.binda 4 weeks ago.

Download all attachments as: .zip

Change History (22)

@boldgrid
5 weeks ago

Gif screencapture demonstration issue.

#1 @johnbillion
5 weeks ago

  • Milestone changed from Awaiting Review to 5.6

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


5 weeks ago

#3 @dlh
5 weeks ago

  • Keywords needs-patch added

Welcome to Trac, @boldgrid!

According to git bisect, the trouble here started with [49101].

#4 @azaozz
5 weeks ago

Yeah, can reproduce it too. It is caused by using jQuery 3.5.1 plus jQuery Migrate 3.3.1. I see that some of the HTML there is missing but can't see where that comes from. Will keep digging.

Last edited 5 weeks ago by azaozz (previous) (diff)

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


5 weeks ago

#6 @boldgrid
5 weeks ago

Making the following additions to wp-admin/js/customize-nav-menus.js resolved the issue for me.

L1361+: menuNameControl.renderContent();
L1375+: menuLocationsControl.renderContent();
L1388+: newMenuSubmitControl.renderContent();

#7 @Clorith
5 weeks ago

Spent some time digging, but couldn't find the root cause either yet, but some more details that have been found so far:

The problem appears to be that the wp.template function never builds the menu selection markup, which is needed to make the markup which the jQuery selector later depends on when clicking to change ot the create menu screen.

r41768 introduced a new menu creation flow, reverting back to just before this point, and feeding in jQuery 3.x and things still work as expected, so I wonder if it's something relating to this flow-change which isn't playing nicely with other related JS changes.

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


5 weeks ago

#9 @david.binda
5 weeks ago

I have looked into this a bit, and it looks like the rendering of the missing nav menu controls is prevented by the conditional in https://build.trac.wordpress.org/browser/trunk/wp-admin/js/customize-controls.js?rev=48413#L3701

Testing the beta with jQuery 3.5.1, the parentContainer and control.container.parent() are the same objects, however with jQuery 1.12.4 the control.container.parent() is empty.

Could this be related to the changes mentioned in https://jquery.com/upgrade-guide/1.9/#ordering-of-disconnected-nodes-within-a-jquery-set ?

However, I'm not sure if that's a root cause or just a symptom.

#10 @Clorith
5 weeks ago

That shouldn't be caused by the 1.9 changes (since it works in 1.12.4-wp).

But the section you found does bubble upwards to a Deferred element, which did have some changes in jQuery 3.0, the code uses .done() though, which (if I read this right) should retain backwards compatibility and not have changed behaviors.

#11 follow-up: @david.binda
5 weeks ago

That shouldn't be caused by the 1.9 changes (since it works in 1.12.4-wp).

Right. I was too fast with my judgement there while looking for possible cause upstream. However, if my testing is correct, the code's behaviour changes in between jQuery 2.2.4 and 3.0.0 (as in the .parent() is not returning the parent for control.container ).

The biggest part of the diff in between the 2.2.4 and 3.0.0 is the Deferred element you are mentioning.

However, even in 2.2.4, the parentNode seems to be set (just .parent() is not returning it).

I wonder, can't the control.renderContent(); call be moved outside of the conditional leaving only the .append inside?

In r41482 there seems to be a similar change, however a part of a bigger changeset, so I'm not really sure why it was needed.

#12 @david.binda
4 weeks ago

My current understanding is that the control rendering is really broken due to the changes in Deferred objects, as it has already been mentioned.

While we are using .done() in customizer code, we are also using $.where() method (see https://build.trac.wordpress.org/browser/trunk/wp-includes/js/customize-base.js?rev=48412#L512 ), which was also updated:

Breaking change and Feature: jQuery.when() arguments
jQuery.when now interprets any input argument with a then method as a Promise-compatible "thenable".

which, IMHO, with changes to .then(),

Another behavior change required for Promises/A+ compliance is that Deferred .then() callbacks are always called asynchronously. Previously, if a .then() callback was added to a Deferred that was already resolved or rejected, the callback would run immediately and synchronously.

(source: https://jquery.com/upgrade-guide/3.0/#breaking-change-and-feature-jquery-deferred-is-now-promises-a-compatible )

means that the timing of when our embed method is being called and when the element is appended to the DOM might be slightly different.

That's why I believe that the control.renderContent(); should be called outside the conditional (see the patch).

@david.binda
4 weeks ago

#13 in reply to: ↑ 11 ; follow-up: @azaozz
4 weeks ago

Replying to david.binda:
Ha, was just about to attach pretty much the same patch, but you beat me to it! :)

The biggest part of the diff in between the 2.2.4 and 3.0.0 is the Deferred element you are mentioning.

However, even in 2.2.4, the parentNode seems to be set (just .parent() is not returning it).

My "current guess" is that this is timing related. Looking at 5.5, when the deferred in inject runs, none of the control.container[0].parentNode exist yet, or are in a DocumentFragment (not in the DOM yet, so jQuery cannot find the parent). So control.container.parent().is( parentContainer ) is always false. Logs:

sectionId ===  nav_menu[5]
control.container[0].parentNode null
control.container.parent()[0] Object { length: 0, prevObject: {…}, context: undefined }
control.container.parent().is( parentContainer ) false
---------------------------------------- 
sectionId ===  add_menu
control.container[0].parentNode null
control.container.parent()[0] Object { length: 0, prevObject: {…}, context: undefined }
control.container.parent().is( parentContainer ) false
---------------------------------------- 
sectionId ===  add_menu
control.container[0].parentNode null
control.container.parent()[0] Object { length: 0, prevObject: {…}, context: undefined }
control.container.parent().is( parentContainer ) false
---------------------------------------- 
sectionId ===  menu_locations
control.container[0].parentNode DocumentFragment [ li#customize-control-nav_menu_locations-top.customize-control.customize-control-nav_menu_location ]
control.container.parent()[0] Object { length: 0, prevObject: {…}, context: undefined }
control.container.parent().is( parentContainer ) false
----------------------------------------

However in 5.6-beta the same debug logging outputs:

sectionId ===  menu_locations
control.container[0].parentNode DocumentFragment [ li#customize-control-nav_menu_locations-social.customize-control.customize-control-nav_menu_location ]
control.container.parent()[0] Object { length: 0, prevObject: {…} }
control.container.parent().is( parentContainer ) false
----------------------------------------
sectionId ===  nav_menu[10]
control.container[0].parentNode <ul id="sub-accordion-section-nav_menu[10]" class="customize-pane-child acc...">
control.container.parent()[0] Object { 0: ul#sub-accordion-section-nav_menu[10].customize-pane-child.acc..., length: 1, prevObject: {…} }
control.container.parent().is( parentContainer ) true
---------------------------------------- 
sectionId ===  add_menu
control.container[0].parentNode <ul id="sub-accordion-section-add_menu" class="customize-pane-child acc...">
control.container.parent()[0] Object { 0: ul#sub-accordion-section-add_menu.customize-pane-child.acc..., length: 1, prevObject: {…} }
control.container.parent().is( parentContainer ) true
---------------------------------------- 

The nav_menu and add_menu containers are already in the DOM, the other containers are not and control.container.parent().is( parentContainer ) is still false for them.

I wonder, can't the control.renderContent(); call be moved outside of the conditional leaving only the .append inside?

Yes, testing this here seems to work. Pinging @westonruter for a review.

#14 @hellofromTonya
4 weeks ago

  • Keywords has-patch dev-feedback added; needs-patch removed

#15 @azaozz
4 weeks ago

  • Keywords needs-testing added

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


4 weeks ago

#17 in reply to: ↑ 13 ; follow-up: @westonruter
4 weeks ago

Replying to azaozz:

The nav_menu and add_menu containers are already in the DOM, the other containers are not and control.container.parent().is( parentContainer ) is still false for them.

I wonder, can't the control.renderContent(); call be moved outside of the conditional leaving only the .append inside?

Yes, testing this here seems to work. Pinging @westonruter for a review.

This code introduced in r30102, with the work having been done in https://github.com/xwp/wordpress-develop/pull/29. The message for the Git commit was:

Allow controls and sections to be registered without parents

  • Add ready() abstract to Section/Panel
  • Remove needless callbacks

That being said, I don't recall why control.renderContent() was being made conditional upon parentContainer not being the control.container.parent(). Moving it outside of that conditional makes sense to me, and I'm not seeing any other regressions caused by doing so.

#18 @hellofromTonya
4 weeks ago

  • Keywords dev-feedback removed

Notes from scrub today:

Sergey:

There's a patch that I think makes sense to try in Beta 2

@helen

Yeah I definitely don’t want this to continue to be broken in beta
I don’t understand the fix but I will trust other people to tell me it’s fine if they don’t commit it first

#19 in reply to: ↑ 17 @SergeyBiryukov
4 weeks ago

  • Keywords commit added; needs-testing removed

Replying to westonruter:

That being said, I don't recall why control.renderContent() was being made conditional upon parentContainer not being the control.container.parent(). Moving it outside of that conditional makes sense to me, and I'm not seeing any other regressions caused by doing so.

It's worth noting that there is a similar fragment for panels earlier in the file, where panel.renderContent() is called outside of the conditionals:

if ( ! panel.headContainer.parent().is( parentContainer ) ) {
	parentContainer.append( panel.headContainer );
}
if ( ! panel.contentContainer.parent().is( panel.headContainer ) ) {
	container.append( panel.contentContainer );
}
panel.renderContent();

So the patch brings some consistency and works as expected in my testing. Marking for commit.

#20 @SergeyBiryukov
4 weeks ago

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

In 49323:

Customize: Ensure the New Menu section is properly rendered.

Follow-up to [30102], [49101].

Props david.binda, boldgrid, dlh, azaozz, Clorith, westonruter, hellofromTonya.
Fixes #51592.

Note: See TracTickets for help on using tickets.