Because you can viewing the content of a node in one language while browsing a site in another we need to add language attributes to page.tpl.php files:
<div id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?> clearfix"<?php print $attributes; ?> lang="<?php print $language; ?>" xml:lang="<?php print $language; ?>">

As identified here, this is a WCAG issue http://groups.drupal.org/node/145894

Comments

mgifford’s picture

I wish I could edit the original post...

<div id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?> clearfix"<?php print $attributes; ?> lang="<?php print $language; ?>" xml:lang="<?php print $language; ?>">

Forgot to wrap that in code.

Jeff Burnz’s picture

Issue tags: +html5

Probably could use $attributes, not sure if we can use xml:lang in HTML5, I can't recall right now, I think not for docs that aren't something like MathML etc. Tagging so we can track it.

mgifford’s picture

Actually, we should be able to deal with just:

<div id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?> clearfix"<?php print $attributes; ?> lang="<?php print $language; ?>">

if we are dealing with HTML5. For xHTML or xHTML5 it would be good practice to include both -- lang="en" xml:lang="en"

For more information, see:
http://www.w3.org/TR/html5/elements.html#the-lang-and-xml:lang-attributes

The xml:lang attribute, for the vast majority of web authors, most likely never will be necessary in #HTML5:
http://twitter.com/#!/dboudreau/status/72288293127077888

jacine’s picture

Component: theme system » language system

We can use $attributes, but this belongs in the language system component (not the theme system).

I don't want to see these attributes added when the language of the node matches the language is the site itself. This only makes sense to me where there are multiple languages present on a site. Otherwise, it's just extra unnecessary markup, as it already inherits this information.

We do not need the xml:lang attribute. The spec says it's to be used in XML documents or in MathML and SVG namespaces. If someone wants to do XHTML5, they can easily add that attribute during preprocess based on the regular lang attribute that would already exist, when this patch happens.

BarisW’s picture

The WCAG specs state that language tags are needed when you are switching languages in content. So if we are viewing an English page on an English website we don't need to add a language tag. However, if we show a list of nodes (eg the ten latest node teasers on the front page), that include nodes from other languages, we need to add a language tag on the nodes that differ.

Example code

<html xml:lang="en">
<body>
<div class="teaser node">
  <h3>Node with the same language</h3>
</div>
<div class="teaser node" xml:lang="nl">
  <h3>Node with the other language</h3>
</div>
<div class="teaser node">
  <h3>Node with the same language</h3>
</div>
</body>
</html>

So no need to include it in every node.

<div id="node-<?php print $node->nid; ?>" class="<?php print $classes; ?> clearfix"<?php print $attributes; ?> <?php if($node->language != $language) print ' lang="' . $node->language . '"'>
mgifford’s picture

As @Jeff notes the hierarchy here is:
page -> node -> field

Node including teasers, but fields could also have different languages specified.

There are also the untranslated strings as well which should be clear what language they are in #1165476: if t() string has no translation or fallback language, text should have lang attribute and actually within fields (or the body) itself content may come in multiple languages #1165466: Language of parts, accessibility and multilanguage: language button.

So this is an interesting but complicated area.

gábor hojtsy’s picture

hanno’s picture

Issue tags: +Needs backport to D7

probably related issue, might also be needed for rtl and ltr handling of nodes?
#780508: Set language by content language for LTR/RTL display

if node/x is in hebrew and node/y is in english and I want to let my users see them both, I have a problem setting the display directionality to change between them.

hanno’s picture

Issue tags: -Needs backport to D7

sorry, changed issue tag

mgifford’s picture

Issue tags: -Needs backport to D7

Related issue - #1260716: Improve language onboarding user experience

All the language drop downs should have their language assigned.

gábor hojtsy’s picture

Status: Active » Needs work
Issue tags: +D8MI

I think the logic in #5 seems to be good, that is only include the lang attribute if the node language is different to the page language. I'm not sure this should go to the template like that, logic in a preprocess for the node template would be much better. Setting to needs work since there is already suggested code. This should possibly be a low hanging fruit I think.

gábor hojtsy’s picture

BTW I think we should also include dir="XXXX", if the direction for the node language is different from the direction of the page language, right?

BarisW’s picture

Gabor, that would be awesome. And you are right, the theme layer is certainly not the best place to do this.

hanno’s picture

Yeah, If we include 'dir' as well, we also tackle #780508: Set language by content language for LTR/RTL display

gábor hojtsy’s picture

I think it would make lots of sense to try and solve both problems at the same time. They require the same kind of logic, and will happen at the same place once the attributes are dynamic like in the main html template.

