Attribute hreflang not allowed on element li at this point

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.

The last submitted patch, 118: 2454289-118-10.1.x.patch, failed testing. View results →

The last submitted patch, 118: 2454289-118-11.x.patch, failed testing. View results →

I've re-read through every comment in this issue and have observed the following:

data-drupal-link-system-path

in #4, @nod said

li element shouldn't have the data-drupal-link-system-path either.

in #94, @andrewmacpherson said

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.

This hasn't been addressed, so we need to decide whether this is something that needs to be done here or not, and update the issue summary and description accordingly.

data-drupal-language

in #94 @andrewmacpherson also said

The new data-drupal-language attribute isn't very well explained.

and this hasn't been addressed yet either.

failing tests in #118 need addressing

needs re-rolling

Here's the re-rolls for 10.3.x and 11.x - both based on #118 🐛 Attribute hreflang not allowed on element li at this point Needs review

The other items from #121 🐛 Attribute hreflang not allowed on element li at this point Needs review remain outstanding.

Created a merge request and adjusted the unit test.

I think changing data-drupal-link-system-path is out of scope. This was added on purpose in #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links → and the concept of having the active class on li links has existed even before that. It's not broken and doesn't cause problems.

Not sure what/where to document on data-drupal-language, it's an internal attribute, like the other two, I don't think they are documented either.

Have not reviewed yet but issue summary could use some love

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Updated the issue summary. Didn't mention the discussion around the other data attributes, I don't really see how that's in scope. This doesn't change them in any way and we afaik specifically support active classes on list items.

Hiding patches as the fix is in the MR,

Thanks for updating the issue summary.

Applying the MR and enabling the translation modules I am seeing the change being made see screenshots.

Before

After

Don't see any open threads or pending items right now. And nothing seems off in the code.