Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#64 | language_220559_6.9_switch_without_test.patch | 1.68 KB | remi |
#38 | 220559-language-switcher-with-test.patch | 5.05 KB | Damien Tournoud |
#36 | drupal-active-language-fix-d7.patch | 1.73 KB | eMPee584 |
#36 | drupal-active-language-fix-d6.patch | 1.65 KB | eMPee584 |
#35 | 220559-language-switcher-test.patch | 3.09 KB | Damien Tournoud |
Comments
Comment #1
ximo CreditAttribution: ximo commentedI 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 inl()
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...
locale_block()
(or usinglocale_translation_link_alter()
):theme_links()
andl()
language prefix awareAlternative 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..
Comment #2
drewish CreditAttribution: drewish commentedI 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'.
Comment #3
Gábor HojtsyJust subscribing.
Comment #4
drewish CreditAttribution: drewish commentedI 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.
Comment #5
drewish CreditAttribution: drewish commentedmarked #254251: Language switcher - all links are active as a duplicate
Comment #6
eMPee584 CreditAttribution: eMPee584 commentedhad the same issue; here's the fix (rfc).
Comment #7
drewish CreditAttribution: drewish commentedhaven't gotten to test this but it'd need to go into HEAD first and then be backported to 6.
Comment #8
eMPee584 CreditAttribution: eMPee584 commentedwell this applies to 6.x aswell as 7.x, same code. And this definitly fixes the problem, no side effects for now.
Comment #9
desbeers CreditAttribution: desbeers commentedAttached 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?
Comment #10
eMPee584 CreditAttribution: eMPee584 commentedDesbeers 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.
Comment #11
abandoned CreditAttribution: abandoned commented:-) subscribing
Comment #12
eMPee584 CreditAttribution: eMPee584 commentedximo, 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 ?
Comment #13
eMPee584 CreditAttribution: eMPee584 commentedRecategorizing 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...
Comment #14
drewish CreditAttribution: drewish commentedit's not helpful putting it back in 6 since patches only get committed there after they've landed in HEAD.
Comment #15
eMPee584 CreditAttribution: eMPee584 commentedwell ok sorry i just felt maybe this is being ignored because the patch queue for D7 is that giant...
Comment #16
eMPee584 CreditAttribution: eMPee584 commentedComment #17
eMPee584 CreditAttribution: eMPee584 commentedis 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...
..?
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commented@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, whileurl()
is *not* a cheap operation.Comment #19
PixelClever CreditAttribution: PixelClever commentedsubscribing...
Comment #20
eMPee584 CreditAttribution: eMPee584 commented@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.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #22
eMPee584 CreditAttribution: eMPee584 commentedwell 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?
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedhttp://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;}
Comment #24
PixelClever CreditAttribution: PixelClever commentedHow large a bounty would it take to get a fix for this issue committed in a release?
Comment #25
mandclu CreditAttribution: mandclu commentedI 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.
Comment #26
TheRec CreditAttribution: TheRec commentedWhen 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.Comment #27
-Anti- CreditAttribution: -Anti- commented> 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.
Comment #28
-Anti- CreditAttribution: -Anti- commentedSorry.. double post.
Comment #29
TheRec CreditAttribution: TheRec commentedBy 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) ?
Comment #30
PixelClever CreditAttribution: PixelClever commentedTo 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 .
Comment #31
TheRec CreditAttribution: TheRec commentedWhile 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 :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).Comment #32
eMPee584 CreditAttribution: eMPee584 commentedInvestigating 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...
Comment #33
nedjoThe solution looks correct.
An additional comment before the main change in
l()
would be useful. Suggestion: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?
Comment #34
eMPee584 CreditAttribution: eMPee584 commentedwell in that case one would better make sure that the link's language attribute is not set..
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere 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.Comment #36
eMPee584 CreditAttribution: eMPee584 commentedwell here's the patches including the change to theme_links aswell. And thanks Damien for that test!
Comment #37
TheRec CreditAttribution: TheRec commentedI 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 :)
Comment #38
Damien Tournoud CreditAttribution: Damien Tournoud commentedIncluded the test from #35 into the patch from #36.
Comment #39
veriKami CreditAttribution: veriKami commentedsubscribing :-)
Comment #40
eMPee584 CreditAttribution: eMPee584 commentedWell it's a tight race between this fix being merged into D6 and hell freezing over.. i think hell is gonna win this one.
Comment #41
nedjo@eMPee584: Well, this issue is still marked "code needs review" so won't necessarily have come onto the radar of the core committers.
Comment #42
eMPee584 CreditAttribution: eMPee584 commentedWell if there's no policy against that, let me do it.. done. Hell's still ahead by lengths though.
Comment #43
nedjo@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.
Comment #44
eMPee584 CreditAttribution: eMPee584 commentedWell 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...
Comment #45
veriKami CreditAttribution: veriKami commented@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...
Comment #46
eMPee584 CreditAttribution: eMPee584 commented@verKami: because not enough people are testing the posted patch and shouting 'yes it works' (which indeed it does)!
Comment #47
TheRec CreditAttribution: TheRec commented'yes it works' ! (patch of #38) :)
Thanks
Comment #48
veriKami CreditAttribution: veriKami commented@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 !!!
Comment #49
PixelClever CreditAttribution: PixelClever commentedComment #50
-Anti- CreditAttribution: -Anti- commentedI take it this patch didn't make it into D6.8?
Will it make it into D6.9 (assuming there is one)?
Thanks.
Comment #51
kalesco CreditAttribution: kalesco commentedwaiting for patch to be included in next release!
(commented for subscription)
Comment #52
veriKami CreditAttribution: veriKami commented...probably in D.6.999... :-(
Comment #53
Damien Tournoud CreditAttribution: Damien Tournoud commentedPlease do not change version unless you have a good reason to.
Comment #54
StevenPatzOnce it makes it into 7.x and then is back ported it MAY be included in the 6.x series.
Comment #55
catchComment #56
nedjoComment #57
webchickCommitted 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.
Comment #58
veriKami CreditAttribution: veriKami commented:-))) 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.
Comment #59
petrescs CreditAttribution: petrescs commentedsubscribing
Comment #60
blackdog CreditAttribution: blackdog commentedI 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.
Comment #61
mr.baileysSubscribing... I'm currently running a Drupal 6.9 multilanguage site with the changes from #36 applied, works as advertised.
Comment #62
attiks CreditAttribution: attiks commented@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.
Comment #63
Gábor HojtsyCommitted #36 to Drupal 6 as well. Thanks for all the testing!
Comment #64
remi CreditAttribution: remi commented@#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.
Comment #65
-Anti- CreditAttribution: -Anti- commentedThanks 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.
Comment #66
eMPee584 CreditAttribution: eMPee584 commentedwell 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
respectivelyli a.language-link.active
for active ones. Note the relatively uncommon chaining of class selector - that's all the magic there is to it.Comment #67
dimmie CreditAttribution: dimmie commentedNot 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.
Comment #69
momper CreditAttribution: momper commentedhello
is it included in the last drupal 6.11? or do i have to patch something?
thanks momper
Comment #70
raj4raj CreditAttribution: raj4raj commentedAdding 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,
Comment #71
eMPee584 CreditAttribution: eMPee584 commented@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.
Comment #72
ain CreditAttribution: ain commentedReopening the issue since the very same problem is there in the most recent dev release of 2010-Mar-05.
Here's the output:
Comment #73
Everett Zufelt CreditAttribution: Everett Zufelt commentedI 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.
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.
Comment #74
klonos...related perhaps?: #1226178: Language switcher block doesn't render any language link as .active when in node edit form...
Comment #75
klonos...honestly don't know how that tag got added there.