feat(Accordion): improve accessibility #1113
Conversation
|
Size stats
|
|
Accessibility report ℹ️ You can run this locally by executing |
|
Deploy preview for mistica-web ready! ✅ Preview Built with commit 597da91. |
| @@ -24,13 +25,15 @@ const ACCORDION_TRANSITION_DURATION_IN_MS = 400; | |||
|
|
|||
| type AccordionContextType = { | |||
| index: ReadonlyArray<number>; | |||
| toogle: (item: number) => void; | |||
| toggle: (item: number) => void; | |||
There was a problem hiding this comment.
typo: toogle -> toggle
src/accordion.tsx
Outdated
| ref={ref} | ||
| dataAttributes={{'component-name': 'AccordionItem', ...dataAttributes}} | ||
| /> | ||
| ({dataAttributes, role = 'listitem', ...props}, ref) => ( |
There was a problem hiding this comment.
Adding list/listitem roles by default (similar to Rows)
There was a problem hiding this comment.
This solves the relationship between the accordion group and each of the accordions in case they have relation between them, since they could also be only regions of content with no relation between them.
How the relationship between the header and the content can be defined?
I see two cases:
- They are a group of controls: The accordion item acts as a
fieldsetwhere the header title is the legend and the controls are inside the collapsable panel. - They are a definition list (usually used for FAQs): The list of accordions acs as a definition list so the accordion header should have the role: "term" and the content of the collapsable section, the role: "definition"
There was a problem hiding this comment.
I'm not sure I understood you. Currently, the relation between the header and the content is made by using aria-controls in the header and aria-labelledby + role=region in the panel.
Is that what you meant?
We are following the accessibility specs described in https://www.w3.org/WAI/ARIA/apg/patterns/accordion/examples/accordion/
The fieldset tag is meant to be used to group fields inside a form. An accordion is not the case.
Also, the accordion content does not necessarily "define" the header. For example, the header can say something like "click to see more". In that case, it would be confusing to define them as term/definition.
I suggest we should stick to the specs described in the link I'm providing in this comment (the current implementation is doing that)
There was a problem hiding this comment.
Discussed offline. Alex will investigate about best practices regarding this and will come back to us with some updates. In case some changes are necessary, we can iterate on it in a different PR.
| ) | ||
| ); | ||
|
|
||
| type AccordionBaseProps = { | ||
| children: React.ReactNode; | ||
| dataAttributes?: DataAttributes; | ||
| onChange?: (index: number, value: boolean) => void; | ||
| role?: string; |
There was a problem hiding this comment.
Initially, I added "list" and "listitem" as default roles for Accordion and AccordionItem, but after discussing with @aweell, he pointed out that it may not be a good idea.
For example, if the accordions are being used to divide some content of a document in different sections, it's not the best idea to use a list for them.
We've decided to make it customizable, but without setting any default values
There was a problem hiding this comment.
Specs are updated accordingly
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| '@media': { | ||
| ['(prefers-reduced-motion)']: { | ||
| transition: 'none', | ||
| }, | ||
| }, |
# [15.8.0](v15.7.0...v15.8.0) (2024-05-17) ### Bug Fixes * **Image:** fix bad aspect ratio cases ([#1119](#1119)) ([2bc69e1](2bc69e1)) * **TextFields:** Use textError color for errors ([#1125](#1125)) ([e9c5abd](e9c5abd)) ### Features * **Accordion:** improve accessibility ([#1113](#1113)) ([f85142c](f85142c))
|
🎉 This PR is included in version 15.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Issue: Link
I've tried to use details/summary HTML markup, but there are some things that were not great about it:
I believe that the current implementation is way more clean. In terms of accessibility, I've compared summary/details with it, and our implementation reads everything that the summary one reads.
Changes on this PR: