I figured out a bug in the theme_links() function for menu links. I found the bug http://drupal.org/node/173459 and solved this bug the same way.

Repro:
1. Add a menu link with path <front> to your primary links
2. Add some more menu link, but this is not required - this is only to verify the bug
3. Click on the <front> link to get it active
4. See source what is marked active and you will see the link is marked 'active', but the <li> *not* (BUG).

This makes styling of the active LI element absolutely impossible. Please get this trivial patch into for 6.2... it bugs me any my theme users.

CommentFileSizeAuthor
#2 theme_link_broken_li_active-HEAD.patch716 byteshass
theme_link_broken_li_active.patch714 byteshass

Comments

damien tournoud’s picture

damien tournoud commented
Version: 6.x-dev » 7.x-dev
Priority: Critical » Normal
Status: Needs review » Needs work

That bug was fixed by #173459: If <front> is added to menu, the link is never marked 'active' in DRUPAL-5, but was never ported to DRUPAL-6. Let's do not make that mistake again: bugs have to be fixed in HEAD, than back-ported.

Could you re-roll the same patch for HEAD?

hass’s picture

hass commented
Status: Needs work » Needs review
StatusFileSize
new theme_link_broken_li_active-HEAD.patch716 bytes

It haven't been documented, but the bug in #173459 is fixed in D6. This bug is new and related to LI only not the link.

See "common.inc":

  // Append active class.
  if ($path == $_GET['q'] || ($path == '<front>' && drupal_is_front_page())) {

Attached patch is rolled for D7, above for D6.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Hum, ok. Thanks for the clarification.

The above patchs only do implement the solution of #173459 in theme_links(), so marking this as ready to go in. Is there other parts of the core where this could be needed?

hass’s picture

hass commented

I cannot say for sure if there is more broken... i only know #173459 is fixed and struggled on trying to style an LI missing the 'active' class. THX for review.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

damz: http://drupal.org/node/173459 you linked to (which adds an extra check into l()) is also fixed in Drupal 6, l() also includes that code there.

Committed to 6.x. Still needs to go to 7.x.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.