WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 10 months ago

Last modified 8 months ago

#35206 closed enhancement (fixed)

Allow theme authors to control white space between menu items

Reported by: peterwilsoncc Owned by: peterwilsoncc
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-patch
Focuses: template Cc:

Description (last modified by 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.

Attachments (9)

35206.diff (9.1 KB) - added by peterwilsoncc 18 months ago.
35206.jpg (35.6 KB) - added by peterwilsoncc 18 months ago.
35206-unit-tests.diff (3.2 KB) - added by peterwilsoncc 18 months ago.
35206.2.diff (14.6 KB) - added by peterwilsoncc 18 months ago.
35206.3.diff (18.7 KB) - added by peterwilsoncc 18 months ago.
35206.4.diff (46.6 KB) - added by peterwilsoncc 13 months ago.
35206.5.diff (46.8 KB) - added by peterwilsoncc 12 months ago.
35206.6.diff (18.0 KB) - added by peterwilsoncc 10 months ago.
35206.7.diff (2.2 KB) - added by peterwilsoncc 10 months ago.

Download all attachments as: .zip

Change History (53)

#1 @peterwilsoncc
18 months ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed

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. :)

#2 @peterwilsoncc
18 months ago

  • Description modified (diff)
  • Keywords needs-patch added; has-patch removed

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

#3 follow-up: @peterwilsoncc
18 months ago

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? :)

#4 in reply to: ↑ 3 ; follow-up: @obenland
18 months ago

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
18 months ago

#5 in reply to: ↑ 4 @peterwilsoncc
18 months ago

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.

#6 @afercia
18 months ago

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.

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


18 months ago

#8 @peterwilsoncc
18 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.

#9 follow-up: @peterwilsoncc
18 months ago

  • Keywords has-patch added; needs-patch removed

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.

#10 in reply to: ↑ 9 ; follow-up: @afercia
18 months ago

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 ?

#11 in reply to: ↑ 10 @peterwilsoncc
18 months ago

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. :)

#12 @peterwilsoncc
18 months ago

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

#13 follow-up: @afercia
17 months ago

  • Milestone changed from Awaiting Review to 4.5

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?

#14 in reply to: ↑ 13 @obenland
17 months ago

Replying to afercia:

@obenland any thoughts?

My view hasn't changed from comment:4.

#15 follow-up: @afercia
17 months ago

@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().

#16 in reply to: ↑ 15 @obenland
17 months ago

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.

#17 @peterwilsoncc
17 months ago

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?

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


16 months ago

#19 follow-up: @Kelderic
16 months ago

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.

#20 in reply to: ↑ 19 @peterwilsoncc
16 months ago

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.

#21 @afercia
16 months ago

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 :)

#22 @afercia
16 months ago

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:

https://cldup.com/-WYejGCGmw.png

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


16 months ago

#24 @jorbin
16 months ago

  • Milestone changed from 4.5 to Future Release

#25 @peterwilsoncc
13 months ago

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.

#26 @peterwilsoncc
13 months ago

  • Keywords needs-refresh added

Needs refresh following [37640]

#27 @peterwilsoncc
12 months ago

  • Keywords needs-refresh removed

#28 @peterwilsoncc
10 months ago

  • Milestone changed from Future Release to 4.7

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.

#29 follow-up: @johnbillion
10 months ago

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.

#30 in reply to: ↑ 29 @peterwilsoncc
10 months ago

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

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.

#31 @peterwilsoncc
10 months ago

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

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.

#32 @johnbillion
10 months ago

  • Keywords needs-docs added; has-unit-tests has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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'.

#33 follow-up: @afercia
10 months ago

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

https://cldup.com/-Ae7D17WAM.png

#34 in reply to: ↑ 33 ; follow-up: @peterwilsoncc
10 months ago

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.

#35 in reply to: ↑ 34 @dgwyer
10 months ago

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.

Last edited 10 months ago by dgwyer (previous) (diff)

#36 @johnbillion
10 months ago

  • Keywords needs-patch added

#37 @johnbillion
10 months ago

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

#38 follow-up: @peterwilsoncc
10 months ago

  • Keywords has-patch added; needs-patch removed

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.

#39 @peterwilsoncc
10 months ago

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.

#40 @peterwilsoncc
10 months ago

  • Keywords needs-docs removed

#41 @peterwilsoncc
10 months ago

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

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.

#42 in reply to: ↑ 38 @dgwyer
10 months ago

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.

This ticket was mentioned in Slack in #themereview by jcastaneda. View the logs.


9 months ago

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


8 months ago

Note: See TracTickets for help on using tickets.