Language switcher block highlights ALL languages as active

I ran into this as well. We were trying to use JQuery to turn the language links list into a select box and couldn't correctly set the current language because they're all 'active'.

Just subscribing.

I feel like the "correct" fix (for 7.x) would be to have l() and theme_links() call some new function drupal_is_active($path) which would test if a path is the active path. Along the way it would calling language_url_rewrite() to get the prefixes inserted.

The short term fix for the local.module in 6.x would be to ditch the call to theme_links and add a custom theme function that is prefix aware.

marked #254251: Language switcher - all links are active as a duplicate

had the same issue; here's the fix (rfc).

haven't gotten to test this but it'd need to go into HEAD first and then be backported to 6.

well this applies to 6.x aswell as 7.x, same code. And this definitly fixes the problem, no side effects for now.

Attached the patch for D7, but indeed the same code :-). But I don't see the reason to patch 'theme.inc' as eMPee584 is doing in his patch in #6. Just a one line patch for 'common.inc' is doing the job for me, or do I miss something?

Desbeers u r indeed missing something, the patch for common.inc fixes links put out with the l() function; the theme.inc file however contains theme_links() which suffers from the same problem, so both need patching.
The patch i attached applies to D7 aswell as D6.

:-) subscribing

ximo, gabor, verikami: could you please test this patch and report back if it solves the problem for you to speed up inclusion in D6/7 ?

Recategorizing this to raise attention.. this is a bug in the core inc files really damaging the user experience on multi-language sites so why does it not get committed, the fix i posted is so obvious...

it's not helpful putting it back in 6 since patches only get committed there after they've landed in HEAD.

well ok sorry i just felt maybe this is being ignored because the patch queue for D7 is that giant...

is someone finally going to commit this? it's really a trivial fix and much less helpful stuff was committed on the d6 branch since this was posted...
..?

@eMPee584: three things:

1) First, welcome to the 21st century, we can classify issues here, no need to prepend "PATCH:"

2) Second, no review of your patch for a long time doesn't mean the patch is right. It just means it has not be getting the attention it (probably) deserves. Your patch touches a very sensitive part of Drupal (url() is called several times on every page), so it *needs* to be reviewed.

3) Third (finally, about the patch!), the logic of your patch looks sound, but the implementation is wrong because you compute url($path, $options) two times, while url() is *not* a cheap operation.

subscribing...

@damien:

1st thank you for your feedback... in fact i know url is not cheap, that is why i'd appreciate any proposals on how to a) directly benchmark and compare speed impacts and b) how to do it better. In any case, the current behaviour is *wrong*, and (at least from my naive perspective) a fast implementation that does the wrong thing is to be considered worse than a somewhat slower variant that does things correctly.

@eMPee584: if we can't avoid adding url() call, fine by me. But at least, please do not call url() two times with the same path and options.

well it did fit into the existing URI comparision logic, but i'm gonna have another look at it..
...static caching is a bad idea for this isn't it?

How large a bounty would it take to get a fix for this issue committed in a release?

I had this same issue in D6, and the patch in #6 solved it.

+1 for committing this patch, and hopefully that will inspire a new patch that solves the url() issue.

When I upgraded to Drupal 6.6 the #6 patch did not work anymore (cleared cache of course). <li> and <a> have both "active" class for all the languages. Switching back to 6.5 makes it work again.

> li and a have both "active" class for all the languages

Yep, and active-trail was also changed during 6.5 -> 6.6.
Luckily the theme-fix is easy for that.

Sorry.. double post.

