Problem/Motivation
When you set up the Language Switcher block in D7, screen readers don't know what language to use when reading the link's text (i.e. 'English', 'Français') because a language attribute is NOT supplied to the reader. Parent issue: http://groups.drupal.org/node/145894
Gábor Hojtsy notes in #44 that native language names have been removed in D8, so as of August 25, 2012 this is not longer an issue in D8. This issue should be revisited before release of D8 however.
Proposed resolution
Add a 'lang' attribute to the language links. For example
<li class="ar"><a href="/ar" class="language-link" lang="ar" xml:lang="ar"
>العربية</a></li>
<li class="fr last"><a href="/fr" class="language-link" lang="fr" xml:lang="fr"
>Français</a></li>
Remaining tasks
A patch has been written for D7 (#47), but it is currently not passing the tests.
User interface changes
Screen reader users hear links in language switcher read in correct language.
API changes
None.
Original report by mgifford
When you set up the Language Switcher block in D7 or D8, screen readers don't know what language to use when reading the link's text (i.e. 'English', 'Français') because a language attribute is NOT supplied to the reader. Parent issue: http://groups.drupal.org/node/145894
The format should be:
<ul class="language-switcher-locale-url"><li class="en first active"><a href="/en" class="language-link active" lang="en" xml:lang="en"
>English</a></li>
<li class="ar"><a href="/ar" class="language-link" lang="ar" xml:lang="ar"
>العربية</a></li>
<li class="fr last"><a href="/fr" class="language-link" lang="fr" xml:lang="fr"
>Français</a></li>
</ul>
And I think this is controlled by language_negotiation_get_switch_links() but that's about as far as I got.
Also, what would be a 'best practice' for NOT displaying the current language link?
Comment | File | Size | Author |
---|---|---|---|
#71 | 1164682-65-71-interdiff.txt | 1.7 KB | thedavidmeister |
#71 | 1164682-71.patch | 4.89 KB | thedavidmeister |
#69 | 1164682-69.patch | 737 bytes | thedavidmeister |
#65 | 1164682-65.patch | 4.63 KB | thedavidmeister |
#52 | language-link-1164682-52.patch | 1.22 KB | lazysoundsystem |
Comments
Comment #1
mgiffordMostly tagging, but also adding WCAG reference - http://www.w3.org/TR/WCAG20/#meaning-other-lang-id
Comment #2
good_man CreditAttribution: good_man commentedCurrently, Language Switcher supports this when enabled. Will be happy to see it in core for sure.
Comment #3
mgiffordThanks @good_man - realized it needs tagging for language.
Comment #4
good_man CreditAttribution: good_man commentedQuick patch as a proof of concept.
Comment #5
good_man CreditAttribution: good_man commentedTagging.
Comment #7
good_man CreditAttribution: good_man commentedFixed the test case for the hardcoded classes.
Comment #8
Gábor HojtsyCan we make the class more descriptive, eg. 'language-link-it' for Italian? That would be in line with how we name classes progressively elsewhere. These very short class names with the language name only can easily become misleading IMHO. Otherwise looks good and updates the tests proper.
Comment #9
mgifford#7: 1164682_7.patch queued for re-testing.
Comment #10
clemens.tolboomI added the suggested 'language-type-'.
Comment #11
Gábor HojtsyVery simple, looks good, improves styleability of links.
Comment #12
Gábor HojtsyTagging for base language feature set.
Comment #13
Dries CreditAttribution: Dries commentedCould we add a code comment here? I'd think we need to add
lang="it" xml:lang="it"
instead? In other words, I'm a bit confused by this patch and I'm guessing others might too.I recommend that we reference the WCAG specification. I read up on http://www.w3.org/TR/WCAG20/#meaning-other-lang-id (provided in #1) but it wasn't clear how that relates to this patch. Where is the "language-link-$langcode" format being defined in WCAG?
Comment #14
mgiffordThanks Dries!
In Garland D8 pre-patch:
<ul class="language-switcher-locale-url"><li class="en odd first"><a href="/" class="language-link">English</a></li><li class="fr even active"><a href="/fr" class="language-link active">Français</a></li><li class="ar odd last"><a href="/ar" class="language-link">العربية</a></li></ul> </div>
With the patch I get:
<ul class="language-switcher-locale-url"><li class="en odd first active"><a href="/" class="language-link language-link-en active">English</a></li><li class="fr even"><a href="/fr" class="language-link language-link-fr">Français</a></li><li class="ar odd last"><a href="/ar" class="language-link language-link-ar">العربية</a></li></ul> </div>
But the crux is
lang="fr"
and I think alsoxml:lang="fr"
I'm not certain that we need to add the xml too, but I don't know of an example for HTML5 from the W3C.http://www.w3.org/TR/UNDERSTANDING-WCAG20/meaning-other-lang-id.html
http://www.w3.org/TR/2012/NOTE-WCAG20-TECHS-20120103/H58
I'm still not any good at writing the simple tests, but I'll just post the code here (with some sample instructions):
@Dries, I put in a different link to WCAG, but both are fine.
Which results in something that is more easily styled and also includes the accessibility changes that are required:
<ul class="language-switcher-locale-url"><li class="en odd first active"><a href="/" class="language-link language-link-en active" lang="en">English</a></li><li class="fr even"><a href="/fr" class="language-link language-link-fr" lang="fr">Français</a></li><li class="ar odd last"><a href="/ar" class="language-link language-link-ar" lang="ar">العربية</a></li></ul> </div>
Comment #15
Gábor Hojtsy@mgifford, @Dries: that indeed looks like a good improvement. We don't need xml:lang in HTML5, lang is enough. @mgifford: can you put this into a patch form?
Comment #16
mgifford@Gabor - Yes, I should have done that.. It does require an additional unit test though before we can mark it RTBC again.
EDIT: Well, I think it does.. It's all wrapped in the same call so perhaps not.
Comment #17
Gábor HojtsyI'm not sure we need an additional test, we can just expand the test we have. We should just be able to add 'and @lang="it"' before 'and @href'... (minus the single quotes).
It is just a different attribute on the same tag.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedTo also improve SEO the switcher language links should has the rel="alternate" and hreflang="destination language" attributes, to help search engines to identify content translation.
See 'Using rel="alternate" hreflang="x"' section:
http://support.google.com/webmasters/bin/answer.py?hl=en&hlrm=es&answer=...
Comment #19
mgiffordThanks @Gabor. I added the tag as you suggested.
Comment #20
Gábor HojtsyRead through the doc suggested by @jmesam and that seems like very logical to introduce here too. Looks like we are getting to a much more complete markup set here. Adding those would be just extending the foreach and the test like we did above. Otherwise the patch looks good.
Comment #21
mgiffordOk, so adding another header element like:
<link rel="alternate" hreflang="it" href="http://example.com/en" />
So adding something like this almost works:
However, it just results in a single instance of the link rather than 3 (which is what I would have expected given my test site):
<link rel="alternate" hreflang="ar" href="ar/node/5" />
I'm not sure how within language_negotiation_get_switch_links() to get the current language - i18n_get_lang() isn't available.
We don't want to send an alternate of the language that it is currently defined as. Actually, if we get this fixed for the header, it might be good to modify this behavior for D8. Who needs to see a link to the page that they are currently on? Switch links shouldn't include the current language.
Comment #22
Gábor HojtsyIndeed, http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/d... will not allow you to add more links with the same rel value. Not sure that we should debug that here, unless you want to take that on, hm. On the links, I think its best to display the current language, in a menu you don't hide the current menu item either :)
Comment #23
mgiffordYa, that seems like an issue I'd like to put elsewhere, so #1435936: drupal_add_html_head_link() will not allow you to add more links with the same rel value
What is the best way not to display the
in the existing language within language_negotiation_get_switch_links()?
In Canada the standard is to simply have a single link to the page in an alternate language. It would be nice if it were configurable as certainly for any government site it's a matter of writing custom code so that only the alternate link is displayed.
Comment #24
Gábor Hojtsy@mgifford: it is certainly very easy to hide with CSS targeting the current language link in the block, so one can argue it is already configurable.
Comment #25
mgiffordYa, but hiding with CSS is a bad hack, especially when you're looking at minimizing page sizes for mobile devices. It's much better to simply not spit out html that you know isn't going to be used.
Comment #26
Gábor HojtsySure, themes can do preprocess for the block too and/or a module to implement hook_alter_language_switch_links(), there are plenty of possibilities even in D7.
Comment #27
mgiffordAbsolutely.. It's just a question of best practices for core with the menus..
My main remaining concern here with this issue though is:
EDIT: As we need that for the
in the HEADER.
Comment #28
mvcComment #29
Gábor Hojtsy#19: language-link-1164682-18.patch queued for re-testing.
Comment #31
Bojhan CreditAttribution: Bojhan commentedComment #32
Gábor HojtsyCross-post.
Comment #33
mvcHere's a patch which adds rel=alternate language links to the header, and language attributes to the language switcher links.
The links area language switching also needs language attributes but I'll stop here for now instead of increasing the scope of this patch further.
Notwithstanding #1435936: drupal_add_html_head_link() will not allow you to add more links with the same rel value this works for me on a test site with three languages enabled. I get header links to the other two languages.
PS: that was very tricky, hiding the test scripts in a completely different directory! But I figured it out despite you folks.
Comment #34
mvcI just checked; the links area link attributes already do have language attributes.
Comment #36
mvctrying again, with better error checking
Comment #37
mvcyay, it passed!
oh, and i forgot to weigh in on the usability debate going on here. i'm with gábor on this one. let's leave the link to the current language in place in the language switcher. @mgifford, people needing to follow government of canada style guides can override that in the theme layer one way or another.
Comment #38
Gábor HojtsyAdd dot at end of line.
Our browser requirements might have changed in the meantime to require browsers that could allow for @lang type of selectors, so if the link has language-link class and @lang equals something, that equals having the class on it. Is our policy now not to include extra classes when the same thing can be achieved width combined selectors, or are we including specific selectors? Sounds like a question for some markup warrior in core.
The links are definitely not good, whether the alternate language version is accessible under the URL you generate here depends on configuration. These links only work in a specific configuration where the path prefix is used (versus domains or session, or some other method) and even the default language has a langcode.
Language specific links should be generated with url().
Since this looks like a non-trivial task, I think it would also be useful to add test coverage for it. (If you still want to go with expanding the goals for this issue with alternate version links in the header).
Comment #39
mgiffordLet's get this through simply & then extend it later.
The basics here involve mostly adding the language code
$result[$langcode]['attributes']['lang'] = $langcode;
Why include an extra class if it isn't needed?
I love the idea of adding drupal_add_html_head_link()'s for alternate languages, but that can be done in a subsequent patch or new issue.
Comment #41
Gábor HojtsyAdding on the sprint. I can get people to work on this later tomorrow :)
Comment #42
mgiffordThat would be excellent, thanks!
Comment #43
sxnc CreditAttribution: sxnc commentedAdded a check to see if theres more than one language, if not the type initialization won't be called.
Comment #44
Gábor HojtsyHm, I think the irony of this patch is that we removed native language names (compare http://api.drupal.org/api/drupal/core%21modules%21language%21language.ne... to http://api.drupal.org/api/drupal/includes%21locale.inc/function/locale_l...). So in Drupal 8, the language name printed is really as configured and therefore the lang property would not apply. We want/hope to re-introduce translated language names later with CMI, but we don't have that currently. So as it is, unfortunately this only applies to Drupal 7. I think we might want to fix that in D7 now and then tag this with something to ensure we "revisit before release" (have a tag name for that) on D8.
So this sounds like should be rolled against D7 now :/
Comment #45
BarisW CreditAttribution: BarisW commentedGabor, just wondering:: why don't we wrap
$language->name
in thet()
function (inlanguage_switcher_url()
) so we can still have localized strings in each language and add the langcodes as well?Comment #46
Gábor Hojtsy@BarisW: language names are user input, even if kinds of user input that very rarely change. So this kind of data maps to the CMI translation work, that is being introduced hopefully with Drupal 8.
Comment #47
mgiffordWould be great if we could bring this into D7 core. Here's a re-rolled patch, but it's actually done against 7.15 rather than the latest D7 git repo. Hopefully it still works. It's essentially the same as 43.
Comment #48
mvcComment #50
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #50.0
BrockBoland CreditAttribution: BrockBoland commentedSimple edits. Added supplemental question. Hope this helps.
Comment #50.1
jvns CreditAttribution: jvns commentedAdded issue summary
Comment #51
jvns CreditAttribution: jvns commentedAdded issue summary
Comment #52
lazysoundsystem CreditAttribution: lazysoundsystem commentedThe patch in #47 was using
language_types_initialize()
which was renamed for D8.In D7 it was
language_initialize()
Comment #53
lazysoundsystem CreditAttribution: lazysoundsystem commentedComment #54
attiks CreditAttribution: attiks commentedPatch looks good.
Comment #55
webchickNormally we don't do markup changes, but in this case it's fixing a legit accessibility bug that I can't see a decent workaround for, so I think it's ok.
Committed and pushed to 7.x. Thanks!
Not... totally sure what to do with this now... I'm going to move it to 8.x, mark it "closed (works as designed)" but also "revisit before release." Does that sound right?
Comment #56
Gábor HojtsySounds right, yes.
Comment #57
mgiffordThat's great news @webchick
@Gabor, how do we remember to review before release in D8?
As you said, "We want/hope to re-introduce translated language names later with CMI, but we don't have that currently."
Any ideas to see that this doesn't get lost?
Comment #58
Gábor Hojtsy@mgifford: yes, there is already an issue at #1754246: Languages should be configuration entities to track introduction of languages as configurables. As long as this is tagged "revisit before release", it should be revisited :)
Comment #59
Gábor HojtsyNot on sprint anymore.
Comment #60
David_Rothstein CreditAttribution: David_Rothstein commentedThis is already mentioned in CHANGELOG.txt, so adding to the 7.17 release notes as well.
Comment #61
thedavidmeister CreditAttribution: thedavidmeister commentedHi, I'm rather clueless on many language switching issues.
I'm coming to this from #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.
Wondering if
lang
could be added with a simple hook_link_alter() (as that exists in D8 now) for *all* links that have a language set in their options?I don't know if that's good/bad/already been discussed because I'm coming at it from the angle of parsing the language with javascript for "active-ness" rather than accessibility.
Comment #62
thedavidmeister CreditAttribution: thedavidmeister commentedI've been told that
hreflang
is what we should be going for.Comment #63
mgiffordYes, using
hreflang
seems like the right way to approach this in HTML5.http://www.w3schools.com/tags/att_a_hreflang.asp
If we do this we're going to need to re-do the patch in #52 that got into Core too.
@thedavidmeister can you propose a patch?
Comment #64
thedavidmeister CreditAttribution: thedavidmeister commentedYeah, lets start with tests.
Comment #65
thedavidmeister CreditAttribution: thedavidmeister commentedI was thinking something like this?
Comment #67
thedavidmeister CreditAttribution: thedavidmeister commentedreally?
Comment #68
thedavidmeister CreditAttribution: thedavidmeister commented#65: 1164682-65.patch queued for re-testing.
Comment #69
thedavidmeister CreditAttribution: thedavidmeister commentedthis might work better, I'm not totally across language objects yet so I left out tests in this one to see if I can get it green against HEAD first.
Comment #70
thedavidmeister CreditAttribution: thedavidmeister commenteddidn't mean to kill all these tags :/
Comment #71
thedavidmeister CreditAttribution: thedavidmeister commentedHopefully this is all that needed to be done for #65.
Comment #72
mgiffordI just got this error in simplytest.me/ar/node/1/edit
Fatal error: Call to a member function bundle() on a non-object in /DRUPAL8/core/modules/node/node.module on line 136
But it doesn't show up:
simplytest.me/node/1/edit
I got that error when trying to test this patch so am listing it here. I will probably need to move it elsewhere later.
Comment #73
thedavidmeister CreditAttribution: thedavidmeister commentedI would be pretty surprised if the change I made to add attributes to links in l() in #71 led to that error you're seeing in #72.
I think you should open a separate issue for that and write a test for it so others can see it.
Comment #74
Pancho#71: 1164682-71.patch queued for re-testing.
Comment #75
mgiffordI replicated #72 without the patch, so yes. Not related to this issue.
New issue for that bug posted here #2047589: Fatal error: Call to a member function bundle() on a non-object in node.module on line 136
Comment #76
Anonymous (not verified) CreditAttribution: Anonymous commentedthis is a simple patch, and blocks #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links, which is important for D8 cacheability and performance.
Comment #77
catchCommitted/pushed to 8.x, thanks!
Comment #78
Gábor HojtsyYay, this is amazing! Let me add some D8MI tags in retrospect :)
Comment #79.0
(not verified) CreditAttribution: commentedRemove spurious parenthesis
Comment #80
Gábor Hojtsy