Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#35107 closed defect (bug) (fixed)

wp_nav_menu outputs tags without line breaks in 4.4, causes strange bug with justified text

Reported by: wp-architect's profile wp-architect Owned by: obenland's profile obenland
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Menus Keywords: good-first-bug 2nd-opinion has-patch
Focuses: template Cc:

Description

After updating from 4.3.1 to 4.4, justified alignment being applied via CSS to my main navigation no longer worked.

After inspecting the problem, It turns out 4.4 was displaying HTML tags on the same line which causes a strange bug in Google Chrome on latest OSX (untested in other browsers). See stackoverflow: https://stackoverflow.com/questions/24781842/justified-horizontal-list-with-minified-html

Reapplying line breaks to the menu output solved this issue.

Proposed fix - NOTE: I am a first time contributor and would like the opportunity to submit a pull request for this small fix, if it is legitimate.

Restore the following Walker_Nav_Menu function in wp-includes/nav-menu-template.php from 4.3 -

<?php
/**
         * Ends the element output, if needed.
         *
         * @see Walker::end_el()
         *
         * @since 3.0.0
         *
         * @param string $output Passed by reference. Used to append additional content.
         * @param object $item   Page data object. Not used.
         * @param int    $depth  Depth of page. Not Used.
         * @param array  $args   An array of arguments. @see wp_nav_menu()
         */
        function end_el( &$output, $item, $depth = 0, $args = array() ) {
                $output .= "</li>\n"; // added \n to appear on new lines
        }

Are you using either the latest version of WordPress, or the latest development version? If not, please update first.

Yes, this occurs on 4.4

What steps should be taken to consistently reproduce the problem?

Activate any theme that uses the wp_nav_menu. View site and see source code for navigation output.

Does the problem occur even when you deactivate all plugins and use the default theme?

Yes

In case it's relevant to the ticket, what is the expected output or result? What did you see instead?

4.3

<div class="menu">
<ul>
<li><a href="#">Menu Item</a></li>
<li><a href="#">Menu Item</a></li>
<li><a href="#">Menu Item</a></li>
<li><a href="#">Menu Item</a></li>
</ul>
</div>

4.4

<div class="menu"><ul><li><a href="#">Menu Item</a></li><li><a href="#">Menu Item</a></li><li><a href="#">Menu Item</a></li><li><a href="#">Menu Item</a></li></ul></div>

Attachments (2)

patch-35107.php (25.6 KB) - added by wp-architect 8 years ago.
35107.diff (7.1 KB) - added by peterwilsoncc 8 years ago.

Download all attachments as: .zip

Change History (18)

#1 @SergeyBiryukov
8 years ago

  • Component changed from General to Menus
  • Owner set to obenland
  • Status changed from new to assigned

Hi @wp-architect, welcome to Trac!

Thanks for the report. Introduced in [34321], see #27762.

#2 follow-up: @DoodleDogCody
8 years ago

Would like to add to this with code example of why this is a problem. On a number of mys sites I do the following

.nav-primary ul.menu-primary {
    text-align: justify;
}
.nav-primary ul.menu-primary::after {
    content: " ";
    display: inline-block;
    width: 100%;
}

This has the effect of evenly spacing the nav menu items within their container. This only works when there is space between the <li> tags because justify only works if there are multiple words or in this case elements for it to justify.

#3 @SergeyBiryukov
8 years ago

Could we perhaps add a space instead of bringing back \n?

#4 follow-up: @DoodleDogCody
8 years ago

@SergeyBiryukov Yes I just ran a quick test in the inspector and a space works as well. I assume this is the same kind of thing that @wp-architect is having problems with and a space should fix his problems as well.

I assume the change was simply to help lower page size a bit by having elements on the same line so adding a space should accomplish that goal without hurting functionality.

#5 @SergeyBiryukov
8 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 4.4.1

#6 @ocean90
8 years ago

  • Keywords good-first-bug removed

A space would produce the same issues as a newline character, see https://css-tricks.com/fighting-the-space-between-inline-block-elements/ for background info.

#7 in reply to: ↑ 4 @SergeyBiryukov
8 years ago

Replying to DoodleDogCody:

