WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#38952 closed defect (bug) (fixed)

Customize Menus: screen options are broken

Reported by: celloexpressions Owned by: westonruter
Milestone: 4.7 Priority: normal
Severity: major Version: 4.7
Component: Customize Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

The screen options for menu item properties in the customizer are intended to make menus less overwhelming. By default, all of the optional properties are hidden/unchecked.

In trunk, no screen options seem to apply. All fields are always shown and changing the screen options checkboxes doesn't toggle their visibility. The selected options do seem to be saving from the checkboxes (via Ajax), so the issue is on the display side. Confirmed that this was introduced in 4.7.

Milestoning for 4.7 due to the severity, but still needs a patch.

Attachments (3)

38952.0.diff (3.2 KB) - added by westonruter 8 months ago.
38952.1.diff (4.6 KB) - added by westonruter 8 months ago.
https://github.com/xwp/wordpress-develop/pull/207
38952.2.diff (4.7 KB) - added by westonruter 8 months ago.

Download all attachments as: .zip

Change History (12)

@westonruter
8 months ago

#1 @westonruter
8 months ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Owner set to westonruter
  • Status changed from new to accepted

The issue stems from the restructuring of the DOM for panels/sections in #34391. Now that the nav_menus panel is no longer logically the parent DOM element to the nav_menu sections, we now have to toggle the active classes on each section container instead.

#2 @westonruter
8 months ago

38952.1.diff improves the organization of the logic for managing and listening for changes to the toggled state for the columns, and it limits the toggling of the class names on the new contentContainer rather than being also toggled on the headContainer as well.

#3 @celloexpressions
8 months ago

  • Keywords commit added; needs-testing removed

38952.1.diff has more churn than I'd like to see during RC, but works. This isn't very efficient, binding an event for each menu to the toggles and duplicating the classes on each section, but this is the only way to keep it contained within menus rather than introducing higher-level class changes for this in the customizer. There were no visual performance issues (and none would be expected in most cases) when testing on a site with 12 menus (and the associated 12 menu sections).

I can confirm that the patch works and looks ready to commit.

#4 @celloexpressions
8 months ago

One other note: at the inline comment Show/hide/save screen options (columns). From common.js., this refers to the code that's removed in 38952.1.diff, which was originally pasted directly from common.js. So the From common.js part of the comment should also be removed on commit.

@westonruter
8 months ago

#5 @westonruter
8 months ago

  • Keywords dev-feedback added

38952.2.diff updates the jsdoc comment on MenusPanel#ready.

In regards to efficiency of adding event listeners to all the active field toggles: there are only 5 toggles. Adding 5 event listeners for each nav menu is going to add an insignificant amount of event listeners overall, even with 10 separate menus there would only be 50 event listeners. I would be worried if we were getting into 500 or 5000 event listeners. Eventually it would be better to have Value states for the checkboxes with a single event handler added from the panel, with the event listeners on Value added in the sections. But that was too much for RC I think.

+1 for commit. Needs dev-reviewed.

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


8 months ago

#7 @ocean90
8 months ago

  • Keywords dev-reviewed added; dev-feedback removed

38952.2.diff looks good and fixes the reported issue.

#8 @westonruter
8 months ago

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

In 39378:

Customize: Fix regression in ability to hide fields for advanced menu properties in nav menu item controls.

Props westonruter, celloexpressions.
See #34391.
Fixes #38952.

#9 @westonruter
8 months ago

In 39379:

Customize: Fix regression in ability to hide fields for advanced menu properties in nav menu item controls.

Props westonruter, celloexpressions.
See #34391.
Fixes #38952 for 4.7 branch.

Note: See TracTickets for help on using tickets.