New Nav Menu needs to have ancestor class

[joostdevalk]

In the new menu functionality, the only thing that's missing from the "old" wp_list_page method is the parent / ancestor class: I'd like to highlight a menu item even if that menu item's page is only the ancestor page of the current page, to highlight the current section.

To do this, we have to simply match the menu item against the current post->ancestors array. Patch attached.

[joostdevalk]

Patch v1

[Denis-de-Bernardy]

post ancestors isn't always set (there's a ticket on this with that name, more or less)

[joostdevalk]

I've got another patch, which changes the class to menu-item-ancestor and adds menu-item-parent as well. If post ancestors isn't always set, that should be fixed too :)

[joostdevalk]

Patch v2

[ryan]

Instead of using global $post, let's try $wp_query->get_queried_object().

[ryan]

Is that the right place to put that code? Should it be in Walker_Nav_Menu?

These aren't really menu item parents and ancestors. They are page parents and ancestors. Menu item hierarchy doesn't have to follow page hierarchy. We need to keep in mind the difference.

[Denis-de-Bernardy]

For reference: #10381

[daltonrooney]

I am running 3.0 beta 1 and I found this feature missing as well. I applied the patch and it works as expected. I hope this makes it into the final release.

[daltonrooney]

This new code works for me on pages and single posts, but throws an error on the blog homepage.

The error I get is

Warning: array_values() [function.array-values]: The argument should be an array in /home/dalton/public_html/nk/wp-includes/nav-menu.php on line 227

[xibe]

Patch refresh.

[filosofo]

I think the proposed patch is the wrong way to do this. wp_setup_nav_menu_item decorates a menu object. If you think of menus in terms of MVC such objects are part of the model, not the view (and it's the view that handles things like context-specific HTML classes).

I'm not just objecting out of academic concern for architectural purity or good software design. wp_setup_nav_menu_item has a clear, discrete purpose which is independent of global state. Trying to account for global state here almost guarantees bugs or hacks to get around bugs in the future.

[gvvaughan]

Replying to ryan:

These aren't really menu item parents and ancestors. They are page parents and ancestors. Menu item hierarchy doesn't have to follow page hierarchy. We need to keep in mind the difference.

Agreed. The patch just happens to work when upgrading from a blog which previously built a menu from the page hierarchy, provided that you build a new wp_nav_menu hierarchy that matches.

I installed the patch after doing just that, and it appears to do the right thing. BUT after going into the Edit Pages admin page and setting all pages to 'No Parent', the patches in this ticket are no longer able to decorate menu items with the correct (or any in fact) menu-item-parent or menu-item-ancestor classes.

I don't know how to get a list of the children of the current menu item, otherwise I would have provided a correct patch... if someone can enlighten me on that detail, I will write the patch though.

[kpdesign]

I ran into this issue also on a site I'm developing using 3.0 and the new nav menu functionality. After searching the wp-testers list, I found an email from Andrew Nacin that talked about this same issue and mentioned an open Trac ticket for it.

Would like to see this resolved; unfortunately I'm not able to provide a patch to fix it. :(

[husobj]

Replying to ryan:

These aren't really menu item parents and ancestors. They are page parents and ancestors. Menu item hierarchy doesn't have to follow page hierarchy. We need to keep in mind the difference.

Ryan, you have a point there - should a menu ancestor be highlighted if the current selected menu item is a child of that menu item (but not necessarily a child of that page)?

... and should the menu item be highlighted if the current page is a child page of that ancestor (but not necessarily a child menu item)

My opinion is that a menu item should be highlighted if the current page is a child of that page OR a child menu item of that page.

Just my 2 cents :)

[ryan]

Doing the post parent and category parent instead of the real menu parent is better than nothing. We definitely need to get Home working.

[filosofo]

I'll make a patch to add parentage classes.

[ryan]

Going to bump this up to blocker because it's an important piece of menu functionality that would be really nice to have in 3.0.

[filosofo]

nav-menu-parent-classes.12812.diff sets the classes property of menu item objects, so that each menu item indicates via class name when a menu item is the parent of the currently-queried thing: it indicates both when it's the parent of that thing's corresponding menu item and when it corresponds to the real parent of the thing.

• current-menu-item: A class value of the menu item when it corresponds to the currently-requested page.
• current-menu-parent: A class value of the menu item when it is the parent of the currently-requested page.
• current-[category, page, post, etc.]-parent: A class value of the menu item when the object to which it corresponds is the parent of the currently-requested object.

[ryan]

(In [14876]) Add parent classes to nav menu. Props filosofo. see #12812

[ryan]

Do we want to add current_page_ancestor, current_page_item, and current_page_parent for back compat? I think we'd only apply them to actual pages and home.

[joostdevalk]

Replying to ryan:

