Primary/secondary links don't get 'active-trail' class

subscribing.

Tiny patch, works well. I think it's fine to have the same class as it does when a menu is in a block, ids are fine if someone needs to differentiate, and one day these might be themed blocks rather than special cases anyway.

I've committed this to CVS HEAD. I leave it up to Gabor to decide whether this should be backported to Drupal 6.

seems like a bug to me... i couldn't figure out why the menu weren't activating. patch applies to DRUPAL-6 with a little fuzz but works correctly.

Thanks, committed to Drupal 6 as well.

A german videotutorial shows how to fix the problem:

http://kommune3.org/tutorials/drupal-6/fehlende-active-trail-klasse-im-p...

---
Kommune3 Media
www.kommune3.org

At this time, i dont know how the Unix Terminal work. It is possible to post the new "menu.inc" for Drupal 6.2?

buntstich

i add the Patch witdh a editor - the new file includes/menu.inc you will find in the attachment. You have also add these class in your theme stylesheet:

#navigation-primary ul.primary-links li a.active-trail {
background-color:# example;
background-repeat: example;
color:# example;
}

buntstich

This patch only assigns the class "active-trail" to the <a> tag. It should assign it to the parent <li> as well. This is how the "active" class is handled.

Strike that. It does appear to apply the "active-trail" class to the parent <li>. In my case, however, it's only showing up in the <a> tag.

<li class="menu-127"><a href="/taxonomy/vocabulary/1" class="active-trail">Research</a></li>

Can anyone offer thoughts as to why this might be happening?

Hum. I agree with scottatdrake on that one. The use of 'active-trail' is inconsistent:

• For menu tree, the 'active-trail' class is added only to the <li> tag by theme_menu_item()
• For primary/secondary link, the 'active-trail' class is added only to the <a> tag by menu_navigation_links()

Is it inconsistent twice: both on the tag used and on the type of function that add the class (a theme function vs. a normal function).

Here is a patch to the theme_links() function in theme.inc that adds the "active-trail" class to an enclosing <li> element if the enclosed <a> link has it. It appends the following code right after the "active" class test:

      // Add active-trail class to <li> if enclosed <a> link has it.
      if (isset($link['attributes']['class']) && stristr($link['attributes']['class'], 'active-trail') == TRUE) {
        $class .= ' active-trail';
      }

Of course, you could just use this patch in an override of the theme_links() function in your template.php file. But this seems like a generally useful feature, especially for themers trying to create advanced primary menu effects, so I'm submitting it for consideration in HEAD.

I'm not sure if the theme layer is the right place to address this issue (rather than, for example, in menu), but it's the place where the active class gets set for the <li> element, so it seems reasonable.

I've tested the patch but would appreciate feedback on anything I might be missing.

Nice! I suspected that there was a more fundamental level at which to apply the active-trail class to <li>. Definitely a cleaner solution than my hack. Consider my patch withdrawn in favor of doing it in menu.inc instead.

Any chance of this going into 6.6, rather than waiting for 7? In the meantime, I can continue to use my hack to theme_links() until it shows up in core.

Thanks.

I tested the above active-trail-li-249571-15.patch against one of our themes under Drupal 6.5 and it appears to work as advertised (and desired!). It indeed puts the active-trail class on the <li> instead of the <a> tag. In my testing it gets a thumbs up.

Just tested it again and I agree. Cleaner, clearer, and it still works. Looks good better.

I just love patches that remove convoluted logic whilst fixing bugs. Yay!

Committed to HEAD. Marking back to 6.x for consideration.

While I'm very eager for this to get committed to 6.x, there is one area of potential breakage: if a theme has some effect based on the active-trail class on the <a> element, then that effect will be broken.

This is because the patch moves the active-trail class to the <li> element, which is where it should be.

So if a theme has, for example, a background image set for #primary-menu ul li a.active-trail, then it'll break unless it's changed to #primary-menu ul li.active-trail a.

One way around this would be to leave the active-trail class on the <a> element as well, but that's not good markup practice (i.e., redundant class naming rather than relying on class inheritance).

I guess I'd prefer to see the patch go through as it currently stands, because it's best practice, it's fixing a bug and Drupal shouldn't have to worry about breaking workarounds for bugs, which is what designing for active-trail on the <a> element is.

