Or rather, all languages are marked with the class active, making it impossible to highlight the current language with CSS. This is certainly not very accessible, visitors should get a visual indication of the active language, and the language links should not all be styled as active. With accessibility/usability in mind, I'm marking this as a bug and not a feature request.

Files: 
CommentFileSizeAuthor
#64 language_220559_6.9_switch_without_test.patch1.68 KBremi
#38 220559-language-switcher-with-test.patch5.05 KBDamien Tournoud
Passed: 8999 passes, 0 fails, 0 exceptions
[ View ]
#36 drupal-active-language-fix-d7.patch1.73 KBeMPee584
Failed: Failed to install HEAD.
[ View ]
#36 drupal-active-language-fix-d6.patch1.65 KBeMPee584
#35 220559-language-switcher-test.patch3.09 KBDamien Tournoud
Failed: 7293 passes, 2 fails, 0 exceptions
[ View ]
#32 drupal-active-language-fix-d7.patch877 byteseMPee584
Failed: Failed to install HEAD.
[ View ]
#32 drupal-active-language-fix-d6.patch789 byteseMPee584
#9 active.patch701 bytesdesbeers
Failed: Failed to install HEAD.
[ View ]
#6 drupal_fix_language_active_path_check.patch1.02 KBeMPee584
Failed: Failed to install HEAD.
[ View ]

Comments

ximo’s picture

Status:Active» Postponed (maintainer needs more info)

I dug into the locale_block() function, trying to solve this..

The list of language switching links are themed using theme_links(), this is where the "active" class is added to the <li> elements. It's in l() that the "active" class is added to the <a> elements. And this is done because the language prefix is not taken into consideration when determining if the link points to the current page.

I've come to two alternatives...

  1. Add a class to the active language link in locale_block() (or using locale_translation_link_alter()):
    <?php
    global $language;
    if (
    $link['language']->language == $language->language) {
     
    $link['attributes']['class'] .= ' active-language';
    }
    ?>
  2. Make theme_links() and l() language prefix aware

Alternative 1 allows themers to style the active language - but all language links will still be applied the "active" class for every single page. Alternative 2 involves adding language prefix detection into already generic functions. I'm not sure that's the best idea..

I'm not happy with Alt 1, though. I hope Alt 2 is possible, or another solution will arise.

Will make some patches when I'm more awake..

drewish’s picture

Status:Postponed (maintainer needs more info)» 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'.

Gábor Hojtsy’s picture

Just subscribing.

drewish’s picture

Version:6.x-dev» 7.x-dev

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.

drewish’s picture

eMPee584’s picture

Version:7.x-dev» 6.x-dev
Status:Active» Needs review
StatusFileSize
new1.02 KB
Failed: Failed to install HEAD.
[ View ]

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

drewish’s picture

Version:6.x-dev» 7.x-dev

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

eMPee584’s picture

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

desbeers’s picture

StatusFileSize
new701 bytes
Failed: Failed to install HEAD.
[ View ]

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?

eMPee584’s picture

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.

abandoned’s picture

:-) subscribing

eMPee584’s picture

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 ?

eMPee584’s picture

Title:Language switcher block doesn't highlight active language» Language switcher block highlights ALL languages as active
Version:7.x-dev» 6.x-dev
Component:locale.module» usability

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...

drewish’s picture

Version:6.x-dev» 7.x-dev

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

eMPee584’s picture

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

eMPee584’s picture

Title:Language switcher block highlights ALL languages as active» PATCH: Language switcher block highlights ALL languages as active
eMPee584’s picture

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...
..?

Damien Tournoud’s picture

Title:PATCH: Language switcher block highlights ALL languages as active» Language switcher block highlights ALL languages as active
Status:Needs review» Needs work

@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.

PixelClever’s picture

subscribing...

eMPee584’s picture

@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.

Damien Tournoud’s picture

@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.

eMPee584’s picture

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?

Anonymous’s picture

http://drupal.org/node/280868#comment-920358

this works perfectly for me. =]

.block-locale .en .en a.active, .block-locale .hu .hu a.active{color:#ff0000;}

PixelClever’s picture

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

mandclu’s picture

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.

TheRec’s picture

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.

-Anti-’s picture

> 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.

-Anti-’s picture

Sorry.. double post.

TheRec’s picture

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) ?

PixelClever’s picture

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 .

TheRec’s picture

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).

eMPee584’s picture

Title:Language switcher block highlights ALL languages as active» Language switcher block highlights ALL languages as active - solved!
Status:Needs work» Needs review
StatusFileSize
new789 bytes
new877 bytes
Failed: Failed to install HEAD.
[ View ]

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...

nedjo’s picture

The solution looks correct.

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

<?php
// 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?

eMPee584’s picture

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

Damien Tournoud’s picture

Status:Needs review» Needs work
StatusFileSize
new3.09 KB
Failed: 7293 passes, 2 fails, 0 exceptions
[ View ]

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.

eMPee584’s picture

Status:Needs review» Needs work
StatusFileSize
new1.65 KB
new1.73 KB
Failed: Failed to install HEAD.
[ View ]

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

TheRec’s picture

Status:Needs work» Needs review

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 :)

Damien Tournoud’s picture

Title:Language switcher block highlights ALL languages as active - solved!» Language switcher block highlights ALL languages as active
Status:Needs work» Needs review
StatusFileSize
new5.05 KB
Passed: 8999 passes, 0 fails, 0 exceptions
[ View ]

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

veriKami’s picture

subscribing :-)

eMPee584’s picture

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.

nedjo’s picture

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

eMPee584’s picture

Status:Needs review» Reviewed & tested by the community

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

nedjo’s picture

Status:Reviewed & tested by the community» Needs review

@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.

eMPee584’s picture

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...

veriKami’s picture

@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...

eMPee584’s picture

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

TheRec’s picture

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

veriKami’s picture

@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 !!!

PixelClever’s picture

Status:Needs review» Reviewed & tested by the community
-Anti-’s picture

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

Thanks.

kalesco’s picture

Version:7.x-dev» 6.9
Component:usability» language system

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

veriKami’s picture

Version:7.x-dev» 6.9

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

Damien Tournoud’s picture

Version:6.9» 7.x-dev

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

StevenPatz’s picture

Version:6.9» 7.x-dev

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

catch’s picture

Issue tags:+needs backport to D6
nedjo’s picture

Issue tags:+i18n sprint
webchick’s picture

Version:7.x-dev» 6.x-dev

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.

veriKami’s picture

:-))) 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.

petrescs’s picture

subscribing

blackdog’s picture

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.

mr.baileys’s picture

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

attiks’s picture

@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;
    }
  }
}
Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Fixed

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

remi’s picture

@#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.

-Anti-’s picture

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.

eMPee584’s picture

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.

ungeek’s picture

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.

Status:Fixed» Closed (fixed)

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

momper’s picture

hello

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

thanks momper

raj4raj’s picture

Assigned:Unassigned» raj4raj
Status:Closed (fixed)» Needs review

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,

eMPee584’s picture

Assigned:raj4raj» Unassigned
Status:Needs review» Closed (fixed)

@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.

ain’s picture

Status:Closed (fixed)» Needs work

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>
Everett Zufelt’s picture

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>
klonos’s picture

klonos’s picture

Issue tags:-needs backport to D6

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