Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#32672 closed defect (bug) (fixed)

Unable to assign Menu location when creating the first menu

Reported by: dinamiko's profile dinamiko Owned by: westonruter's profile westonruter
Milestone: 4.3 Priority: high
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description (last modified by stevenkword)

Replying to designsimply:

I also tested 32672.2.diff, but I spotted an error in the console and the first menu gets listed twice in the Menu Locations panel if you follow the testing steps exactly and click Save & Publish before going to Menu Locations (screenshot).

@gabrielperezs and I took a look at this during WCEU's contributor day. Both patches apply without issue, but introduce a new bug with the Menu Locations.

We were not able to reproduce the duplicate menus in the Locations Panel, but we are noticing that the newly created menus will not appear in the Locations Panel, without a Save & Publish action. Otherwise, the menu remains empty.

Steps to reproduce.

1.) Apply 32672.2.diff
2.) Add a new menu via the Customizer's Menu Panel.
3.) Select a menu location from the Menu Panel.
4.) Without saving, navigate to the Menu Locations Panel.

The newly created menu will not appear. This is presumably because the newly created menu is never saved.

Attachments (3)

32672.diff (2.7 KB) - added by markoheijnen 9 years ago.
32672.2.diff (3.3 KB) - added by valendesigns 9 years ago.
32672.3.diff (3.2 KB) - added by westonruter 9 years ago.
Additional change: https://github.com/xwp/wordpress-develop/commit/9a4fbe8570237ff9c05242d0876244026b7fc018 All patches: https://github.com/xwp/wordpress-develop/pull/97

Download all attachments as: .zip

Change History (21)

#1 @markoheijnen
9 years ago

  • Milestone changed from Awaiting Review to 4.3

customize-nav-menus.js?ver=4.3-alpha-32280-src:1869 Uncaught TypeError: Cannot read property 'get' of undefined

#2 @obenland
9 years ago

  • Owner set to westonruter
  • Status changed from new to assigned

#3 @designsimply
9 years ago

  • Keywords reporter-feedback added

I did spot the error markoheijnen mentioned the first time I tried testing this, but I wasn't recording.

TypeError: api.section(...) is undefined1 customize-nav-menus.js:1583:11

Went back to record, tried testing two more times, and couldn't reproduce:

  • 59s - using the menu location dropdown
  • 1m1s - using the Primary Menu checkbox

Seen at whatisinfinity.com/wp-admin/customize.php tested with WordPress 4.3-alpha-32819 using Firefox 38.0.5 on Mac OSX 10.10.3.

Aside: dinamiko, your screencasts are great! Thanks for including them.

@dinamiko and @markoheijnen, any chance you could update trunk and try it again?

#4 @markoheijnen
9 years ago

I was up-to-date but you need to delete all the menus first. Only then you will see the error.

@markoheijnen
9 years ago

#5 @markoheijnen
9 years ago

I deleted the if statement if menus are created so the locations do get added.

This is still a todo if a menu should be created or not. I could live either way. Since if the menu is empty, the customizer isn't saving it.

#6 @valendesigns
9 years ago

  • Keywords has-patch needs-testing added; reporter-feedback removed

@markoheijnen I'll test your patch later tonight. Thanks!

@valendesigns
9 years ago

#7 @valendesigns
9 years ago

Sorry for the delay! I've tested the patch and it does work, the only issue I found was that it allowed empty select options to be appended into the location dropdowns. I changed the JS a little to account for this in 32672.2.diff.

#8 follow-up: @designsimply
9 years ago

I was up-to-date but you need to delete all the menus first. Only then you will see the error.

Aha. Thanks!

I re-tested 32672.diff on WP 4.3-alpha-32975 and it worked for me: 42s screencast.

I also tested 32672.2.diff, but I spotted an error in the console and the first menu gets listed twice in the Menu Locations panel if you follow the testing steps exactly and click Save & Publish before going to Menu Locations (screenshot).

#9 in reply to: ↑ 8 @stevenkword
9 years ago

  • Description modified (diff)

Replying to designsimply:

I also tested 32672.2.diff, but I spotted an error in the console and the first menu gets listed twice in the Menu Locations panel if you follow the testing steps exactly and click Save & Publish before going to Menu Locations (screenshot).

@gabrielperezs and I took a look at this during WCEU's contributor day. Both patches apply without issue, but introduce a new bug with the Menu Locations.

We were not able to reproduce the duplicate menus in the Locations Panel, but we are noticing that the newly created menus will not appear in the Locations Panel, without a Save & Publish action. Otherwise, the menu remains empty.

Steps to reproduce.

1.) Apply 32672.2.diff
2.) Add a new menu via the Customizer's Menu Panel.
3.) Select a menu location from the Menu Panel.
4.) Without saving, navigate to the Menu Locations Panel.

The newly created menu will not appear, presumably because the newly created menu is never saved.

We are only seeing this behavior in twentyfifteen and twentyeleven.

Last edited 9 years ago by stevenkword (previous) (diff)

#10 follow-up: @westonruter
9 years ago

Just took a quick look at the patch. Why the change to check for the name property being undefined as opposed to the setting being false? Note that false means the setting is marked for deletion.

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


9 years ago

#12 @westonruter
9 years ago

  • Priority changed from normal to high

#13 in reply to: ↑ 10 @valendesigns
9 years ago

Replying to westonruter:

Just took a quick look at the patch. Why the change to check for the name property being undefined as opposed to the setting being false? Note that false means the setting is marked for deletion.

Because there were situations when setting().name was undefined and setting() was not returning false. So only checking for false didn't stop the console error messages.

#14 @valendesigns
9 years ago

#32871 was marked as a duplicate.

#15 @westonruter
9 years ago

@valendesigns: When I tried 32672.2.diff I didn't see newly-added menus appearing in the nav menu locations' dropdowns because the menu option would only get added in that api.bind( 'add' ) call, whereas the api.bind( 'change' ) call is then just updating the existing option. When a new nav menu is added, it first gets added with an empty object {} and then right after gets updated to the the full nav menu object that is to be created; this ensures it is marked dirty. So the add callback fires first to inject the new option with the placeholder ID, and then when the change callback fires it supplies the name to the option.

I've restored the original false check in 32672.3.diff and I've added a different safeguard for ensuring a string is passed as the name. Nevertheless, I was not able to reproduce the JS error even without making sure displayNavMenuName was passed a string. Perhaps the JS error was happening before displayNavMenuName was introduced in [33071].

If anyone can reproduce the JS error now with this latest path applied to trunk, please share the details.

#16 @westonruter
9 years ago

Note that most of the changes in the patch are whitespace, as an if statement is removed. Patch without whitespace changes: https://github.com/xwp/wordpress-develop/pull/97/files?w=1

#17 @valendesigns
9 years ago

  • Keywords commit added; needs-testing removed

@westonruter Your updated patch solves the issues and there are no more errors. Without the patch I could not see the menu locations control and I would get this error in the console:

TypeError: navMenuLocationSetting is undefined
	element.set( navMenuLocationSetting.get() === control.params.menu_id );

line 1862: wp-admin/js/customize-nav-menus.js

#18 @westonruter
9 years ago

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

Fix was committed in [33094]. I made a typo by putting the wrong ticket number (#32858).

Note: See TracTickets for help on using tickets.