Allow theme authors to control white space between menu items

[peterwilsoncc]

Sometimes menu items are best floated, sometimes they're best as inline-blocks. Whitespace between the lis can have an affect on layout for the later.

Let's allow theme authors to control the white space between menu items:

• remove \n, \r, \t string replacement in wp_page_menu since [9246]
• allow theme authors to choose between the markup detailed in comment:2

Related #27762 and #35107.

---

original description

Sometimes menu items are best floated, sometimes they're best as inline-blocks. Whitespace between the lis can have an affect on layout for the later.

Let's allow theme authors to control the white space between menu items:

• remove \n, \r, \t string replacement in wp_page_menu since [9246]
• Introduce item_before and item_after arguments to
  • wp_nav_menu
  • wp_list_pages
  • wp_page_menu
• item_before defaults to an empty string
• for the first two functions item_after defaults to a line-break to match the current behaviour
• for wp_page_menu, item_after defaults to an empty string to match the current behaviour

Related #27762 and #35107.

Patch will need to follow the resolution of #35107.

[peterwilsoncc]

In 35206.diff​:

• as described above
• tab indents removed in front of <li>s because it messes up the white-space if the theme author wishes to have none.

Default markup of a menu, wp_nav_menu is:

<ul><li><a /></li>
<li><a />
	<ul class="sub-menu"><li><a /></li>
<li><a /></li>
</ul>
</li>
</ul>

If the theme author specifies no white space between items, it becomes

<ul><li><a /></li><li><a />
	<ul class="sub-menu"><li><a /></li><li><a /></li>
</ul></li>
</ul>

wp_page_menu defaults to the white space free version.

This will need unit tests. I will need help and advice writing them. :)

[peterwilsoncc]

The more I think about the initial approach, the less wise I think it is.

The desired outcome is allowing theme authors to specify a white-spaced and no-white-space version of wp_nav_menu and wp_page_menu. The output code (again using self closing a tags to save space) becomes:

white space

<ul>
	<li><a /></li>
	<li><a />
		<ul class="sub-menu">
			<li><a /></li>
			<li><a /></li>
		</ul>
	</li>
</ul>

no white space

<ul><li><a /></li><li><a /><ul class="sub-menu"><li><a /></li><li><a /></li></ul></li></ul>

item_before, item_after don't really work as they leave open the possibility of malformed markup. Possible options:

• whitespaced: true/false
• indented: true/false

[peterwilsoncc]

Tests required:

• wp_nav_menu without options (white space not suppressed), menu defined
• wp_nav_menu with white space suppression, menu defined
• wp_nav_menu without options (white space not suppressed), no menu defined, falls back to wp_page_menu
• wp_nav_menu with white space suppression, menu defined, no menu defined, falls back to wp_page_menu
• wp_page_menu without options (white space suppressed)
• wp_page_menu without white space suppressed

Would it be best to search for the white space \t or \n or full markup? @obenland do you have an opinion following #27762? :)

[obenland]

Replying to peterwilsoncc:

@obenland do you have an opinion following #27762? :)

I do, not sure if you like it though. I don't think it is necessary to go as far as offering one/two more arguments to remove whitespace from menu items, it feels over-engineered. Theme authors have a few ways to deal with it, floating, setting font-size to 0 and resetting it, or using a custom nav menu walker that overwrites the end_el() method.

It would have been nice to just be able to remove the new line, but I don't feel like it's severe enough of a problem to add more arguments.

[peterwilsoncc]

Replying to obenland:

I do, not sure if you like it though.

You're good, I'm happy with pushback as it makes for a better WordPress :).

This is what I'm aiming to do:

• have wp_nav_menu remain whitespace consistent when it falls back to the page listing (they currently differ)
• have the menus and page listings consistent
• allow right aligned menus to be space free while displaying in menu order. float:right removes the space but reverses the items. See 2014 menu in 35206.jpg​ with the space highlighted.
• allow for center aligned menus without spaces between.
• maintain backward compatibility.

It's possible with filters (one for line breaks and one for indents) but messy as filters would need to be added and removed when menus fall back to page listings.

As a themer, the first item is the biggest hurdle. The same function can output different whitespace.

[afercia]

Replying to peterwilsoncc:

• have wp_nav_menu remain whitespace consistent when it falls back to the page listing (they currently differ)

Yep, this was one of the concerns that #27762 tried to address. Totally agree these two functions should have a consistent white space handling :) For example, the CSS hack mentioned in #35107 to have justified menu items won't work when the menu fallbacks to wp_page_menu().

