Attribute hreflang not allowed on element li at this point

li element shouldn't have the data-drupal-link-system-path either. Not sure why we want the li to have the active class. It doesn't happen for menus anywhere else.

No longer applies.

Not sure whether this helps in any way, but running the patch now leads to the following error (or, at least, it does for me):

Checking patch core/modules/system/src/Tests/Theme/FunctionsTest.php...
error: while searching for:
    $expected_links .= '<li class="a-link"><a href="' . Url::fromUri('base:a/link')->toString() . '">' . Html::escape('A <link>') .                                              '</a></li>';
    $expected_links .= '<li class="plain-text"><span class="a/class">' . Html::escape('Plain "text"') . '</span></li>';
    $expected_links .= '<li class="html-text"><span class="unescaped">' . Html::escape('potentially unsafe text that <should> be es                                             caped') . '</span></li>';
    $expected_links .= '<li data-drupal-link-system-path="&lt;front&gt;" class="front-page"><a href="' . Url::fromRoute('<front>')-                                             >toString() . '" data-drupal-link-system-path="&lt;front&gt;">' . Html::escape('Front page') . '</a></li>';
    $expected_links .= '<li data-drupal-link-system-path="router_test/test1" class="router-test"><a href="' . \Drupal::urlGenerator                                             ()->generate('router_test.1') . '" data-drupal-link-system-path="router_test/test1">' . Html::escape('Test route') . '</a></li>';
    $query = array('key' => 'value');
    $encoded_query = Html::escape(Json::encode($query));
    $expected_links .= '<li data-drupal-link-query="'.$encoded_query.'" data-drupal-link-system-path="router_test/test1" class="que                                             ry-test"><a href="' . \Drupal::urlGenerator()->generate('router_test.1', $query) . '" data-drupal-link-query="'.$encoded_query.'" d                                             ata-drupal-link-system-path="router_test/test1">' . Html::escape('Query test route') . '</a></li>';
    $expected_links .= '</ul>';
    $expected = $expected_heading . $expected_links;
    $this->assertThemeOutput('links', $variables, $expected);

error: patch failed: core/modules/system/src/Tests/Theme/FunctionsTest.php:279
error: core/modules/system/src/Tests/Theme/FunctionsTest.php: patch does not apply

Patch rerolled

Changes to pass tests

