Usability: Menu items in active trail should be styled

I agree. Otherwise the menu functionality is excellent in Drupal. But because of this bug, context sensitive menuing is serioulsy broken.

I've solved this before with custom theme functions in template.php instead of the default theme_menu* functions.

Checking template.php from the Mollio theme might give you a good starting point, but I agree it would be nice to have this as the default core behaviour.

I'd like to see this fixed. No idea where to start looking though, apart from "somewhere in menu.inc" ;)

+1 I'd like to see this fixed too!

Yes, I'd like to see this fixed too. It's an important usability issue. FYI, the least dirty solution to this problem I have seen so far is this one.

Seeing that 5.0 is coming around and that some other changes have been made to the menu system already (i.e. primary and secondary links returning structured links, see here), I have changed the status from normal to critical again. If we're going to improve the menu system in 5.0, we better do it really good.

After hunting through the code, it turns out that the function responsible for setting class="active" on menu links is NOT in menu.inc. It is the l() function in common.inc (and therefore, class="active" should also be getting set for all other links generated by l(), if there are any others that link to the current page).

I have attached a new patch, that does the following things:

1. Modifies the l() function, to output a special class not just for links to the current page, but also for links to a page in the current active trail.
2. Outputs this special class as a new class called 'active-trail', rather than re-using the existing 'active' class. This allows 'active-trail' links to be styled differently to 'active' links. For example, some themes may want to style it in a different colour to 'active', while other themes may not want it to be styled differently to normal links at all (i.e. they may want it to behave just as it does now).
3. Adds some 'active-class' styles to the core CSS, i.e. menu.css and the CSS for the core themes.

@kiteatlas: while the approach in your patch works, I think that mine is an improvement, because it is consistent with the way that things currently work in core (i.e. the logic is all in the l() function, rather than in menu.inc functions), and because it provides the potential for any other 'active-trail' links to be styled (e.g. tabs, primary/secondary links, non-menu-item links, etc), rather than just sidebar menu items.

Please see below for some screenshots of what the core themes look like with this patch applied. Feedback on both the code and the new colours is welcome (I'm no artist, so I may have picked ugly 'active-class' colours!).

Note that although this patch adds the 'active-trail' class for all relevant primary and secondary links on a site, it does not make ALL of the core themes apply any special styling to these links. Bluemarine and Pushbutton currently don't apply any special styling to 'active' primary/secondary links, and so this patch keeps that behaviour consistent, and doesn't apply any styling to 'active-trail' primary/secondary links for these themes.

However, Chameleon and Marvin DO apply special styling to 'active' primary/secondary links, and this patch extends that styling to include 'active-trail' primary/secondary links. For other themes, of course, whether and what to style is completely up to the themer. :P

Attached screenshot shows what 'active-trail' primary links look like in Chameleon with this patch applied.

This patch only adds "active" to the link, most often a much better option (design-wise) is to set it on the surrounding LI.
The other benefit is that its easy to pass a class from an LI down to the contained A, but impossible the other way around:

 <li class="item <?print $active ? " active" : ""  

"> print theme('menu_item' $item, $active)

?>
is simple, but once the class is only available inside the theme_menu_item() you cannot extract it and print it in the LI.

But taken that into consideration, this patch is better then nothing. At least far better then what we have now.

Some alternative way to achieve this at http://webschuur.com/node/658

I complete agree with Bèr. This patch needs some work, because it's the

(note to myself: "never, but never give examples of tags using the TAG, and ALWAYS do a post preview")

I complete agree with Bèr. This patch needs some work, because it's the LI tag that is active and not the A tag.

When we are talking about styling this, if the active class is only on the link we can do nothing about styling it's LI container.

attempt to fix HTML

moving. +1

moving again (+1: active primary links aren't highlighted when in a subfolder)

+1 for this functionality. You can fake it now by writing your own functions to generate the primary_links which pass along the $mid. Basically we want to get it to where any menu_link checks its own id against the active trail and adds a class. I've done this with theme_menu_item_link by passing it the $mid and doing as follows:

function phptemplate_menu_item_link($item, $link_item, $mid = NULL) {
  $attributes = !empty($item['description']) ? array('title' => $item['description']) : array();
  $trail = _menu_get_active_trail();
  if (in_array($mid, $trail)) {
    $attributes['class'] = 'active';
  }
  return l($item['title'], $link_item['path'], $attributes);
} 

Here's another alternative. Sets an 'active-trail' class to li instead of anchors (agreed with Bèr Kessels). I really needed this feature and the previous posts lead me to this..

function theme_menu_item($mid, $children = '', $leaf = TRUE) {
  $active_class = in_array($mid, _menu_get_active_trail()) ? ' active-trail' : '';
  return '<li class="'. ($leaf ? 'leaf' : ($children ? 'expanded' : 'collapsed')) . $active_class .'">'. menu_item_link($mid) . $children ."</li>\n";
}

I don't know if something changed, but this seems to be almost working ok for me in 5.0 now... The only change I had to make was in menu.inc, changing $index .= "-active"; to $index .= " active";

Agreed, this functionality needs to be there, such a pain for us themers *heh*

This works in Drupal 5 if one sets the menu entry to the actual path of the home page. For instance if you are using the default front page of 'node' and set the path to 'node' the home page primary link is set to active, even at the base url: http://example.com/ and not just /node.

Marking this fixed, I don't see the need to add more code for this.

I believe this is still an issue. If you set your menu item to match your front page path, your path will be added at the end of the url. Not very nice looking, and could potentially hurt your SEO. One shouldn't have to hack to get an acceptable default behavior.

Yes - I really think this needs to be fixed. I don't want home page links to be 'mysite/node'!

Making a note that this was incorporated into D6. :-)

http://api.drupal.org/api/HEAD/function/theme_menu_item

Whoops. Setting it to fixed.

But is there yet a clean solution for those of us that still need to develop in D5?

Yes this might be a duplicate of http://drupal.org/node/274532, I just got frustrated working my way through the whole thread to then finally see what I perceive as a "it's not so important" attitude, when so many less wizardly users have been expressing frustration over this core issue for so many years.

Moving all usability issues to Drupal - component usability.

This is not completely fixed in 6.16 either.

component got messed up

Considering that no new features will be added to D5 and that no one has shown any interest in this issue for a long time, I am closing this issue ticket. If you think we still need this feature request, please reopen it and move it to the D8 issue queue.