About the second issue, yes ​there are a number of ways to fight white space using CSS only, including using Flexbox, but they all have drawbacks. It's almost 2016 and still the only reliable way to do this is controlling the markup output, waiting for ​white-space-collapsing in CSS4.

Software should make things easy and solve this kind of issues for you, so I'd agree with @peterwilsoncc about introducing a simple way (maybe an argument?) to remove white space.

[peterwilsoncc]

35206-unit-tests.diff​ is a first attempt at unit tests described in comment:3.

It uses whitespace: true/false as a single new option for the time being.

[peterwilsoncc]

In 35206.2.diff​:

• consistent whitespace for wp_nav_menu falling back to wp_page_menu
• new boolean option for both functions whitespace
• unit tests
• bit of a tidy up of the whitespace

I'm not entirely happy with whitespace as the option name for suppressing/including it. Would love suggestions.

[afercia]

Replying to peterwilsoncc:

I'm not entirely happy with whitespace as the option name for suppressing/including it. Would love suggestions.

Hm maybe "item_spacing" ​preserve - discard ?

[peterwilsoncc]

Suggestions I've sort from devs elsewhere:

• html_linebreak
• menu_whitespace

Replying to afercia:

Hm maybe "item_spacing" preserve - discard ?

That's nice too, I'll wait a few days before refreshing to allow any other suggestions to come in while we have a few extra eyes around here. :)

[peterwilsoncc]

In 35206.3.diff​, refreshed to use the argument item_spacing : preserve/discard.

[afercia]

Haven't looked in depth but I'd like to propose this for 4.5 consideration. It would be an improvement to a long standing CSS/layout issue and would give developers a clean way to solve it. @obenland any thoughts?

[obenland]

Replying to afercia:

@obenland any thoughts?

My view hasn't changed from comment:4.

[afercia]

@obenland I see your point but I think the current situation is not ideal. Worth reminding [36082] reverted [34321] to support what I'd consider a CSS hack (justified menu items) so now we're back to the original situation where Walker_Nav_Menu outputs a white space while there's no white space in the fallback wp_page_menu().
This can potentially break layouts, including the mentioned "justified menu items" hack, when falling back to wp_page_menu().

[obenland]

Replying to afercia:

[…] we're back to the original situation where Walker_Nav_Menu outputs a white space while there's no white space in the fallback wp_page_menu(). This can potentially break layouts, including the mentioned "justified menu items" hack, when falling back to wp_page_menu().

I'm really not too concerned about this. It has been that way ever since nav menus were introduced and themes make up for it with different styles for these scenarios or by removing the fallback.

[peterwilsoncc]

Disabling the fallback/the CSS above are hacky workarounds an issue with core, rahter than solutions.

Calling a function with default arguments should return consistent markup. I remember that being commented upon in #core when [34321] was committed.

Of the proposed CSS workarounds, float dictates design and font-size prevents the use of particular CSS units and - at best - lacks consistency cross browser.

If adding an argument to the function is not an option, how can we solve this problem?

[Kelderic]

Why do we need to preserve white space at all? Why not just change the behavior to compress white space. Then inline-block has no oddities in behavior, and we don't have to add a new option.

[peterwilsoncc]

Replying to Kelderic:

Why do we need to preserve white space at all? Why not just change the behavior to compress white space. Then inline-block has no oddities in behavior, and we don't have to add a new option.

This was how the inconsistent markup was solved in #27762 but it broke some themes so was reverted in #35107. Thus the hunt for a solution that maintains backward compatibility, be in an argument, theme-support, or some other suggestion.

[afercia]

Just wanted to add that the previous change was reverted just to support a single, specific, rarely used, CSS technique for "justified menu items" which is basically a CSS hack, you can see it here:
https://core.trac.wordpress.org/ticket/35107#comment:2
it is a hack because it has to "fake" a second line of text in order to make justified alignment work. Worth considering we'll all soon ​use Flexbox for this.

By the way, the main issue is still the original one: WordPress uses two functions for the same purpose and these two functions output inconsistent HTML. Even if it's a small detail, I think we should find a solution unless we intentionally want to make developers life harder :)

[afercia]

For completeness, the justified menu items CSS trick can't work when a menu fallbacks to wp_page_menu(). A quick demo using Twenty Thirteen:

[peterwilsoncc]