fastangel’s picture

Assigned: Unassigned » fastangel
StatusFileSize
new952 bytes

Hi,

Attach a patch that I think can be a starting point. Now work with nodes. Should be extend to comments, ... I've tried to raise through entity.

Regards

gábor hojtsy’s picture

Marked #780508: Set language by content language for LTR/RTL display duplicate. Let's solve both lang and dir here.

@fastangel: Thanks for picking this up! Let's use "lang" not xml:lang. We are using lang elsewhere in Drupal 8. Also, please add dir as well. Also lang should not be added at all times / in all cases, only when different from the page language. You seem to add it unconditionally at least in locale_preprocess_node().

fastangel’s picture

Status: Needs work » Needs review
StatusFileSize
new1.14 KB

Hi,

Attached new patch. With dir and corrected lang. It can be proved in http://drupal8-dev.jacintocapote.com/ (2 nodes in spanish, 1 node in english). In spanish show dir and lang. In english not show dir and lang.

NOTE: The language default in my test is english.

Regards

Status: Needs review » Needs work

The last submitted patch, locale.module.diff, failed testing.

gábor hojtsy’s picture

This seems like a bit of an overkill to me. You set an entity content pre-render for when the language is different to locale_entity_pre_render(). Then there you set the #xml_lang property to TRUE, which is then checked in the template preprocess. I understand you approach this from a point of view to generalize this for nodes, comments, etc. As long as those use the same html attribute structure in their templates though, just reusing the preprocess function seems like it would achieve the same effect, no?

So what about putting the logic itself into the preprocess and do away with the entity view and the pre_render? Also, let's do the same difference check for the direction. Only include dir="rtl" if the page was ltr and vice versa. In other words, if its a different language but the same direction, why include the direction? That is inherited proper.

All this should dramatically decrease the things that can go wrong as well :)

Thanks!

fastangel’s picture

Status: Needs work » Needs review
StatusFileSize
new882 bytes

Ok Gábor. Attached a path with all the logic in function preprocess. Fixed dir also when it is different. Sorry to start complicating it with 3 functions. :D

Status: Needs review » Needs work

The last submitted patch, locale.module.diff, failed testing.

gábor hojtsy’s picture

1. Does not seem to apply. Did you roll the patch with an updated D8 checkout?
2. Looks good otherwise, except I think we should rename $language_content to $node_language or something like that. $language_content is the name of a global used to designate the global content language, so this can easily end up in confusion. Let's be more specific here IMHO.

Otherwise it might look strange that this is in locale module (to become language module soon), but unfortunately it does require language_load(), which is provided by the module and is not generally available without locale module. So it cannot just go straight into node module (without conditions). Where this exactly goes might be debated more later when we break out more of node module's language code from locale module (#540294: Move node language settings from Locale to Node module), I think it is as good here now as it can be. So let's fix 1 and 2 :)

fastangel’s picture

Status: Needs work » Needs review
StatusFileSize
new878 bytes

1. Ok.
2. Fixed. I did not know.

I look forward to see how we continue.

Status: Needs review » Needs work

The last submitted patch, locale.module.diff, failed testing.

gábor hojtsy’s picture

Status: Needs work » Needs review
StatusFileSize
new871 bytes

Hm, got this when applying the patch:

$ patch -p1 < locale.module_2.diff 
patching file core/modules/locale/locale.module
patch unexpectedly ends in middle of line
patch: **** malformed patch at line 26:  

Here is a rerolled version, same code (which IMHO looks good).

Status: Needs review » Needs work

The last submitted patch, locale-node-language.patch, failed testing.

gábor hojtsy’s picture

Ok, we are getting Trying to get property of non-object on line 1186, where $node_language is attempted to be used. I think the basic problem here is that we did not yet consider LANGUAGE_NONE as a case, which cannot be language_load()-ed. If $variables['language'] was LANGUAGE_NONE, I think the best we can do is to skip this preprocess altogether.

good_man’s picture

If $variables['language'] was LANGUAGE_NONE, I think the best we can do is to skip this preprocess altogether.

Agree.

fastangel’s picture

StatusFileSize
new942 bytes

Attached new patch. With this patch should pass the test.

hanno’s picture

Or better use lang=""?:

If the resulting value is the empty string, then it must be interpreted as meaning that the language of the node is explicitly unknown.

http://www.w3.org/TR/html5/elements.html#the-lang-and-xml:lang-attributes

gábor hojtsy’s picture

