[accordion] Automatically set ARIA attributes#48559
Conversation
mj12albert
added
accessibility
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
mj12albert
force-pushed
the
accordion-auto-aria-attributes
branch
from
0afbcc2 to
99369fb
Compare
mj12albert
requested review from
silviuaavram and
siriwatknp
and removed request for
Janpot
siriwatknp
left a comment
siriwatknp
left a comment
There was a problem hiding this comment.
Quick question, which pattern is the doc update following?
You mean an aria pattern or reorganizing the structure of the sections and such? |
| hasGeneratedRegionId && | ||
| (isRegionAlwaysMounted || (isDefaultCollapseTransition && expanded) || isRegionMounted); | ||
|
|
||
| const regionSlotProps = mergeSlotProps( |
There was a problem hiding this comment.
maybe memoize this so it's not computed on every render
| slotProps: | ||
| accordionContext === undefined | ||
| ? slotProps | ||
| : { | ||
| ...slotProps, | ||
| root: rootSlotPropsWithRelationship, | ||
| }, |
There was a problem hiding this comment.
maybe memoize this value so we don't compute {...slotProps, root: ... } on every render if the context is true
There was a problem hiding this comment.
This doesn't help much because useSlot runs on every render anyway, and also doesn't help the common case of inline object slot props: slotProps={{ region: {...} }}
| ## Accessibility | ||
|
|
||
| The [WAI-ARIA guidelines for accordions](https://www.w3.org/WAI/ARIA/apg/patterns/accordion/) recommend setting an `id` and `aria-controls`, which in this case would apply to the Accordion Summary component. | ||
| The Accordion component then derives the necessary `aria-labelledby` and `id` from its content. |
There was a problem hiding this comment.
Maybe keep the section and also mention that we are computing the ids and aria-controls automatically but they can be overriden by props?
There was a problem hiding this comment.
The ARIA pattern is already linked via the "Chip" at the top, and since users generally don't need to do anything about ids anymore, there's not a lot of value in mentioning this as it's just describing an implementation detail/re-stating something from the APG
Also aria-controls isn't useful anyway as it historically has bad support and JAWSs ignores it by default
| const summaryId = useId(summaryIdProp); | ||
| const regionId = useId(summaryAriaControlsProp); | ||
| const hasSummaryAriaControls = summaryAriaControlsProp != null; | ||
| const [isRegionMounted, setIsRegionMounted] = React.useState(false); |
There was a problem hiding this comment.
Is it an option to always mount the region node, but instead hide its content? Such that aria-controls always resolves, but we don't add an extra render to track the mount.
It could make sense to add a benchmark test like https://github.com/mui/base-ui-charts/tree/master/test/performance first to verify we're not adding extra renders.
There was a problem hiding this comment.
Sadly no, because the existing structure is:
<TransitionSlot {...transitionProps}>
<RegionSlot {...regionProps}>{children}</RegionSlot>
</TransitionSlot>always mounting the region would affect transitions
But yeah it's needed exactly to avoid orphaned aria-controls in cases like slotProps.transition={{ unmountOnExit: true }} or mountOnEnter: true
| ariaControls?: string | undefined; | ||
| } | ||
|
|
||
| export const NOOP = () => {}; |
There was a problem hiding this comment.
I see that this is used to check if the Summary is inside an Accordion or not. Maybe to add an explicit boolean to the context with that information, to communicate the intent better?
| }); | ||
| }); | ||
|
|
||
| it('does not allow slotProps ids to break the relationship', async () => { |
There was a problem hiding this comment.
This behavior might be surprising for consumers, having their ids overridden when passed via slotProps when rendering Summary inside Accordion. Should we at least show a non-prod onlyOnce error?
| export function useAccordionContext(): AccordionContextValue { | ||
| const context = React.useContext(AccordionContext); | ||
| if (context === DEFAULT_CONTEXT_VALUE) { | ||
| if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
should this check come before the context check?
also, should we only warn once?
4 participants
Preview: https://deploy-preview-48559--material-ui.netlify.app/material-ui/react-accordion/
Also polished all the docs and demos, in particular added
Usage guidelineswith updated accessibility recommendations. The only previous recommendation was to add explicit ids and aria-controls which is no longer necessary.Closes #48305