By the way, it happens only on frontpage (with or without the #6 patch). On other pages, it puts the "active" class on the current language which is what should happen too on the frontpage.

**EDIT**
Well, it does it also on a Panel page (that I can't set translation on since it is not a common node). So maybe those links/list items should be tagged with a specific class (like "current-language") according to the current language (or if not present the default language... according to language negociation results) ?

To those who are working on multi language sites and are impatient for a bug fix to core I just released a module that provides a language switcher that assigns the active tags separately from core so it doesn't have this issue. It also has a couple of other behavioral differences. To download it go to http://drupal.org/project/languageinterface .

While waiting for a better fix, I would like to post an implementation (tested with D6.6) of the first solution offered in #1 (point 1) using translation_link_alter. Place it in a custom module :

function <yourmodulename>_translation_link_alter(&$links, $path) {
  global $language;
  foreach($links as $key => $link) {
    if ($link['language']->language == $language->language) {
      $links[$key]['attributes']['class'] .= ' active-language';
    }
  }
}

And use this new class "active-language" to style it, instead of "active".
It would be possible to use the $key directly to compare with the current language (since it equal to the language of the link). But I am not sure that it would be a good idea, it is not guaranteed that this key will never change (with further updates of locale.module), when the language property of the link is not likely to be changed in my opinion.

Hope this helps, of course it does not solve the problem that "active" is set for every <li> and <a>, but as I said it does the trick for me while waiting for the core to implement this (or any other better solution... but I don't see how this could be done otherwise).

Investigating this further lead to understanding which in turn contributed greatly to fix it properly (without calling url()). For the theme_links() function, that does the same check and would have to be changed in the same way, however another option would be to drop the active class for the list element the link is contained on...

The solution looks correct.

An additional comment before the main change in l() would be useful. Suggestion:

// If a language is set, only set the active class if it's the current language.

We need to confirm that this will not skip needed active links. What are the current places we send a language in the $options array, besides in locale_block()? Are there any cases where we would have a language-specific link (i.e., send a language as part of the $options array to l()) but want 'active' irrespective of the current language?

well in that case one would better make sure that the link's language attribute is not set..

Here is a test for that issue. It will show you that the patch in #32 only fixed part of the issue: the anchor (the <a> tag) for non-active languages is not marked as active anymore, but the list item (the <li> tag) still is.

well here's the patches including the change to theme_links aswell. And thanks Damien for that test!

I think having the active class on the list item and the link can be very useful in certain styling conditions so thanks for this last patch. It works very well (also for other lists than the locale block of course), tested with Drupal 6.6.

Thanks eMPee584 :)

Included the test from #35 into the patch from #36.

subscribing :-)

Well it's a tight race between this fix being merged into D6 and hell freezing over.. i think hell is gonna win this one.

@eMPee584: Well, this issue is still marked "code needs review" so won't necessarily have come onto the radar of the core committers.

Well if there's no policy against that, let me do it.. done. Hell's still ahead by lengths though.

@eMPee584: You can find some information here on project issue status settings: http://drupal.org/node/156119, including the general advice "One should not set their own patch to [RTBC] status."

When marking an issue RTBC, it's important to provide justification, to help the core committers understand why this status is appropriate. E.g.: Does the test provided successfully identify the problem? Have you thoroughly tested the latest version of the patch? Do the test(s) now pass? Are all problems previously identified in the issue addressed?

I haven't found time myself to do such testing. I'm setting this back to code needs review pending a bit more detail.

Well ok so there *is* a policy against doing that..
Anyways, Damien provided the tests, and it is running live on my site since weeks without issues...

@eMPee584 (#40): why the hell it is not in D6....????

I have 3 multilang sites on D6 and have to still remember about this patch...

@verKami: because not enough people are testing the posted patch and shouting 'yes it works' (which indeed it does)!

'yes it works' ! (patch of #38) :)
Thanks

@eMPee584: "Yes it WORKS !!!" (doing 'patchwork' on D.6.6 and thinking about D.6.7)

on my every Drupal site & on every core version update !!!

I take it this patch didn't make it into D6.8?
Will it make it into D6.9 (assuming there is one)?

Thanks.

waiting for patch to be included in next release!
(commented for subscription)

...probably in D.6.999... :-(

Please do not change version unless you have a good reason to.

Once it makes it into 7.x and then is back ported it MAY be included in the 6.x series.

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

Just FYI, I am around in #drupal at least 12 hours a day. If you need attention drawn to a patch, talking about it in there is generally a better use of time than making snarky comments about it in an issue. Thanks.

:-))) webchick - Thanx !!!

And also big thanks to all people involved with this patch. I think no one here (especially me) wanted to be snarky but only effective - we simply didn't know how to make this patch being included into the core.

subscribing

I applied the D6 patch in #36, applies with hunks on 6.9. Now the a tags have correct classes, but both li's still have the active class. Not sure if that meens this patch needs work, or if that's another patch.

Edit: Sorry, had a module installed that implemented the theme function, once that was updated it works fine.

Subscribing... I'm currently running a Drupal 6.9 multilanguage site with the changes from #36 applied, works as advertised.

@TheRec, thanks for the idea, it's working for me :p

I changed it a bit to hide the current language and to show the shorter names.

function <yourmodulename>_translation_link_alter(&$links, $path) {
  global $language;
  foreach($links as $key => $link) {
    if ($link['language']->language == $language->language) {
      $links[$key]['attributes']['class'] .= ' active-language';
      unset ($links[$key]);
    } 
    else {
      $links[$key]['title'] = $key;
    }
  }
}

Committed #36 to Drupal 6 as well. Thanks for all the testing!

@#38:

I'm using Drupal 6.9. Although the modifications worked for me, I had to put the first block for common.inc manually from the patch file, since I couldn't apply it automatically. Attached is the updated patch file (without the test).

Edit: I'm really sorry. I guess I didn't read the thread carefully and didn't realise the patch was actually committed in 6.10. Instead of applying a patch, I simply upgraded my 6.9 to 6.10 instead. Thanks for committing this patch! Please disregard mine.

Thanks to everyone for finally getting this fix into core.

I don't know css syntax. How do we use this - what's the new css?
The class now provided for the active language is: language-link active

At the moment by default, system-menu.css is providing the style:
li a.active {
color:#000000;
}

I don't know what to add to style.css to over-ride this, for styling the language switcher.

Thanks.

well anti maybe you need some more lines about how CSS selectors work *g
anyways li a.active applies to all <a> tags with class 'active' within an enclosing <li> tag - that is links inside lists which are marked as active.
Language links also have the class language-link (doesn't sound too far fetched huh) so to specifically target those you can use
li a.language-link respectively li a.language-link.active for active ones. Note the relatively uncommon chaining of class selector - that's all the magic there is to it.

Not sure my comment is on-topic here, but ---
I ran into a similar problem with the languageicons module, and, since my capabilities and understanding of the system are limited, I decided to draw the flag of the active language in 24x18 instead of 16x12 dimension.

If this is a generally acceptable solution, I can post a patch.

hello

is it included in the last drupal 6.11? or do i have to patch something?

thanks momper

Adding this to the style sheet fixed the problem in my Drupal installation.

#block-locale-0 .content li.active a.active{
font-weight:bold !important;
}

Where "#block-locale-0 .content" is the block in which the language links are placed,

@raj: please do not reopen bugs if they actually and in fact have been resolved. By the way, the assignee field is for someone adopting the issue to code for it. "Needs review" means that you actually have a proposal how to fix the bug - this is all not the case here.

Reopening the issue since the very same problem is there in the most recent dev release of 2010-Mar-05.

Here's the output:

<div id="block-locale-0" class="block block-locale">
    <div class="content">
        <ul>
            <li class="en first active"><a href="/drupal6/en" class="language-link active">English</a></li>
            <li class="et active"><a href="/drupal6/et" class="language-link active">Eesti</a></li>
            <li class="ru last active"><a href="/drupal6/ru" class="language-link active">Русский</a></li>
        </ul>
     </div>
</div>

I haven't read through this issue, it just popped up in the accessibility queue because of the use of the term accessibility in the original bug description.

Can I suggest that along with any patch required to fix the current bug, that you provide a way to indicate to screen-reader users which language is active. I am assuming that the active language is currently distinguishable through visual style alone.

For local tasks (tabs) we appended the following markup to the active tab.

<span class="element-invisible">(active tab)</span>

The .element-invisible class is a Drupal system class that hides content from all but screen-reader users and users with styles disabled in their browsers.

You might consider appending the following markup to the selected language, within the list item.

<span class="element-invisible">(active language)</span>

...related perhaps?: #1226178: Language switcher block doesn't render any language link as .active when in node edit form...

...honestly don't know how that tag got added there.