Wrapping embedded entities in <article> is bad for accessibility, use <div> instead

Articles like sections are both regions that should have an associated heading.
https://thepaciellogroup.github.io/AT-browser-tests/test-files/article.html
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article
http://html5doctor.com/the-article-element/

Seems to me that entities are are too generic. They could be articles, but we shouldn't assume that they are.

Any reason to not just make the <article> a <div>?

Edit: Adding a thanks to Dennis Lembrée for pointing out the issue from Twitter & pointing me to good direction on this too.

This was introduced in #2712111: Embed specific attributes are cached along with the entity. @Dave Reid also questioned this in #2712111-28: Embed specific attributes are cached along with the entity, but @slashrsm said he thought it was better in #2712111-30: Embed specific attributes are cached along with the entity. Accessibility wasn't mentioned at all in that issue.

Marked #2927867: Why is my embedded image being wrapped in an <article> tag? as a duplicate of this.

Thanks Wim for looking into this.

Quick patch, let's see how many tests break (which aren't already broken in HEAD).

Okay, this should fix the non-HEAD failure.

Those failures are expected. This is squarely in a reviewable state.

+++ b/templates/entity-embed-container.html.twig
@@ -12,4 +12,4 @@
-<article{{ attributes }}>{{ children }}</article>
+<div{{ attributes }}>{{ children }}</div>

Without entering into the discussion about what tag is more appropriate, changing it here would introduce a BC break for existing sites (it can actually break existing sites' theming). I would then suggest we either consider adding this to a new branch, or create a configurable setting (which may or may not be exposed on the UI), where: on new sites the setting is configured to use a <div> tag, and on existing sites it will keep using <article>. Thoughts?

would introduce a BC break for existing sites

Hm … interesting remark!

Three thoughts:

1. It's our duty to fix the accessibility of existing sites too.
2. Any CSS targeting this wrapper tag ought to be targeting it using classes already anyway.
3. This module is only in beta, not in RC or stable, so it's not frozen.

So IMHO disruption should be minimal, and people know the consequences of using beta-stability modules.

I agree that these should be generic divs, overuse of the articles isn't semantically correct and could be frustrating for assistive tech users.

Theres been a core issue similar which had been fixed (I found it when looking for this issue), so following that appraoch makes sense.
https://www.drupal.org/project/drupal/issues/2878115

+1 for making this change.

👍

@joelhsmith Awww! ❤️❤️❤️

#10: per #2, #13 (which refers to #2878115: Make the HTML wrapper tag for media items more semantically correct, which basically made the same change in core) and #15 (who also opened this issue), I think it's fair to say we've actually already had the necessary accessibility review.

#12:

So IMHO disruption should be minimal, and people know the consequences of using beta-stability modules.

Can we at least add a specific warning at the notes of the release where this will be included, so this change stands out to people reading that before updating?

Thanks!

#2328659: Convert all existing entity_embed tests from WebTestBase to BrowserTestBase improved the tests, which means this patch no longer applies.

Can we at least add a specific warning at the notes of the release where this will be included, so this change stands out to people reading that before updating?

+1. Done: added release notes snippet to IS.

Rerolled!

🎉

If this issue was marked critical on the basis of accessibility, that's WAY off the mark, I think.

Articles like sections are both regions that should have an associated heading.

#2 is misleading in my view. I've given an explanation on an active core issue: #3029740-7: Media content is wrapped with an article wrapper if stable or classy theme is used or extended