@Hanno: Well, not really, LANGUAGE_NONE is interpreted/displayed on the UI for nodes as "Language neutral" which means "No language applicable", *not* "We don't know the language". I think with this understanding, its best to avoid putting in a lang attribute, right?

BarisW’s picture

@Hanno: not sure if this is the right way. If the node language is unknown, can we not just assume it's the same as the website language? And thus not show a lang tag at all?

@fastangel: patch looks good. A minor improvement would be to define the 'global $language' part only if we need it (so within the IF function).

fastangel’s picture

StatusFileSize
new944 bytes

New patch with global $language in IF.

good_man’s picture

Status: Needs work » Needs review
hanno’s picture

First of all I like this patch!
@gabor @baris About lang="" Not sure what we want to happen if we have nodes in nodes (for example node_embed or views in views). Let's say we have this situation:

<html lang=en>
<node 1 lang=ar>
<node 2 lang=LANGUAGE_NONE>
Hello
</node 2>
</node 1>
<html>

Will node 2 get the arabic or english language and direction, or unknown?

  • no language attribute: it will get the arabic language from node 1
  • lang="", Will screen readers attach the english language to this node (page fallback), or will it stay undefined?
  • lang="und", as this text is undermined
  • or should we add lang="en" even if LANGUAGE_NONE (using site's default language?)
gábor hojtsy’s picture

@Hanno: the concept for the "Language neutral" language for nodes is to be used for nodes which do not have any textual or other type of content that can have a language. Such as if the node is just for storing numbers and does not have a textual title or any other piece that would need language information. Once again, it is not supposed to mean "Unknown language", it means "Independent of language", so whatever language context it is embedded in, it should just work. It does not need and should not have special markup for its language, since it is by definition independent of a language environment.

That is at least the definition / intention of the feature, to allow of opting out of specifying language where it does not make sense. Maybe people do use it for other meanings, in which case it would be a valid feature request to add a different option to specify "Unknown language". Drupal up to Drupal 7 does not have such a feature.

hanno’s picture

@gabor, I see. This might be the difference between lang ="und" and lang="":

"Und" stands for "undetermined", which may mean that the language is completely unidentified (e.g. the text is in Linear A), or that some process has not as yet determined what the language is (a book that nobody has read yet, e.g.).
"", on the other hand, simply means that we aren't providing language information, either because it doesn't exist (the text is not in a natural language), or because we don't want to impose a default when we don't know if it's right, or because our source provided no information, or for any other reason. "Und" makes a statement that we are ignorant, but "" makes no statement at all.

http://lists.w3.org/Archives/Public/xml-editor/2002JulSep/0062.html
For us, both lang="" or no attribute give the same results then. But... nodes without language context? For accessibility all information should ideally have a textual representation.

gábor hojtsy’s picture

Yeah, nodes used for math formulas without explanation are good examples. Once they get an "explanation" field, users should add language information. That is really a site building detail and we can encourage that but cannot do much to enforce it.

gábor hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All in-all looks like we agree that the current patch is as good as we can get. And a huge improvement to what we had before, right? :)

BarisW’s picture

Agreed. Great work all!

hanno’s picture

Sure, this patch would be really helpful for multilingual sites! And let's have another philosophical discussion about the difference between null, empty and undetermined languages after we have implemented D8MI :)

chx’s picture

I usually skip language patches when reviewing RTBC patches but I would like to simply observe this one misses a test.

gábor hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Let's add some then.

fastangel’s picture

One question about the test. The test should be as follows:

  • On two languages.
  • Create a node language.
  • Enable another language and see if the html node has returned the correct dir.

Would correct this behavior?.

gábor hojtsy’s picture

@fastangel: there is probably a node.test case in there that already creates nodes in different languages. I think you can ensure those nodes are promoted to front page and then check /node and the markup for each node and their language info (which should be different since you have multiple nodes in multiple languages). So I think this can be implemented with a little addition to existing tests.

However, if you don't find such tests, yes, the idea basically is you create 2 or more nodes with different languages. Then you visit the page in a language (eg. /de/node and /it/node) and check if the nodes with different languages do indeed have specific markup added.

Did this help or do you need more guidance?

fastangel’s picture

Thanks @gabor. I'm working on it.

gábor hojtsy’s picture

@fastangel: how is it going, do you need help?

fastangel’s picture

@Gábor Sorry for delay,

Now I have almost implemented the test. The test is in the node module. And what does is the following:
* Active two languages ​​(English and Spanish).
* Create two nodes (English and Spanish).
* Set the default English language.
* Add the condition to check if the Spanish node on the cover is labeled dir. Although the question I have is how to find out which is the node (spanish). ¿With label node-{nid}?. At this point I'm stuck.