I assume the change was simply to help lower page size a bit by having elements on the same line so adding a space should accomplish that goal without hurting functionality.

Per #27762, it was to avoid spacing between menu items when displaying them horizontally.

#8 follow-up: @DoodleDogCody
8 years ago

  • Keywords good-first-bug added

After reading #27762 I see this was done for a specific reason of not relying on Floats for horizontal nav menus.

That ticket mentions edge cases for display inline-block. I assume there are other ways to accomplishing the goal of justifying nav menus using things like flexbox but I feel this change negatively effects sites that have relied on such css like listed above in my first comment.

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

Replying to DoodleDogCody:

That ticket mentions edge cases for display inline-block.

Using inline-block is far from being an edge case.

In all my time dealing with themes, reviewing themes, and making them, I have not once seen a justified menu. Even visiting "regular" websites, I don't think I ever noticed one. This just to explain why that was not caught by me in the initial work on the ticket—it just did not think of displaying it that way.

I guess we'll have to deal with floats a while longer.

#10 in reply to: ↑ 2 @wp-architect
8 years ago

Replying to DoodleDogCody:

Would like to add to this with code example of why this is a problem. On a number of mys sites I do the following

.nav-primary ul.menu-primary {
    text-align: justify;
}
.nav-primary ul.menu-primary::after {
    content: " ";
    display: inline-block;
    width: 100%;
}

This has the effect of evenly spacing the nav menu items within their container. This only works when there is space between the <li> tags because justify only works if there are multiple words or in this case elements for it to justify.

Thanks for mentioning that @DoodleDogCody - this is actually the exact method I am using. It works fairly well as an approach to responsive layout since Flexbox is still not widely adopted by browsers. Switching over to floats in an option but requires careful calculations when it comes to padding/margins between elements whereas text-align:justify figures it out for you.

For some added context, here is an article which further explains this approach. http://www.barrelny.com/blog/text-align-justify-and-rwd/

@peterwilsoncc
8 years ago

#11 follow-up: @peterwilsoncc
8 years ago

  • Keywords 2nd-opinion has-patch added; needs-patch removed

35107.diff is an approach to allow both approaches:

  • remove \n, \r, \t string replacement in wp_page_menu since [9246]
  • Introduces item_before and item_after arguments to
    • wp_nav_menu
    • wp_list_pages
    • wp_page_menu
  • item_before defaults to an empty string
  • item_after defaults to a line-break to match the pre 4.4 behaviour

The first item has a reasonably heavy touch, possibly too much for a point release.

#12 in reply to: ↑ 11 @DoodleDogCody
8 years ago

Replying to peterwilsoncc:

35107.diff is an approach to allow both approaches:

  • remove \n, \r, \t string replacement in wp_page_menu since [9246]
  • Introduces item_before and item_after arguments to
    • wp_nav_menu
    • wp_list_pages
    • wp_page_menu
  • item_before defaults to an empty string
  • item_after defaults to a line-break to match the pre 4.4 behaviour

The first item has a reasonably heavy touch, possibly too much for a point release.

I would say +1 to this idea. I think it's the best way to accomplish this. Allows default to go back to what it was before the update and gives people the options of easily editing with a simple filter in functions.php file.

#13 follow-up: @obenland
8 years ago

I think for 4.4.1 we should just revert the change. It would have been nice to catch that before release. It was introduced in the first third of the release cycle.

#14 in reply to: ↑ 13 @peterwilsoncc
8 years ago

Replying to obenland:

I think for 4.4.1 we should just revert the change.

Yep, that works. I've forked the enhancement that is my patch to #35206.

#15 @obenland
8 years ago

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

In 36081:

Menus: Bring back line break between menu items.

While removing line breaks was great for all use cases except justified menu
items, it broke menus with justified menu items.
Reverts [34321].

Props wp-architect.
Fixes #35107.

#16 @obenland
8 years ago

In 36082:

Menus: Bring back line break between menu items.

While removing line breaks was great for all use cases except justified menu
items, it broke menus with justified menu items.
Reverts [34321].

Merges [36081] to the 4.4 branch.
Props wp-architect.
Fixes #35107.

Note: See TracTickets for help on using tickets.