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
| Comment | File | Size | Author |
|---|---|---|---|
| #86 | drupal7lang.png | 195.14 KB | hanno |
| #77 | locale-node-language-77.patch | 7.3 KB | gábor hojtsy |
| #75 | locale-node-language-75.patch | 4.87 KB | gábor hojtsy |
| #72 | locale-node-language_7.patch | 4.58 KB | good_man |
| #69 | locale-node-language_6.patch | 4.53 KB | good_man |
Comments
Comment #1
mgiffordI wish I could edit the original post...
Forgot to wrap that in code.
Comment #2
Jeff Burnz commentedProbably 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.
Comment #3
mgiffordActually, we should be able to deal with just:
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
Comment #4
jacineWe 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:langattribute. 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.Comment #5
BarisW commentedThe 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
So no need to include it in every node.
Comment #6
mgiffordAs @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.
Comment #7
gábor hojtsyRelated: #673020: Tests for adding the Content-Language HTTP header to the generated page.
Comment #8
hanno commentedprobably 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.
Comment #9
hanno commentedsorry, changed issue tag
Comment #10
mgiffordRelated issue - #1260716: Improve language onboarding user experience
All the language drop downs should have their language assigned.
Comment #11
gábor hojtsyI 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.
Comment #12
gábor hojtsyBTW 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?
Comment #13
BarisW commentedGabor, that would be awesome. And you are right, the theme layer is certainly not the best place to do this.
Comment #14
hanno commentedYeah, If we include 'dir' as well, we also tackle #780508: Set language by content language for LTR/RTL display
Comment #15
gábor hojtsyI 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.
Comment #16
fastangel commentedHi,
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
Comment #17
gábor hojtsyMarked #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().
Comment #18
fastangel commentedHi,
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
Comment #20
gábor hojtsyThis 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!
Comment #21
fastangel commentedOk 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
Comment #23
gábor hojtsy1. 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 :)
Comment #24
fastangel commented1. Ok.
2. Fixed. I did not know.
I look forward to see how we continue.
Comment #26
gábor hojtsyHm, got this when applying the patch:
Here is a rerolled version, same code (which IMHO looks good).
Comment #28
gábor hojtsyOk, 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.
Comment #29
good_man commentedAgree.
Comment #30
fastangel commentedAttached new patch. With this patch should pass the test.
Comment #31
hanno commentedOr better use
lang=""?:http://www.w3.org/TR/html5/elements.html#the-lang-and-xml:lang-attributes
Comment #32
gábor hojtsy@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?
Comment #33
BarisW commented@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).
Comment #34
fastangel commentedNew patch with global $language in IF.
Comment #35
good_man commentedComment #36
hanno commentedFirst 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:
Will node 2 get the arabic or english language and direction, or unknown?
Comment #37
gábor hojtsy@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.
Comment #38
hanno commented@gabor, I see. This might be the difference between lang ="und" and lang="":
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.
Comment #39
gábor hojtsyYeah, 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.
Comment #40
gábor hojtsyAll 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? :)
Comment #41
BarisW commentedAgreed. Great work all!
Comment #42
hanno commentedSure, 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 :)
Comment #43
chx commentedI usually skip language patches when reviewing RTBC patches but I would like to simply observe this one misses a test.
Comment #44
gábor hojtsyLet's add some then.
Comment #45
fastangel commentedOne question about the test. The test should be as follows:
Would correct this behavior?.
Comment #46
gábor hojtsy@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?
Comment #47
fastangel commentedThanks @gabor. I'm working on it.
Comment #48
gábor hojtsy@fastangel: how is it going, do you need help?
Comment #49
fastangel commented@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.
Comment #50
gábor hojtsyYou can use assertRaw() to assert for concrete node wrapper tag markup.
Comment #51
hanno commented@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'Comment #52
gábor hojtsy@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.
Comment #53
fastangel commented@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.
Comment #54
gábor hojtsyI 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.
Comment #55
fastangel commentedIt's true. I forgot to change the configuration of Spanish: D. I'll see how to do it with a regular expression.
Comment #57
fastangel commentedI attach new patch. I am not sure that the pattern in assert is correct.
Regards.
Comment #59
good_man commentedMy 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.
Comment #60
gábor hojtsy@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.
Comment #61
good_man commentedEnglish site will got no lang nor dir if it's default site lang:
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?
Comment #62
hanno commented@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.htmlComment #63
gábor hojtsy@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).
Comment #64
good_man commentedThanks for the feedback. Your point is very true. Attached a patch with @Gabor review from #60.
Comment #65
fastangel commentedHi,
I attach new patch. With patch use pattern instead of rawText. And check in node not in node/nid.
Regards.
Comment #66
fastangel commentedSorry for my latest patch. I don't saw the patch of @good_man :D
Comment #68
gábor hojtsy@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
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.
Comment #69
good_man commentedReplacing .* with [^<>]* to match the current tag only, not another one in the page.
Comment #70
gábor hojtsyI 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.
Comment #71
gábor hojtsyComment #72
good_man commentedRemoved $base_url and also update the comment of the test function.
Comment #74
dries commentedI 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.
Comment #75
gábor hojtsyNow with plenty of docs. No functionality change. I agree this patch looks good and has plenty of test coverage to prove that :)
Comment #76
catchFunction summary should be one line.
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.
Comment #77
gábor hojtsyThere 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?
Comment #78
catchJust 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.
Comment #79
gábor hojtsyYeah, that is why it looked important to fix the existing bad examples.
Comment #80
catchAlright, thanks for the quick turnaround. Committed/pushed to 8.x, thanks!
Comment #81
hanno commentedGreat work and happy to see this in!
Just wondering, is it possible to backport this patch (with little effort) to D7?
Comment #82
gábor hojtsy@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?
Comment #83
mgiffordMight be useful to simply describe how to bring it into themes that are looking to demonstrate accessibility best practices like Genesis & Zen.
Comment #84
good_man commentedAFAIK introducing new features is only for next major release. Anyhow for D7 similar feature is in contrib Language Swticher (the dev version).
Comment #85
hanno commented@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.
Comment #86
hanno commentedDid 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.
Comment #87
hanno commentedDuring 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
Comment #88
gábor hojtsy@Hanno: I do understand those are related, but are not the same issue that we resolved here. Please open related issues. Thanks!
Comment #89
hanno commentedSorry, I understand :) I created the issue #1368410: When viewing a node in another language as interface language, comment form should have correct language.
Comment #91
gábor hojtsyTagging for the content handling leg of D8MI.
Comment #92
panchoRe #87/89:
Also created #2036277: Language attribute is not attached to node title on a full page view as a followup.
Comment #92.0
panchocode tag added