Posted by hass on March 15, 2008 at 10:50pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| theme_link_broken_li_active.patch | 714 bytes | Ignored: Check issue status. | None | None |
Comments
#1
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?
#2
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":
<?php// Append active class.
if ($path == $_GET['q'] || ($path == '<front>' && drupal_is_front_page())) {
?>
Attached patch is rolled for D7, above for D6.
#3
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?#4
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.
#5
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.
#6
Committed to CVS HEAD. Thanks.
#7
Automatically closed -- issue fixed for two weeks with no activity.