The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
Claro-theme always renders media image to an html-tag article even if the image has meaning only as part of other content. This behaviour was inherited from the former Classy theme, and also remains in the Umami and Starterkit themes. The media module's default template and Olivero already use a div.
Excess use of article tag results in incorrect semantic structure for the content.
Using nested article-tags is not against the html-specification but it should not be done lightly.
Steps to reproduce
With Umami demo edit content list view by adding the articles image as field. Use rendered image only.
None of the individual rows presenting an individual node is presented as an article but the main image for each node is presented as independent article content.
Proposed resolution
Default to rendering media/images without wrapping the content inside an article tag. Additional logic may be added to identify a case where the media in itself actually is semantically an individual piece of content. For an admin theme such case has not been identified as individual media are not by default exposed for viewing as individual pages.
Remaining tasks
- Change record for the markup change.
- Decide whether to change media library templates here or in follow-up
- Accessibility maintainer re-review (the #8 review predates the retarget to main).
- Final Review and testing
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 20240321_162046.jpg | 3.42 MB | simohell |
| #2 | Screenshot 2024-03-21 at 15.08.23.png | 447.01 KB | simohell |
Issue fork drupal-3432727
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
simohell commentedComment #3
simohell commentedAn example from Voice over rotor why the use of article-tags for each media can be a problem.
Comment #6
gauravvvv commentedComment #7
smustgrave commentedComment #8
bnjmnmIt looks like the
<article>use extends beyond Claro - it's in additional places such as the core module template and Stable 9. This could be disruptive to existing sites using<article>as a selector for styling/JS/tests so it really shouldn't happen in 10 outside of Starterkit. Thi change could be introduced in 11, though!The adverse effects of this excess
<article>use can be somewhat mitigated by labeling these instances usingaria-labelor a similar approach. That way, assistive tech would see something more descriptive than just ArticleI'm going to keep the "Needs subsystem maintainer review" around as I suspect an additional review will be needed, but that's my review for this stage of the process ✔️
Comment #10
liam morlandThese changes should be made for Drupal 12.
Comment #11
smustgrave commentedThink this should be postponed and tagged to D12
Comment #12
smustgrave commentedhopefully that helps us find it when the time comes.
Comment #13
smustgrave commentedI'm digging around for a tag too so this doesn't go another rotation
Comment #14
liam morlandThanks
Comment #16
quietone commentedComment #18
mgiffordEvery time I do this I hope it will be less confusing...
Anyways, new patch. I think that's what is needed. Think this just adjusts the tests.
https://git.drupalcode.org/issue/drupal-3432727/-/tree/3432727-divs-rock
AI was used in the production of this patch (and likely my confusion).
Comment #20
kentr commentedI rebased MR !7598.
I think the currently-failing tests are legitimate and need to be fixed (they fail locally, and also failed when I reran the failed GitLab jobs).
Comment #21
kentr commentedChanging to Needs work for the failing tests in the MR.
Comment #22
liam morlandTests updated, now passing.
Comment #23
kentr commentedFTR, I don't see the problem in Default Admin, which appears to use the system module template.
Comment #24
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #25
liam morlandRerolled; tests are passing.
Comment #26
marcoscanoI agree with this change, thanks for working on this. 👍
I have updated the IS to more accurately reflect the current MR and decisions made so far in the comments.
Switching to NW mainly to address:
- We will need a Change Record, this MR changes markup in Claro for all existing sites, which will affect all claro and sub-themed pages that display media items. I believe we should have a CR describing old markup -> new markup per theme.
- After this we will still have
<article>tags in the Media Library, both in the core module template (core/modules/media_library/templates/media--media-library.html.twig) and in thedefault_adminadmin theme override (core/themes/default_admin/templates/media-library/media--media-library.html.twig). We need to decide if we want to either 1) fix that here (and likely work on the tests that that would break) or 2) file a follow-up for these two remaining spots.I am in favor of ripping the band-aid off and fixing the Media Library occurrences here too, but I don't have strong opinions if others prefer a follow-up in order not to delay things too much.
AI Disclaimer: I have used an LLM to help analyze this issue, whose output I then reviewed and adjusted myself.