Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[material-ui][Accordion] Default Accordion now uses div's nested within a heading tag, which is invalid HTML #44153

Open
johndatserakis opened this issue Oct 19, 2024 · 3 comments · May be fixed by #44408
Open

[material-ui][Accordion] Default Accordion now uses div's nested within a heading tag, which is invalid HTML #44153

johndatserakis opened this issue Oct 19, 2024 · 3 comments · May be fixed by #44408
Assignees
@ZeeshanTamboli
Labels
accessibility a11y breaking change bug 🐛 Something doesn't work component: accordion This is the name of the generic UI component, not the React module! regression A bug, but worse

Comments

@johndatserakis
Copy link

johndatserakis commented Oct 19, 2024 •
edited
Loading

Steps to reproduce

Link to live example: (required)

Steps:

  1. Go to the official accordion component page
  2. Inspect the "Accordion 1" content
  3. See in the inspector that divs are nested within a heading tag
Screenshot 2024-10-18 at 10 42 47 PM

Current behavior

Accordions have nested div's within a single heading (h3) tag, which is invalid HTML.

Expected behavior

Accordion should generate valid HTML.

Context

Hi there. In the process of upgrading my MUI version to v6.1.4 I came across notes on changes to the Accordion component. When I saw that it mentioned the "Accordion Summary is now wrapped with a default <h3> heading element" I found that strange, as I remembered the nested Accordion content was rendered within divs.

In inspecting the v6.1.4 Accordion HTML from the live docs, and inspecting my newly upgraded local code, I found that the Accordion content divs were in fact now being rendered with an h3 element.

Heading elements cannot contain div elements, as that is invalid HTML. Heading elements can only contain what are referred to as "phrasing elements" which are things like span, strong, em, button, etc. Stackoverflow post, whatwg/html thread.

I traced back this back to this issue and this PR. In the example brought up in that thread, within the h3, only button's and spans are rendered, which is valid HTML. There are no nested div's.

Screenshot 2024-10-18 at 10 31 37 PM

If you look at the live Accordion docs page, you can see the nested divs.

Screenshot 2024-10-18 at 10 32 39 PM

To test what I'm saying about the invalid HTML, you can use the official W3C markup tester: https://validator.w3.org/nu/#textarea.

Provide this to pass as valid HTML.

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Document</title>
  </head>
  <body>
    <h3><button>Hi</button></h3>
  </body>
</html>

Provide this to fail as invalid HTML.

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Document</title>
  </head>
  <body>
    <h3><div>Hi</div></h3>
  </body>
</html>

Anyways, I just wanted to raise.

To get around this locally for now, I'm using the following to set the h3 back to a div:

<Accordion slotProps={{ heading: { component: 'div' } }}>
  <AccordionSummary ...>
    <Typography>...</Typography>
  </AccordionSummary>
  <AccordionDetails>...</AccordionDetails>
</Accordion>

Your environment

npx @mui/envinfo 
  Brave Browser

  System:
    OS: macOS 15.0.1
  Binaries:
    Node: 21.1.0 - ~/.nvm/versions/node/v21.1.0/bin/node
    npm: 10.2.0 - ~/.nvm/versions/node/v21.1.0/bin/npm
    pnpm: 9.12.2 - ~/.nvm/versions/node/v21.1.0/bin/pnpm
  Browsers:
    Chrome: 130.0.6723.58
    Edge: 130.0.2849.46
    Safari: 18.0.1
  npmPackages:
    @emotion/react: ^11.13.0 => 11.13.3 
    @emotion/styled: ^11.13.0 => 11.13.0 
    @mui/base: ^5.0.0-beta.59 => 5.0.0-beta.59 
    @mui/core-downloads-tracker:  6.1.4 
    @mui/icons-material: ^6.1.4 => 6.1.4 
    @mui/material: ^6.1.4 => 6.1.4 
    @mui/material-nextjs: ^6.1.4 => 6.1.4 
    @mui/private-theming:  6.1.4 
    @mui/styled-engine:  6.1.4 
    @mui/system:  6.1.4 
    @mui/types:  7.2.18 
    @mui/utils:  6.1.4 
    @mui/x-data-grid: ^7.12.0 => 7.21.0 
    @mui/x-date-pickers: ^7.11.1 => 7.21.0 
    @mui/x-internals:  7.21.0 
    @types/react: ^18.3.3 => 18.3.11 
    react: ^18.3.1 => 18.3.1 
    react-dom: ^18.3.1 => 18.3.1 
    typescript: 5.6.3 => 5.6.3

Search keywords: Accordion
@johndatserakis johndatserakis added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 19, 2024
@zannager zannager added the component: accordion This is the name of the generic UI component, not the React module! label Oct 21, 2024
@mj12albert mj12albert added accessibility a11y and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 23, 2024
@mj12albert
Copy link
Member

mj12albert commented Oct 23, 2024 •
edited
Loading

An easy fix could be to replace all the divs with spans, a small breaking change assuming customizations dont depend so much on the markup here like classes

spans are phrasing content which are allowed within heading tags

What do you think @aarongarciah ?

@aarongarciah
Copy link
Member

@mj12albert looks like the best path forward

@mj12albert mj12albert assigned mj12albert and unassigned siriwatknp Nov 4, 2024
@ZeeshanTamboli ZeeshanTamboli linked a pull request Nov 18, 2024 that will close this issue
Open
@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Nov 18, 2024 •
edited
Loading

This is a regression from #42914 as you mentioned. I have set up a PR to fix it: #44408.

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work breaking change regression A bug, but worse labels Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ZeeshanTamboli ZeeshanTamboli

Labels
accessibility a11y breaking change bug 🐛 Something doesn't work component: accordion This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Accordion] Fix invalid HTML inside heading ZeeshanTamboli/material-ui
6 participants
@mj12albert @aarongarciah @johndatserakis @siriwatknp @ZeeshanTamboli @zannager