Regards.

gábor hojtsy’s picture

You can use assertRaw() to assert for concrete node wrapper tag markup.

hanno’s picture

@fastangel wow great work so far! To make this test fully complete, besides a the Spanish language, we could also create a node in the Arab or Hebrew language to check the dir tag as this is only printed on this condition: '$node_language->direction != $language->direction'

gábor hojtsy’s picture

@fastangel: it would be much much easier to help if you could post your interim patch. Looks like we can figure this out together much more easily.

fastangel’s picture

Status: Needs work » Needs review
StatusFileSize
new2.57 KB

@Gábor: Attached the patch. I have never worked with test. So I'm taking so long. I hope you can help me. I think it should work but do not know if I've missed something.
@Hanno: Good idea. If @Gábor looks good my test I add new idiom with different direction.

gábor hojtsy’s picture

I doubt Spanish would be RTL, at least definitely not how you set it up. So assert-ing for RTL would/should not pass. You can also use some regular expressions to ensure the tags are found at the right place. I'll take a look at extending your patch shortly.

fastangel’s picture

It's true. I forgot to change the configuration of Spanish: D. I'll see how to do it with a regular expression.

Status: Needs review » Needs work

The last submitted patch, locale-node-language_2.patch, failed testing.

fastangel’s picture

Status: Needs work » Needs review
StatusFileSize
new2.59 KB

I attach new patch. I am not sure that the pattern in assert is correct.

Regards.

Status: Needs review » Needs work

The last submitted patch, locale-node-language_3.patch, failed testing.

good_man’s picture

Status: Needs work » Needs review
StatusFileSize
new3.39 KB

My 2-cents:

- 'dir' and 'lang' attributes should be added to 'attributes_array' not to content array.
- $pattern in test should examine each attribute alone. This way we guarantee both tests pass, and also the pattern will be much easier.

Attached a patch for that.

gábor hojtsy’s picture

Status: Needs review » Needs work

@good_man, @fastangel: if you use |lang="ar"| only to match, that could just as well be assertRaw() without the |s. However, I don' think this is really doing a good thing. The original test from @fastangel tested the /node list page (good), but created Page nodes, which will not show up there (bad), so I think we should create articles instead (or promote pages, but why not create articles?). @good_man, you modified to test for node/$nid, which is not really a good place to test if multiple nodes displayed would include the different language codes and directions.

I'd suggest we

1. Create article nodes.
2. Create three of them: English, Spanish and Arabic.
3. Test on /node
4. Test if the right node gets the right language tag and the right direction.
- The English one should not get lang or dir (I assume the page/default language for the site would be English still for the test)
- The Spanish one should get lang but not dir
- The Arabic one should get both lang and dir

The patterns can be devised based on the markup produced by the template output.

good_man’s picture

English site will got no lang nor dir if it's default site lang:

  if ($node_language->language != $language->language) {
      $variables['attributes_array']['lang'] = $variables['language'];
      if ($node_language->direction != $language->direction) {
        $dir = array('ltr', 'rtl');
        $variables['attributes_array']['dir'] = $dir[$node_language->direction];
      }
    }

I think we need to change this too, English nodes should still have lang & dir attribute even if it's default site language. What do you think?

hanno’s picture

@good_man html elements inherit its language code from the upper structure (with English as default, we will have <html lang="en">) and only get a language tag if it differs, according to http://www.w3.org/TR/html4/struct/dirlang.html

gábor hojtsy’s picture

@good_man: in other words (expanding on @Hanno), its not the site default language that is in question here, but the page interface default language, that was already marked up on the HTML root tag. We only need to specify the lang and dir values if they are different from the default (or more precisely if different from the one we had on our wrapper).

good_man’s picture

Status: Needs work » Needs review
StatusFileSize
new4.51 KB

Thanks for the feedback. Your point is very true. Attached a patch with @Gabor review from #60.

fastangel’s picture

StatusFileSize
new4.53 KB

Hi,

I attach new patch. With patch use pattern instead of rawText. And check in node not in node/nid.

Regards.

fastangel’s picture

Sorry for my latest patch. I don't saw the patch of @good_man :D

Status: Needs review » Needs work

The last submitted patch, locale-node-language_5.patch, failed testing.

gábor hojtsy’s picture

@good_man's version is getting to look pretty good. However, to ensure that the pattern matches in the same tag, IMHO all ".*" should be replaced with "[^<>]*" which would ensure that there are no end-of tag items there. Otherwise the pattern could match