This patch (#26) is working for me. I've no longer got the hreflang attribute within the list element:

<ul class="links"><li class="en"><a href="/en" class="language-link" hreflang="en" data-drupal-link-system-path="&lt;front&gt;">English</a></li></ul>

Cool, we'll soon be able to check on http://validator.w3.org/check without falling ill :-)

I did validate it with https://validator.w3.org/ (Doctype HTML5(experimental)). Not sure whether we would have to check it any further.

So weirdly this is contradicting there own examples see https://www.w3.org/MarkUp/xhtml2/wiki/XHTML2/i18n/hreflang_examples

Are we sure we haven't discovered a bug in the validator?

Hum, you may be right.
But at first, everyone would think "the W3C must be right"

Not necessarily, the link describes UL nested in a P, not UL's nested in NAV elements, which is the case in this issue. Though it must be said that W3C also gives an error with the examples presented on the website.

The example above is for xhtml not html5. I still think the comment in #2454289-3: Attribute hreflang not allowed on element li at this point holds true as there is no link to anything.

So what do we do now ?

I guess #34 and #3 are right, and it could be also a bug in the validator.
Nonetheless it costs nothing to us to remove this attribute !

https://validator.w3.org/nu, another w3c tool, returns the same error as http://validator.w3.org/check. Both state that the HTML5 validator is in an experimental stage, so it wouldn't suprise me if #34 and #3 are correct. However, I can't find any issues on the respective github pages pertaining to this hreflang issue.

in fact, Drupal seems to use the attribute hreflang on the "li" to add an "is-active" class on it in the language switcher block (without it, all the "li" in the language switcher are set "is-active")

if only it was named "data-hreflang" instead it would be W3C valid

When I tried to apply this patch, I got the following error message: "Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/attribute_hreflang_not-2454289-26.patch".
So I recreated the patch for the version of Drupal I am using (8.3.7).

This is the new patch targeted against 8.5.x-dev.

This is the new patch targeted against 8.5.x-dev: hreflang_attribute_not-2454289-45.patch. Needs review.

@ajzetat : Something got wrong with your patch updad ?

One small nitpick.

+++ b/core/modules/language/tests/src/Functional/LanguageSwitchingTest.php
@@ -93,8 +92,8 @@ protected function doTestLanguageBlockAuthenticated($block_label) {
+      0 => array('langcode_class' => 'en'),
+      1 => array('langcode_class' => 'fr'),

This should use the short notation for arrays ([] instead of array())

Applied suggestion at #49 by borisson_.

#50 applied to core 8.53 and fixed the issue according to https://validator.w3.org/nu/

This patch applies to 8.5.6 without issues and solves the problem and validation now passes. Would be nice to get this in.

The patch also applies to 8.6 with proper validation. However, I'm wondering whether we won't be breaking any functionality in the drupal.active-link library. Are we sure this is the way to go?

Patch for use with 8.6.x

Patches should be applicable to 8.7.x now, so I've added a test for that.

Here is an alternative approach that wont affect the drupal.active-link library, (but probably breaks the active link library for all other links where the hreflang was on an actual a tag)

@Rob Holmes: If I understand correctly, patch #60 replaces the hreflang attribute with a custom HTML5 data attribute. I think there is a rule that states that data attributes should not be used if there is in fact already an existing attribute which is more appropriate for storing the data (language in this case). Hreflang is the more appropriate attribute even if it isn't allowed on a li element. Therefore, while applying this patch might resolve the w3c validation error, it would introduce another non-detectable 'error'.

That's a fair assessment,

Maybe we need a test for the active link library to ensure that nothing gets broken by any changes introduced by removing the hreflang from li elements?

That might be useful. The LanguageSwitchingTest seems to check some things with the active-link library, so I would say we're at least partway there. I've noticed that the library itself has a redundant variable initializer, but I'm not touching that since it has a big "DO NOT EDIT THIS FILE" warning in it.

The patch in #58 is functionally the same as the patch in #50, so I have no issues with that. However, issue #2713451 introduced a long comment stating why the 'is-active' class ha been placed on the li element. Apparently, it is needed to allow different styling on an active element. I'll ask the reporter of that issue to take a look at this issue.

> Apparently, it is needed to allow different styling on an active element.

That's pretty much the reason the 'is-active' class should be on the LI, and not just the A element. Sometimes you want to apply some sort of highlighting to the whole LI element.

Hello, I update this solution, because when we get originalSelectors in this array get all the items, not only li. And as a result we get a bug through which the two elements are placed in the class "is-active".

Updated patch and fix problem with failing test.

Fixed issue for not authentication users.

I think for the original issue or question the solution is rather simple.
Please do tell me if I am forgetting something but in this patch I just removed the hreflang from the LI element.

@Mschudders while that technically fixes the validation error, it regresses the ability for Drupal.behaviors.activeLinks to highlight which language is the active one (which it does via javascript)

Any solution to this problem will have to provide valid HTML as well as retaining the ability to have a correctly applied .active class.

Okay but I still see the is-active classes:

From the screenshot you have attached it looks like you have an .is-active class on the NL language as well as the EN Language links?

@Mschudders, can you apply my patch #63? In this patch I fix bug when hreflang attributes missing in li elements. And ".is-active" sets correctly.

Changed the test ActiveLinkResponseFilterTest to validate the results.

Fixed error in the test.

NR for testbot.

Please credit eugene.brit and ksemihin since they helped Zulljin locally in the office.

RTBC.
Patch 82 resolves the related accessibility issue and the test looks correct and corresponds to the changes required for w3c validation.

It also applies cleanly to head of 8.7.x and works correctly for 8.7.x as well.

Thanks

Re #85 - what's the accessibility issue you refer to? It isn't mentioned anywhere earlier in the issue.

accessibility:
for the language switcher block in particular (our site is two languages English / French)
Language of Parts (Level AA)
1. All English pages: require (lang=”fr”) on Francais language toggle link
2. All French pages: require (lang=”en”) on English language toggle link

@joseph.olstad - The issue summary here is about a HTML validation error, where <li hreflang="es"> isn't allowed. That has nothing to do with WCAG Language of Parts.

You can't solve Language of Parts with the hreflang attribute. The attribute indicates the language of the linked resource, but WCAG Language of Parts is concerned with the language of the link text. There's a separate issue to address this on the language switcher block - #3049122: Links in language switcher block do not conform to WCAG language-of-parts. I started to look into a solution for that, and noticed the HTML validation error on the containing <li> element, which led me to this issue.

I tried patch #82 with the specimen content in the Umami demo, and the language switcher block:

• It does solve the HTML validation error from <li hreflang="es">.
• It leaves the <a hreflang="es"> attribute in place on the link.
• It doesn't solve the WCAG Language of Parts problem with the language switcher block. Specifically, it doesn't introduce any lang attributes. That's OK, because the issue summary here doesn't claim to fix that.

Crediting eugene.brit and ksemihin per #84

A couple of things which might need work. I don't know if they're important though, so I'm leaving it at RTBC.

1. What's going on with the <li data-drupal-link-system-path> attribute? In #4, _nod suggested removing it from the list item. It's still there though. It looks like an earlier patch (#26) removed it, but by patch #82 it's come back.
2. The new data-drupal-language attribute isn't very well explained.

+++ b/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php
@@ -187,7 +187,10 @@ public static function setLinkActiveClass($html_markup, $current_path, $is_front
-        if ($node->hasAttribute('hreflang') && $node->getAttribute('hreflang') !== $url_language) {
+        if ($node->nodeName !== 'a' && $node->hasAttribute('data-drupal-language') && $node->getAttribute('data-drupal-language') !== $url_language) {
+          $add_active = FALSE;
+        }
+        elseif ($node->nodeName === 'a' && $node->hasAttribute('hreflang') && $node->getAttribute('hreflang') !== $url_language) {
           $add_active = FALSE;
         }

I think we can tidy this up a bit by doing.

        $attribute = $node->nodeName === 'a' ? 'hreflang' : 'data-drupal-language';
        if ($node->hasAttribute($attribute) && $node->getAttribute($attribute) !== $url_language) {
          $add_active = FALSE;
        }

Also #82 had changes to the core/misc/active-link.js that were not build from core/misc/active-link.es6.js so I've fixed that.

#95 not applied for drupal 9.0.

Here is 95 rerolled for 9.0.x
hope the reroll is ok.

reroll for 9.1.0

Patch in comment #95 appears to work well for Drupal 8.9.14

needs a reroll for 9.0 and probably 9.1/9.2/9.3

Patch #95 in Drupal 8.9.19 makes my default links language switcher block having no more hreflang tag attributes on the li.
BUT it makes all li having an is-active class.

Patch #101 in Drupal 9.2.5 works fine, no more hreflang tag attributes on the li. And I do have the "is-active" class right on the li containing the displayed language link.

I just tried patch 101 and 109 and I am getting the following error.

My settings is:
Drupal Version
9.4.5

PHP
Version
7.4.30

Database MySQL
Version
5.7.39

Checking patch core/includes/theme.inc...
error: core/includes/theme.inc: No such file or directory
Checking patch core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php...
error: core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php: No such file or directory
Checking patch core/misc/active-link.es6.js...
error: core/misc/active-link.es6.js: No such file or directory
Checking patch core/misc/active-link.js...
error: core/misc/active-link.js: No such file or directory
Checking patch core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php...
error: core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php: No such file or directory

Note I am testing in a local environment and then upload to a shared server

Patch #109 applies on 9.5.x, but needs a reroll for 10.1.x as core/misc/active-link.es6.js was renamed to core/misc/active-link.js

Rerolled 109:
This new patch works with D9.4.9 and D9.5.0

straight up reroll, I changed nothing except fix conflict rejects on hreflang substitute with data-drupal-language in
core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php

ok the CI framework is really picky, patch 113 is clean for D9.4.9 however it will work for D9.5.0 also

however to please the very picky CI framework that doesn't accept fuzz, I've got another D9.5.0 patch, 114

Re-roll #113 for 10.0.x

Re-roll #116 against 10.1.x and 11.x. The custom commands failure from #116 should have been fixed.