Do we want to add current_page_ancestor, current_page_item, and current_page_parent for back compat? I think we'd only apply them to actual pages and home.

Might be smart and save us some crying in the forums and blogs :)

[ryan]

Does twentyten have any current menu item styling?

[ryan]

We might also want to add the new ids to the page walker so that theme authors can target the new selectors and still have them work when wp_page_menu() is used as a fallback.

[filosofo]

add-ancestor-classes.12812.diff adds the ancestors.

If you decide to hack in current_page_ancestor (underscores), it could be done in the Walker class with str substitution.

[ryan]

(In [14881]) Add ancestor classes. Props filosofo. see #12812

[ryan]

(In [14882]) Active menu styling. Props ianddtewart. see #12812

[ryan]

(In [14900]) Back compat menu classes. see #12812 #13379

[ryan]

This warning is generated when clicking a category link:

PHP Notice:  Undefined property: stdClass::$post_type in /.../wp-includes/nav-menu-template.php on line 364

And this one for the same line when clicking a custom link:

PHP Notice:  Trying to get property of non-object in /.../wp-includes/nav-menu-template.php on line 364

[ryan]

[14905]

[ryanhellyer]

HTML showing ancestor/parent bugs

[ryanhellyer]

This does not appear to be fixed, in fact I think it might be worse than before. The HTML attached, is a chunk of code from my test install running WP 3.0 beta 3.

To create that code, I set a Page to be at the fourth level, the list item gets classes of "current-menu-item" and "current_page_item", which makes complete sense.

However ... the parent of that Page, regardless of whether the parent is in fact it's "Page parent", is given the class of "current_page_parent" as well as the expected "current-menu-parent". Since it isn't actually the current pages parent and is just a parent in the menu, it shouldn't be given the "current_page_parent" class IMO.

Then it gets really bizarre, as the actual top level page ancestor, regardless of where it is in the menu structure, is given a class of "current-menu-ancestor" despite not being an ancestor within the menu structure and also classes of "current-page-parent" (expected) and "current_page_ancestor" (unexpected).

Unfortunately I don't have a solution :( I'm just pointing out the problem in the hope that someone with more knowledge than I can help figure out a way to make it work!

Thanks to Diana for bringing this to my attention :)

[filosofo]

Replying to ryanhellyer:

However ... the parent of that Page, regardless of whether the parent is in fact it's "Page parent", is given the class of "current_page_parent" as well as the expected "current-menu-parent". Since it isn't actually the current pages parent and is just a parent in the menu, it shouldn't be given the "current_page_parent" class IMO.

This was done deliberately to be backwards-compatible with wp_page_menu styling.

Then it gets really bizarre, as the actual top level page ancestor, regardless of where it is in the menu structure, is given a class of "current-menu-ancestor" despite not being an ancestor within the menu structure

That does sounds like a bug. I'll investigate.

and also classes of "current-page-parent" (expected) and "current_page_ancestor" (unexpected).

Why is current_page_ancestor unexpected? Isn't it the current page's ancestor?

[ryanhellyer]

Why is current_page_ancestor unexpected? Isn't it the current page's ancestor?

Because in this particular case it is the immediate parent of the current page and therefore is the page parent, not the page ancestor.

[filosofo]

Replying to ryanhellyer:

Because in this particular case it is the immediate parent of the current page and therefore is the page parent, not the page ancestor.

That's both the meaning of "ancestor" and WP's prior terminology with pages and categories: a parent is an ancestor.

[nacin]

"That does sounds like a bug. I'll investigate." filosofo->reviewing?

[ocean90]

I get this view ​http://grab.by/4Vqn. It happens if I had an item twice and for example I click the first 'Tags A and C' link. The HTML output:

<ul id="menu-testmenu" class="menu">
<li id="menu-item-4" class="menu-item menu-item-type-custom"><a href="http://localhost/wp/">Home</a></li> 
<li id="menu-item-402" class="menu-item menu-item-type-post_type current-menu-item"><a href="http://localhost/wp/blog/archives/166">Tags A and C</a></li> 
<li id="menu-item-403" class="menu-item menu-item-type-post_type current-menu-parent"><a href="http://localhost/wp/blog/archives/162">Tag B</a> 
  <ul class="sub-menu"> 
    <li id="menu-item-413" class="menu-item menu-item-type-post_type current-menu-item"><a href="http://localhost/wp/blog/archives/166">Tags A and C</a></li> 
  </ul> 
</li> 
</ul>

Perhaps ryanhellyer means this.

[nacin]

ocean90: Nothing we can do about that; we don't know which menu item was clicked.

[ocean90]

Last patch works good for me. Screenshot: ​http://grab.by/4VUD
The active menu is 'page with comments'. Without the patch the item 'Tab b' will not be highlighted.

[ryan]

(In [15257]) Menu ancestor fixes. Props filosofo. fixes #12812