<div class="node-12"><div lang="en">

The "[^<>]*" would quality the characters we are looking for that those should not be HTML tag markers. That would perfectly focus the test to ensure we are looking at the right thing.

good_man’s picture

Status: Needs work » Needs review
StatusFileSize
new4.53 KB

Replacing .* with [^<>]* to match the current tag only, not another one in the page.

gábor hojtsy’s picture

I think this looks very good now. Just one small thing, the "global $base_url;" code is not used for anything else, so let's remove that. Otherwise the code looks good IMHO.

gábor hojtsy’s picture

Status: Needs review » Needs work
good_man’s picture

Assigned: fastangel » Unassigned
Status: Needs work » Needs review
StatusFileSize
new4.58 KB

Removed $base_url and also update the comment of the test function.

dries’s picture

I reviewed the patch and it looks good.

It may be worthwhile to add one or two sentences of documentation for those who don't know the 'dir' and 'lang' attribute and why it matters. I don't feel super strong about adding the documentation, but it could be helpful.

gábor hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new4.87 KB

Now with plenty of docs. No functionality change. I agree this patch looks good and has plenty of test coverage to prove that :)

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/locale/locale.testundefined
@@ -1956,6 +1956,89 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
+  /**
+   * Test if a dir and lang tags exist in node's attributes, if node's
+   * language and direction are different from the interface language and

Function summary should be one line.

+++ b/core/modules/locale/locale.testundefined
@@ -1956,6 +1956,89 @@ class LocaleContentFunctionalTest extends DrupalWebTestCase {
+   */
+  private function createNodeArticle($langcode) {

Private makes this inaccessible in case someone wanted to subclass the test (and I have done that with core tests before in contrib). Could we make that protected?

5 days to next Drupal core point release.

gábor hojtsy’s picture

Status: Needs work » Needs review
StatusFileSize
new7.3 KB

There are plenty of multiline phpdoc and all utility functions are marked private in this test, so plenty of inspiration for the patch author to do what was in the patch. Its just following patterns. This patch fixes the ones you called out and the other bad examples in the test. Looks good now?

catch’s picture

Status: Needs review » Reviewed & tested by the community

Just because there are bad examples in a file doesn't mean we should follow them. Looks fine to me, moving to RTBC but will wait for the test bot.

gábor hojtsy’s picture

Yeah, that is why it looked important to fix the existing bad examples.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Alright, thanks for the quick turnaround. Committed/pushed to 8.x, thanks!

hanno’s picture

Great work and happy to see this in!

Just wondering, is it possible to backport this patch (with little effort) to D7?

gábor hojtsy’s picture

@Hanno: I don't think this is backportable without changes in node.tpl.php, which would make existing themes not carry the feature. Do you see a way?

mgifford’s picture

Might be useful to simply describe how to bring it into themes that are looking to demonstrate accessibility best practices like Genesis & Zen.

good_man’s picture

AFAIK introducing new features is only for next major release. Anyhow for D7 similar feature is in contrib Language Swticher (the dev version).

hanno’s picture

@good_man, this issue is an (accessibility) bug, not a feature, so backporting would be preferred. However, if there are drawbacks for backporting, affected users could indeed add this functionality manually with instructions or a contributed module.
@gabor I will have a look in a fresh D7 install and see what happens.

hanno’s picture

StatusFileSize
new195.14 KB

Did a test this weekend for Drupal 7 for this patch.
With a small change for the new D8 function language_load() (http://drupal.org/node/1260510) it works exactly as for D8. Also in existing themes. Attached a screenshot. Will provide the patch later.

hanno’s picture

During testing in D8 I found two issues:
* the language attribute is not attached to the title in a full page view. As the title rendering is in discussion right now, I propose to wait until that has been settled (#1328048: Page title location needs to be flexible and #1188394: Make title behave as a configurable, translatable field (again))
* the node language is also attached to the comment section and labels as well, while the language for these elements is in fact the interface language. Not sure what the desired behaviour should be, will post an issue for this. If a node is in another language, should we print the field labels in that language as well or in the interface language? And do we expect comments in the node language or in the interface language?

Edit: issue created: #1368410: When viewing a node in another language as interface language, comment form should have correct language

gábor hojtsy’s picture

@Hanno: I do understand those are related, but are not the same issue that we resolved here. Please open related issues. Thanks!

hanno’s picture

Status: Fixed » Closed (fixed)

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

gábor hojtsy’s picture

Issue tags: +language-content

Tagging for the content handling leg of D8MI.

pancho’s picture

pancho’s picture

Issue summary: View changes

code tag added