Refreshed in 35206.4.diff​ to pass coding standards and removed a couple of bugs.

Still quite eager to resolve this one way or another, be it arguments, filters or theme support.

[peterwilsoncc]

Needs refresh following [37640]

[peterwilsoncc]

I'd like to have one last go at this over the next couple of weeks.

In 35206.6.diff​, I've made the following changes:

• touches much fewer lines, existing tabs can remain imperfect
• no red lines in tests
• after seeking some advice, I applied the logic once and used variables.

I'm happy enough to own this, so putting this on the milestone to encourage discussion.

[johnbillion]

I've not checked, but #24587 might affect this. Need to confirm whether $args is an array or an object in every situation where it's used.

I approve of the item_spacing logic introduced in 35206.6.diff​.

[peterwilsoncc]

Replying to johnbillion:

I've not checked, but #24587 might affect this. Need to confirm whether $args is an array or an object in every situation where it's used.

I've looked over #24587, within each walker $args is consistent but it differs between the two. I've added some tests to ensure I don't (un)break this.

[peterwilsoncc]

In 38523:

Menus: Add white space option to wp_nav_menu() and wp_list_pages().

Adds an item_spacing option to the arguments array for the functions wp_nav_menu(), wp_list_pages(), and wp_page_menu(). item_spacing is a boolean accepting either preserve or discard.

Previously, certain CSS choices could result in a site's layout changing if wp_nav_menu() fell back to the default wp_list_pages() due to differences in the whitespace within the HTML. The new argument ensures a function outputs consistant HTML while maintaining backward compatibility.

Fixes #35206.

[johnbillion]

wp_nav_menu(), wp_list_pages(), and wp_page_menu() need a @since doc for the new item_spacing argument in the $args array. Also I think the argument description can be improved. How about:

Whether to preserve whitespace within the menu's HTML. Accepts 'preserve' or 'discard'. Default 'preserve'.

[afercia]

Getting a notice, reproduced on two different installs:
Notice: Undefined property: stdClass::$item_spacing in /srv/www/wordpress-develop/src/wp-includes/class-walker-nav-menu.php on line 240

[peterwilsoncc]

Replying to afercia:

Getting a notice, reproduced on two different installs:
Notice: Undefined property: stdClass::$item_spacing in /srv/www/wordpress-develop/src/wp-includes/class-walker-nav-menu.php on line 240

Thanks @afercia, I'll take a look over the next few hours. The easy fix is to add Walker_Nav_Menu_Edit::end_el() but that's not really the solution given others may extend the base class in a similar fashion for a custom admin.

[dgwyer]

I'm seeing exactly the same notice as afercia.

Also, are there some redundant variable declarations in 35206.6.diff?

e.g. In src/wp-includes/class-walker-nav-menu.php on lines 241, 244 the variable declaration for $t doesn't seem to be used anywhere inside the function end_el.

Maybe I'm missing something but I thought I'd mention it.

[johnbillion]

In 38559:

Menus: Correct the docblocks for Walker_Nav_Menu, wp_nav_menu(), and walk_nav_menu_tree().

This corrects the parameter type for the $args and $item parameters passed throughout these functions, class methods, and hooks.

See #24587
See #35206
See #37770

[peterwilsoncc]

In 35206.7.diff​, there's a fix to avoid having notices when Walker_Nav_Menu is extended but methods are called with different arguments.

@dgwyer you're correct, I decided to keep the logic and assignments identical throughout. The intent is to lower the risk of failure if a the class is extended and logic is copy-pasted.

[peterwilsoncc]

In 38574:

Menus: Improve documentation of new $item_spacing argument.

Adds @since tags for and improves description of the new $item_spacing argument added to wp_nav_menu(), wp_list_pages(), and wp_page_menu() in [38523].

Props johnbillion for copy.
See #35206.

[peterwilsoncc]

In 38575:

Menus: Fix notices thrown by classes extending Walker_Nav_Menu.

Methods in Walker_Nav_Menu can be called with multiple, incompatible $args objects so an insset() check is required to avoid throwing notices.

Introduced in [38523].

Fixes #35206.

[dgwyer]

I understand. Thanks for clarifying.

Replying to peterwilsoncc:

In 35206.7.diff​, there's a fix to avoid having notices when Walker_Nav_Menu is extended but methods are called with different arguments.

@dgwyer you're correct, I decided to keep the logic and assignments identical throughout. The intent is to lower the risk of failure if a the class is extended and logic is copy-pasted.