But I can't speak for others and I'm not sure what the general exposure would be on this. Our (TNT's) current exposure is about 4 themes out of a couple dozen.

The patches do not work with Views and not with Path-auto on D6.5.

@pwolanin - yeah, it's not a problem for us. We have a number of options going forward. Just wasn't sure about other folks' themes.

If you put the full path in, it's treated as an external link, and Drupal won't know to apply the class to the <li>. That'd be a different issue to this patch, and probably 'by design'.

Huh, hah. I am not sure changing the $links keys are a great idea at this point. I expect themes might be hung on those keys to theme menu items specifically. I've at least seen themes which were applying special styles tied to the specific menu items. Adding a class in there would quickly break those themes (plus moving around the class is just a possible problem for these kinds of themes). Would be great to have feedback from others as well. We've heard that several of TNT's themes are affected.

I think it'd be worth adding a note in the release notes and/or maintainer news about the change. But this is enough of an annoyance that we really ought to fix it. The other option would be keeping .active on both the <li> and <a> but I imagine we'd get even more complaints if we did that.

I'm all for getting this into D6. It'd let me pull some nasty cruft out of my theme. It'd be nice if this was done as part of a non-security release so that people could update when it was convenient rather than in a rush to get the security fixes in.

I've talked through this with Peter again. He looked into the contrib repo and identified that our contrib themes are not affected. With all the support of the people here, I am leaning towards committing the patch. It does not apply to Drupal 6 however.

There was one 7.x artifact in it, concatenation spacing. I've fixed that and committed this one.

I agree that the methods described above are slightly cleaner, requiring only one class, but this would be slightly inconsistent with the block menus and the way the links in the headers "almost" work...

In order to have zero or little impact on existing themes, maybe maybe the LI class should be "active" and the "active-trail" class should remain on the links. This potentially should have zero or little impact on existing themes, because this method partially exists.

For example, in the current Drupal 6.4 release, I have a first level of primary links in the header styled as tabs, and primary links in the left block menu with access to all sub-levels.

I know people have gone over the problem, but it's just to accurately illustrate my example...

When I click on a header tab link or the equivalent top-Level link in the left hand block menu, the following is applied in the html...

<li class="menu-144 active"><a href="/drupal-6.4/?q=node/4" title="Services" class="active-trail active">

However, when I click on a Sub-level link on the left hand side (block) menu, I get this...

<li class="menu-144"><a href="/drupal-6.4/?q=node/4" title="Services" class="active-trail active">

The "active" class disappears from the LI item at the tabs at the header (but in a more consistent manner an "active-trail" class remains at the top in the block menu LI). If the "active" class was simply to remain on the LI top level list item, then all issues would surely be resolved.

Alternative would be to replicate the structure of the block menu and applying the same "active-trail" class to the LI and the link, but "nesting" class-names on different presentational/functional elements in this case can lead to bloated CSS styling, something we should try to avoid doing twice.

It's only a thought, and perhaps I've got the wrong end of the stick on the exact issue people are discussing. So don't shoot me in flames just "yet". With things being committed, I'm probably too late now anyway. :D

> So if a theme has, for example, a background image set for
> #primary-menu ul li a.active-trail, then it'll break unless it's
> changed to #primary-menu ul li.active-trail a.

Thank you for telling me how to fix my menu, after you guys broke it!
Erm, I mean fixed it. Uh... I don't know what I mean! :)

Very thoughtful!
Keep up the good work.

Hello,
I am still having this issue in Drupal 6.6. I have a book and a link to one of it's pages in Primary Links, and it doesn't get "active-trail" class whene I am navigating inside it, whereas the active-trail class is applied to the correct links inside book navigation block...
Oh, and I'm using a Zen subtheme.

Thanks for your help,
David

This is still happening when on an active sub-page, primary links don't get the active class and $primary_links don't carry it either. This issue is also a duplicate of http://drupal.org/node/202245 which seems like it was forgotten for D6. Menu trails issues could also be related to this.

hummm... well your patch would apply to HEAD still so i've included that as well as a properly re-rolled D6 patch but I haven't tested either.

Indeed, this issue happens when theming, however shouldn't the link carry the in_active_trail info also? It doesn't change anything, simply passing the flag along (in its trail). It simply allows more flexibility in a one liner. Could modules also be looking for this in_active_trail flag?

Hi,
I asked my provider to patch my drupal 6.12 with this:

Index: includes/menu.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/menu.inc,v
retrieving revision 1.268
diff -u -p -r1.268 menu.inc
--- includes/menu.inc 14 Apr 2008 17:48:33 -0000 1.268
+++ includes/menu.inc 22 Apr 2008 02:00:25 -0000
@@ -1257,6 +1257,9 @@ function menu_navigation_links($menu_nam
$l = $item['link']['localized_options'];
$l['href'] = $item['link']['href'];
$l['title'] = $item['link']['title'];
+ if ($item['link']['in_active_trail']) {
+ $l['attributes'] = array('class' => 'active-trail');
+ }
// Keyed with unique menu id to generate classes from theme_links().
$links['menu-' . $item['link']['mlid']] = $l;
}

He received:

patching file menu.inc
Hunk #1 FAILED at 1257.

Any suggestions?

Thanks a lot in advance,
Marko

Proper status. Though I'm not sure about the proper status. Maybe you should create a new issue.

I think there are two issues, one is that in early drupal 6 and earlier releases, or in certain themes, the li does not include the "active-trail" class. The other is that on later versions where the li class is correct, the a does not include the "active" class. My simple CSS solution below applies only to the second one. Here is what I posted on a different thread:

I'm not sure exactly where to post what worked for me since there are so many threads open on this one subject, but this works for me in D6 using Barlow theme. I'm using the Primary Menu as parent menu and the secondary menu as the children of each separate link (secondary links set to primary links)

ie)
[link1] link2 link3
childoflink1 childoflink1 childoflink1

link1 [link2] link3
childoflink2 childoflink2 childoflink2 childoflink2

I used this in my stylesheet

.links a.active:visited, .links .active-trail a{
color:#990000;
text-decoration:none;
}

When I looked at the HTML source, the li had the correct active-trail class set, but the a did not include the active class and therefore was not working properly. If you do not have the li active-trail class, it could be because I installed the "Menu Trails" Module http://drupalmodules.com/module/menu-trails.

(I included the a.active:visited because the first link of the secondary child menu worked and the selected primary menu parent link included the active class as it should, it was just the rest of the child links that when clicked on, the parent link lost the the active class.)

This can be considered a work around, it's not really a good solution. Hope this helps at least as a temporary solution, I don't have the skill or the time to change the php to say if li class="active-trail", then a class="active".

I think we can and should just do this.

Part of #907690: Breadcrumbs don't work for dynamic paths & local tasks #2

I have created SecondaryLink and I want when user will hit on that
that menu will be selected can any one help me

Reverting status.