-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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.
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
[material-ui][Accordion] AccordionSummary should be rendered as a heading that wraps a button, per W3C Accordion Pattern standards #42891
Comments
@natsid Thanks for reporting the issue with a clear description 👍. You are correct. @DiegoAndai We should consider this in the component refactor. It might be a breaking change if we implement it now or maybe not, but we should treat it as a bug since we claim to provide full accessibility support. @natsid Feel free to create a pull request if you'd like. |
Thank you for the quick reply, @ZeeshanTamboli ! I haven't looked into how involved the PR would be. At first glance it seems simple - just add a wrapper div with role="heading" and aria-level="?". However, I think we'd also need or at least want to add a new (optional) input on AccordionSummary for the aria-level value. I do think you're right that this could be a breaking change since users' CSS selectors may depend on the current structure. In addition, whatever default value we choose for aria-level could also be considered "breaking" since it may conflict with existing heading structures where it is used. Do you know if we'd be able to patch this into v5 or if it would only make sense to add into v6? |
@natsid Thanks for your insights on why it could be a breaking change. It makes sense to add it in v6 or v7, not into v5. |
Ah, OK @ZeeshanTamboli . Just trying to get a temperature check on how firm that is. I'm personally invested in seeing the fix in v5 since that is what my project is in and v6 is still alpha - and we wouldn't be able to update to v6 until it is stable. Is there any way I can make the case for a v5 bug fix? |
@natsid I created PR #42914 to fix this. In my opinion, it's not a breaking change, so it can be included in v5, but it depends on the reviewers. If not, v6 will be stable soon, targeting July 28th as per the latest release note. |
Thank you so much @ZeeshanTamboli ! |
Steps to reproduce
Link to live example: https://stackblitz.com/edit/github-zesdz2?file=src%2FApp.tsx
Steps:
Current behavior
The Material UI Accordion component neglects to meet the following W3C Accordion Pattern standard:
(source: W3C Accordion Pattern (Sections With Show/Hide Functionality))
Expected behavior
The Accordion or AccordionSummary should provide some way to wrap the element that has
role="button"
in some sort of heading element.For example:
OR:
Context
I have tried numerous ways to achieve this structure (
<h2 ...><div role="button" ...> ... </div></h2>
) while still using the Material UI Accordion, AccordionSummary, and AccordionDetails with no success. Here are some of the things I have tried already:Other solutions considered
I have attempted to avoid a total rewrite of the component, instead continuing to rely on Material UI (MUI) Accordion. However, none of my attempts proved fruitful. I have documented them here to save future toil on these dead-end paths. That being said, perhaps there is some other way to continue using MUI Accordion while still meeting the above requirement that I haven’t attempted.
✗ Pass in a React fragment that is an h2 to the AccordionSummary
Summary of solution
Each instance of the ExpandablePanel is made to pass in a title that is a heading (specifically an h2 or equivalent). This gets forwarded to the AccordionSummary.
Why this didn’t work
The AccordionSummary ultimately renders as a div with role=”button”. This is good, as the accordion summary should be a button. But the requirement is that the heading wraps the button and not the other way around. This setup has the button wrapping the h2 (essentially something like <button …><h2 …>...). While I thought we might be able to get away with this, unfortunately the outer button ends up obscuring the inner h2, so that it is not recognized as a heading. This is evident by the fact that the screen-reader does not read out “heading level 2” when the summary is in focus.
✗ Wrap the AccordionSummary in an h2
Summary of solution
The ExpandablePanel is updated to look roughly like:
Why this didn’t work
At first it seemed promising. The AccordionSummary was rendered with a div with role=”button”, so we did indeed have the h2 wrapping the button, as desired. However, unfortunately, this ended up breaking some of the functionality of the MUI Accordion. For example, the aria-controls and aria-describedby properties were not getting set properly.
✗ Pass in component=“h2” to AccordionSummary
Summary of solution
The ExpandablePanel is updated to look roughly like:
Why this didn’t work
As mentioned earlier, the AccordionSummary is typically rendered as a div with role=”button”. Setting component=”h2” meant that we ended up with something like
<h2 role=”button” …>...</h2>
. This causes [...] errors since a heading (such as an h2) implicitly has role=”heading”, which conflicts with the explicit setting of role=”button”. Semantically, one element cannot have two different roles.Your environment
npx @mui/envinfo
Search keywords: Accordion AccordionSummary heading role accessibility w3c WCAG standards a11y
The text was updated successfully, but these errors were encountered: