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?

Files: 
CommentFileSizeAuthor
#71 1164682-65-71-interdiff.txt1.7 KBthedavidmeister
#71 1164682-71.patch4.89 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,405 pass(es).
[ View ]
#69 1164682-69.patch737 bytesthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,243 pass(es).
[ View ]
#65 1164682-65.patch4.63 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 56,865 pass(es), 132 fail(s), and 44 exception(s).
[ View ]
#52 language-link-1164682-52.patch1.22 KBlazysoundsystem
PASSED: [[SimpleTest]]: [MySQL] 39,361 pass(es).
[ View ]
#47 language-link-1164682-47.patch1.22 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] 39,242 pass(es), 262 fail(s), and 364 exception(s).
[ View ]
#43 language-link-1164682-43.patch1.26 KBsxnc
PASSED: [[SimpleTest]]: [MySQL] 40,247 pass(es).
[ View ]
#39 language-link-1164682-39.patch1.19 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] 40,108 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#36 language-link-1164682-36.patch3.06 KBmvc
PASSED: [[SimpleTest]]: [MySQL] 37,004 pass(es).
[ View ]
#33 language-link-1164682-33.patch3.04 KBmvc
FAILED: [[SimpleTest]]: [MySQL] 37,022 pass(es), 0 fail(s), and 42 exception(s).
[ View ]
#19 language-link-1164682-18.patch2.21 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language-link-1164682-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 language-link-1164682-16.patch2.2 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 34,198 pass(es).
[ View ]
#10 language-link-1164682-10.patch1.93 KBclemens.tolboom
PASSED: [[SimpleTest]]: [MySQL] 34,074 pass(es).
[ View ]
#7 1164682_7.patch1.9 KBgood_man
PASSED: [[SimpleTest]]: [MySQL] 33,352 pass(es).
[ View ]
#4 1164682_4.patch645 bytesgood_man
FAILED: [[SimpleTest]]: [MySQL] 34,123 pass(es), 1 fail(s), and 1 exception(es).
[ View ]

Comments

Issue tags:+accessibility

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

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

Issue tags:+i18n, +wcag2

Thanks @good_man - realized it needs tagging for language.

Status:Active» Needs review
StatusFileSize
new645 bytes
FAILED: [[SimpleTest]]: [MySQL] 34,123 pass(es), 1 fail(s), and 1 exception(es).
[ View ]

Quick patch as a proof of concept.

Issue tags:+D8MI

Tagging.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
PASSED: [[SimpleTest]]: [MySQL] 33,352 pass(es).
[ View ]

Fixed the test case for the hardcoded classes.

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.

Status:Needs work» Needs review

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

StatusFileSize
new1.93 KB
PASSED: [[SimpleTest]]: [MySQL] 34,074 pass(es).
[ View ]

I added the suggested 'language-type-'.

Status:Needs review» Reviewed & tested by the community

Very simple, looks good, improves styleability of links.

Issue tags:+language-base

Tagging for base language feature set.

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?

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>

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

Status:Needs work» Needs review
StatusFileSize
new2.2 KB
PASSED: [[SimpleTest]]: [MySQL] 34,198 pass(es).
[ View ]

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new2.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch language-link-1164682-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

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.

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

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.

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

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.

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.

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.

Assigned:Unassigned» mvc

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.

Title:Switch Language Links Need Language IdentifierSwitch language links need language identifier
Status:Needs work» Needs review

Status:Needs review» Needs work

Cross-post.

Status:Needs work» Needs review
StatusFileSize
new3.04 KB
FAILED: [[SimpleTest]]: [MySQL] 37,022 pass(es), 0 fail(s), and 42 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.06 KB
PASSED: [[SimpleTest]]: [MySQL] 37,004 pass(es).
[ View ]

trying again, with better error checking

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.

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

Status:Needs work» Needs review
StatusFileSize
new1.19 KB
FAILED: [[SimpleTest]]: [MySQL] 40,108 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

Issue tags:+sprint

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

That would be excellent, thanks!

Status:Needs work» Needs review
StatusFileSize
new1.26 KB
PASSED: [[SimpleTest]]: [MySQL] 40,247 pass(es).
[ View ]

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

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

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?

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

StatusFileSize
new1.22 KB
FAILED: [[SimpleTest]]: [MySQL] 39,242 pass(es), 262 fail(s), and 364 exception(s).
[ View ]

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.

Assigned:mvc» Unassigned

Status:Needs review» Needs work

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

Needs issue summary

Issue summary:View changes

Simple edits. Added supplemental question. Hope this helps.

Issue summary:View changes

Added issue summary

Added issue summary

Assigned:Unassigned» lazysoundsystem
Status:Needs work» Needs review
StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 39,361 pass(es).
[ View ]

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

In D7 it was language_initialize()

Assigned:lazysoundsystem» Unassigned

Status:Needs review» Reviewed & tested by the community

Patch looks good.

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?

Sounds right, yes.

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?

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

Issue tags:-sprint

Not on sprint anymore.

Issue tags:+7.17 release notes

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

Title:Switch language links need language identifierlinks 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.

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

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?

Issue tags:+Needs tests

Yeah, lets start with tests.

Status:Active» Needs review
Issue tags:-Needs tests
StatusFileSize
new4.63 KB
FAILED: [[SimpleTest]]: [MySQL] 56,865 pass(es), 132 fail(s), and 44 exception(s).
[ View ]

I was thinking something like this?

Status:Needs review» Needs work

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

Status:Needs work» Needs review

really?

Issue tags:+Needs tests
StatusFileSize
new737 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,243 pass(es).
[ View ]

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.

didn't mean to kill all these tags :/

Issue tags:-i18n, -accessibility, -revisit before beta, -D8MI, -wcag2, -language of parts, -language-base
StatusFileSize
new4.89 KB
PASSED: [[SimpleTest]]: [MySQL] 57,405 pass(es).
[ View ]
new1.7 KB

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

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.

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.

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

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

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.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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.

Issue summary:View changes

Remove spurious parenthesis