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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

Issue tags: +Accessibility

Mostly tagging, but also adding WCAG reference - http://www.w3.org/TR/WCAG20/#meaning-other-lang-id

good_man’s picture

Currently, Language Switcher supports this when enabled. Will be happy to see it in core for sure.

mgifford’s picture

Issue tags: +i18n, +wcag2

Thanks @good_man - realized it needs tagging for language.

good_man’s picture

Status: Active » Needs review
FileSize
645 bytes

Quick patch as a proof of concept.

good_man’s picture

Issue tags: +D8MI

Tagging.

Status: Needs review » Needs work

The last submitted patch, 1164682_4.patch, failed testing.

good_man’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

Fixed the test case for the hardcoded classes.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

mgifford’s picture

Status: Needs work » Needs review

#7: 1164682_7.patch queued for re-testing.

clemens.tolboom’s picture

I added the suggested 'language-type-'.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Very simple, looks good, improves styleability of links.

Gábor Hojtsy’s picture

Issue tags: +language-base

Tagging for base language feature set.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/language.incundefined
@@ -199,6 +199,10 @@ function language_negotiation_get_switch_links($type, $path) {
+      foreach ($result as $langcode => $link) {
+        $result[$langcode]['attributes']['class'][] = 'language-link-' . $langcode;

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

mgifford’s picture

Thanks 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 also xml: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.

diff --git a/core/includes/language.inc b/core/includes/language.inc
index 4628053..6263f09 100644
--- a/core/includes/language.inc
+++ b/core/includes/language.inc
@@ -199,6 +199,14 @@ function language_negotiation_get_switch_links($type, $path) {
       $callback = $provider['callbacks']['switcher'];
       $result = $callback($type, $path);
 
+      // Add language specific CSS class as well as support for WCAG 2.0's 
+      // Language of Parts to add language identifiers
+      // http://www.w3.org/TR/UNDERSTANDING-WCAG20/meaning-other-lang-id.html
+      foreach ($result as $langcode => $link) {
+        $result[$langcode]['attributes']['class'][] = 'language-link-' . $langcode;
+        $result[$langcode]['attributes']['lang'] = $langcode;
+      }
+
       if (!empty($result)) {
         // Allow modules to provide translations for specific links.
         drupal_alter('language_switch_links', $result, $type, $path);

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>

Gábor Hojtsy’s picture

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

mgifford’s picture

Status: Needs work » Needs review
FileSize
2.2 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/locale/locale.testundefined
@@ -2205,7 +2205,7 @@ class LocaleUILanguageNegotiationTest extends DrupalWebTestCase {
-    $fields = $this->xpath('//div[@id="block-locale-language"]//a[@class="language-link active" and @href=:url]', $args);
+    $fields = $this->xpath('//div[@id="block-locale-language"]//a[@class="language-link language-link-it active" and @href=:url]', $args);

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

Anonymous’s picture

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

mgifford’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

Thanks @Gabor. I added the tag as you suggested.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

mgifford’s picture

Ok, so adding another header element like:
<link rel="alternate" hreflang="it" href="http://example.com/en" />

So adding something like this almost works:

      if (!empty($result)) {
        // Allow modules to provide translations for specific links.
        drupal_alter('language_switch_links', $result, $type, $path);
        $links = (object) array('links' => $result, 'provider' => $id);
        drupal_add_html_head_link(array(
          'rel' => 'alternate',
          'hreflang' => $langcode,
          'href' => $langcode  . '/' . $result[$langcode]['href']
        ));
        break;
      }

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.

Gábor Hojtsy’s picture

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

mgifford’s picture

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

Gábor Hojtsy’s picture

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

mgifford’s picture

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

Gábor Hojtsy’s picture

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

mgifford’s picture

Absolutely.. It's just a question of best practices for core with the menus..

My main remaining concern here with this issue though is:

What is the best way not to display the in the existing language within language_negotiation_get_switch_links()?

EDIT: As we need that for the
in the HEADER.

mvc’s picture

Assigned: Unassigned » mvc
Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -i18n, -Accessibility, -D8MI, -wcag2, -language-base

#19: language-link-1164682-18.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +i18n, +Accessibility, +D8MI, +wcag2, +language-base

The last submitted patch, language-link-1164682-18.patch, failed testing.

Bojhan’s picture

Title: Switch Language Links Need Language Identifier » Switch language links need language identifier
Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work

Cross-post.

mvc’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

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

mvc’s picture

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.

I just checked; the links area link attributes already do have language attributes.

Status: Needs review » Needs work

The last submitted patch, language-link-1164682-33.patch, failed testing.

mvc’s picture

Status: Needs work » Needs review
FileSize
3.06 KB

trying again, with better error checking

mvc’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/includes/language.incundefined
@@ -313,9 +314,28 @@ function language_negotiation_get_switch_links($type, $path) {
+      // Add language specific CSS class as well as support for WCAG 2.0's
+      // Language of Parts to add language identifiers

Add dot at end of line.

+++ b/core/includes/language.incundefined
@@ -313,9 +314,28 @@ function language_negotiation_get_switch_links($type, $path) {
+        $result[$langcode]['attributes']['class'][] = 'language-link-' . $langcode;
+        $result[$langcode]['attributes']['lang'] = $langcode;

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.

+++ b/core/includes/language.incundefined
@@ -313,9 +314,28 @@ function language_negotiation_get_switch_links($type, $path) {
+        // Add links to alternative versions of this content in other
+        // languages to the header to improve SEO
+        foreach($result as $langcode => $link) {
+          if (isset($link['href']) && $langcode != $language->langcode) {
+            drupal_add_html_head_link(array(
+              'rel' => 'alternate',
+              'hreflang' => $langcode,
+              'href' => $langcode  . '/' . $link['href'],
+            ));
+          }
+        }

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

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

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

Status: Needs review » Needs work

The last submitted patch, language-link-1164682-39.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +sprint

Adding on the sprint. I can get people to work on this later tomorrow :)

mgifford’s picture

That would be excellent, thanks!

sxnc’s picture

Status: Needs work » Needs review
FileSize
1.26 KB

Added a check to see if theres more than one language, if not the type initialization won't be called.

Gábor Hojtsy’s picture

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

Hm, 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 :/

BarisW’s picture

Gabor, just wondering:: why don't we wrap $language->name in the t() function (in language_switcher_url()) so we can still have localized strings in each language and add the langcodes as well?

Gábor Hojtsy’s picture

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

mgifford’s picture

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

mvc’s picture

Assigned: mvc » Unassigned

Status: Needs review » Needs work

The last submitted patch, language-link-1164682-47.patch, failed testing.

BrockBoland’s picture

Needs issue summary

BrockBoland’s picture

Issue summary: View changes

Simple edits. Added supplemental question. Hope this helps.

jvns’s picture

Issue summary: View changes

Added issue summary

jvns’s picture

Added issue summary

lazysoundsystem’s picture

Assigned: Unassigned » lazysoundsystem
Status: Needs work » Needs review
FileSize
1.22 KB

The patch in #47 was using language_types_initialize() which was renamed for D8.

In D7 it was language_initialize()

lazysoundsystem’s picture

Assigned: lazysoundsystem » Unassigned
attiks’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good.

webchick’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Closed (works as designed)
Issue tags: +revisit before beta

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

Gábor Hojtsy’s picture

Sounds right, yes.

mgifford’s picture

That'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?

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Not on sprint anymore.

David_Rothstein’s picture

Issue tags: +7.17 release notes

This is already mentioned in CHANGELOG.txt, so adding to the 7.17 release notes as well.

thedavidmeister’s picture

Title: Switch language links need language identifier » links with a known language need language identifier
Status: Closed (works as designed) » Active

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

thedavidmeister’s picture

I've been told that hreflang is what we should be going for.

mgifford’s picture

Issue tags: +language of parts

Yes, 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?

thedavidmeister’s picture

Issue tags: +Needs tests

Yeah, lets start with tests.

thedavidmeister’s picture

Status: Active » Needs review
Issue tags: -Needs tests
FileSize
4.63 KB

I was thinking something like this?

Status: Needs review » Needs work

The last submitted patch, 1164682-65.patch, failed testing.

thedavidmeister’s picture

Status: Needs work » Needs review

really?

thedavidmeister’s picture

#65: 1164682-65.patch queued for re-testing.

thedavidmeister’s picture

Issue tags: +Needs tests
FileSize
737 bytes

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

thedavidmeister’s picture

didn't mean to kill all these tags :/

thedavidmeister’s picture

Hopefully this is all that needed to be done for #65.

mgifford’s picture

I 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

    case 'node/%/edit':
      $node = node_load($arg[1]);
      $type = node_type_load($node->bundle());
      return (!empty($type->help) ? '<p>' . filter_xss_admin($type->help) . '</p>' : '');

I got that error when trying to test this patch so am listing it here. I will probably need to move it elsewhere later.

thedavidmeister’s picture

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

Pancho’s picture

#71: 1164682-71.patch queued for re-testing.

mgifford’s picture

I 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

Anonymous’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -7.17 release notes +Performance, +D8 cacheability

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: +D8MI

Yay, this is amazing! Let me add some D8MI tags in retrospect :)

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

Anonymous’s picture

Issue summary: View changes

Remove spurious parenthesis

Gábor Hojtsy’s picture

Issue tags: +language-base