Opened 6 months ago
Closed 6 months ago
#51592 closed defect (bug) (fixed)
Customizer: Create New Menu is Empty
Reported by: |
|
Owned by: |
|
---|---|---|---|
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:
- Install clean wordpress with latest stable version.
- Install beta tester plugin.
- Set to bleeding edge nightlies
- Dashboard > Updates > update core ( this installed build 5.6-beta1-49262 for me )
- Go to Appearence > Customize
- Go to Menus > Create New Menu
Attachments (2)
Change History (22)
This ticket was mentioned in Slack in #core by jamesros161. View the logs.
6 months ago
#3
@
6 months ago
- Keywords needs-patch added
Welcome to Trac, @boldgrid!
According to git bisect
, the trouble here started with [49101].
#4
@
6 months 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.
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
6 months ago
#6
@
6 months 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
@
6 months 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.
6 months ago
#9
@
6 months 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
@
6 months 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:
↓ 13
@
6 months 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
@
6 months 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.
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).
#13
in reply to:
↑ 11
;
follow-up:
↓ 17
@
6 months 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.
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
6 months ago
#17
in reply to:
↑ 13
;
follow-up:
↓ 19
@
6 months ago
Replying to azaozz:
The
nav_menu
andadd_menu
containers are already in the DOM, the other containers are not andcontrol.container.parent().is( parentContainer )
is stillfalse
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
@
6 months 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
@
6 months ago
- Keywords commit added; needs-testing removed
Replying to westonruter:
That being said, I don't recall why
control.renderContent()
was being made conditional uponparentContainer
not being thecontrol.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.
Gif screencapture demonstration issue.