WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#34111 closed defect (bug) (fixed)

Menu CSS Classes are saved as comma separated inside the Customiser

Reported by: paulwilde Owned by: westonruter
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit fixed-major
Focuses: javascript Cc:
PR Number:

Description

If you have a menu item with more than 1 CSS Class, it converts the spaces into a comma separated list.

If you alter the CSS classes, or introduce a new one it then merges all of the classes into a single string therefore breaking the output (and thus layout). You have to manually change all the commas into spaces if you want to update a menu item.

Unfortunately, I've had to steer clients away from the Customiser because of this bug.

Screenshots attached.

Attachments (3)

menu-css-classes.png (279.8 KB) - added by paulwilde 4 years ago.
menu-css-classes-output.png (165.8 KB) - added by paulwilde 4 years ago.
34111.patch (527 bytes) - added by tyxla 4 years ago.
Nav menu customizer: on menu control rendering, manually concatenate the classes element to avoid the automatic toString() concatenation with a comma glue.

Download all attachments as: .zip

Change History (13)

#1 @westonruter
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.3.2
  • Owner set to westonruter
  • Status changed from new to accepted

@tyxla
4 years ago

Nav menu customizer: on menu control rendering, manually concatenate the classes element to avoid the automatic toString() concatenation with a comma glue.

#2 @tyxla
4 years ago

  • Keywords has-patch added; needs-patch removed

Since the classes property is an array, it is automatically toString()-ed before being inserted as a value of the "CSS Classes" input field.

The above patch handles this by manually turning the array into string, concatenating the classes by a space " ". This will avoid the automatic toString() which turns the spaces into commas.

Side note: We can implement this for any array properties, but I consider that we should do this only for classes property. Reason: we should respect potential menu item element properties for other types of fields that would use arrays (for example sets of checkboxes or <select multiple>). Such fields can currently be added by overriding the default admin menu walker.

#3 @westonruter
4 years ago

@tyxla Patch looks good, thanks. I would make one small change, however, and that is to not override the settingValue object value, because since it is returned by reference from setting() it will have a side-effect of mutating the underlying setting's value. So I think this would be better:

  • src/wp-admin/js/customize-nav-menus.js

     
    11321132                                        }
    11331133                                });
    11341134                                if ( settingValue ) {
    1135                                         element.set( settingValue[ property ] );
     1135                                        if ( 'classes' === property && _.isArray( settingValue[ property ] ) ) {
     1136                                                element.set( settingValue[ property ].join( " " ) );
     1137                                        } else {
     1138                                                element.set( settingValue[ property ] );
     1139                                        }
    11361140                                }
    11371141                        });

@paulwilde Can you confirm this fixes the issue you reported?

#4 @paulwilde
4 years ago

I can confirm that the patch fixes the issue.

Edit: The snippet above also works.

Last edited 4 years ago by paulwilde (previous) (diff)

#5 @westonruter
4 years ago

  • Keywords commit added

Aside, since xfn and classes are sanitized in the same way, I was curious why the fix wasn't also needed for xfn. Turns out that wp_setup_nav_menu_item() ensures the former is an array but the latter is not:

$menu_item->classes = ! isset( $menu_item->classes ) ? (array) get_post_meta( $menu_item->ID, '_menu_item_classes', true ) : $menu_item->classes;
$menu_item->xfn = ! isset( $menu_item->xfn ) ? get_post_meta( $menu_item->ID, '_menu_item_xfn', true ) : $menu_item->xfn;

And also in wp_update_nav_menu_item(), it stores classes in postmeta as an array, but xfn is stored as a string:

$args['menu-item-classes'] = array_map( 'sanitize_html_class', explode( ' ', $args['menu-item-classes'] ) );
$args['menu-item-xfn'] = implode( ' ', array_map( 'sanitize_html_class', explode( ' ', $args['menu-item-xfn'] ) ) );

Alas.

#6 @westonruter
4 years ago

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

In 34788:

Customize: Fix nav_menu_item CSS classes array being incorrectly presented in input field as comma-delimited list.

Instead of using Array.toString() to serialize an array with comma delimiters, explicitly join the array using spaces instead. Also ensure that xfn is handled properly if it ever gets stored as an array.

Props tyxla, westonruter.
Fixes #34111.

#7 @westonruter
4 years ago

In 34789:

Customize: Fix nav_menu_item CSS classes array being incorrectly presented in input field as comma-delimited list.

Instead of using Array.toString() to serialize an array with comma delimiters, explicitly join the array using spaces instead. Also ensure that xfn is handled properly if it ever gets stored as an array.

Cherry-picks [34788].

Props tyxla, westonruter.
Fixes #34111 for 4.3.

#8 @westonruter
4 years ago

  • Focuses javascript added
  • Keywords fixed-major added
  • Version changed from 4.3.1 to 4.3

#9 @westonruter
4 years ago

In 35308:

Customizer: Prevent nav_menu_item settings from becoming dirty when their controls are set up.

Since wp_setup_nav_menu_item() returns the classes property as an array but the Customizer manages the value as a string, the setting needs to initially export the value as a string. This prevents the classes property type change from causing the setting to get marked as dirty even though nothing changed. This is a regression from [34788].

See #34111.

#10 @johnbillion
4 years ago

  • Milestone changed from 4.3.2 to 4.4
Note: See TracTickets for help on using tickets.