-
Notifications
You must be signed in to change notification settings - Fork 4.8k
ui/CollapsibleCard: include card title in trigger's accessible label #76329
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
Changes from all commits
cfc8423
6cf86e7
6c1c877
3fbb4ce
0628aef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { createContext } from '@wordpress/element'; | ||
|
|
||
| export const TitleTextContext = createContext< | ||
| | { | ||
| setTitleText: ( text: string | undefined ) => void; | ||
| } | ||
| | undefined | ||
| >( undefined ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,56 @@ | ||
| import { Collapsible } from '@base-ui/react/collapsible'; | ||
| import clsx from 'clsx'; | ||
| import type { MouseEvent } from 'react'; | ||
| import { forwardRef, useCallback, useRef } from '@wordpress/element'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import type { MouseEvent, ReactNode } from 'react'; | ||
| import { | ||
| forwardRef, | ||
| useCallback, | ||
| useLayoutEffect, | ||
| useMemo, | ||
| useRef, | ||
| useState, | ||
| } from '@wordpress/element'; | ||
| import { __, sprintf } from '@wordpress/i18n'; | ||
| import { chevronDown, chevronUp } from '@wordpress/icons'; | ||
| import * as Card from '../card'; | ||
| import { TitleTextContext } from '../card/context'; | ||
| import { IconButton } from '../icon-button'; | ||
|
ciampo marked this conversation as resolved.
|
||
| import styles from './style.module.css'; | ||
| import type { HeaderProps } from './types'; | ||
|
|
||
| /** | ||
| * Tracks the title text from a Card.Title child (via TitleTextContext) and | ||
| * falls back to the full header content text when no Card.Title is present. | ||
| * Returns the context provider value, a ref for the header content wrapper, | ||
| * and the composed trigger label. | ||
| */ | ||
| function useTriggerLabel( children: ReactNode ) { | ||
| const headerContentRef = useRef< HTMLDivElement >( null ); | ||
| const [ titleText, setTitleText ] = useState< string >(); | ||
| const [ headerText, setHeaderText ] = useState< string >(); | ||
| const titleTextContextValue = useMemo( () => ( { setTitleText } ), [] ); | ||
|
|
||
| // Fallback: read the header content's text when no Card.Title is | ||
| // present. `children` is listed as a dependency so the label | ||
| // re-syncs when the header content changes. | ||
| useLayoutEffect( () => { | ||
| if ( titleText === undefined ) { | ||
| const text = headerContentRef.current?.textContent?.trim(); | ||
| setHeaderText( text || undefined ); | ||
| } | ||
| }, [ titleText, children ] ); | ||
|
|
||
| const identifierText = titleText ?? headerText; | ||
| const triggerLabel = identifierText | ||
| ? sprintf( | ||
| /* translators: %s: title of the card being expanded or collapsed */ | ||
| __( 'Expand or collapse %s' ), | ||
| identifierText | ||
| ) | ||
| : __( 'Expand or collapse' ); | ||
|
|
||
| return { titleTextContextValue, headerContentRef, triggerLabel }; | ||
| } | ||
|
|
||
| /** | ||
| * The header of a collapsible card. Always visible, and acts as the | ||
| * toggle trigger — clicking anywhere on it expands or collapses the | ||
|
|
@@ -24,6 +66,8 @@ export const Header = forwardRef< HTMLDivElement, HeaderProps >( | |
| ref | ||
| ) { | ||
| const triggerRef = useRef< HTMLButtonElement >( null ); | ||
| const { titleTextContextValue, headerContentRef, triggerLabel } = | ||
| useTriggerLabel( children ); | ||
|
|
||
| const handleHeaderClick = useCallback( | ||
| ( event: MouseEvent< HTMLDivElement > ) => { | ||
|
|
@@ -48,14 +92,21 @@ export const Header = forwardRef< HTMLDivElement, HeaderProps >( | |
| onClick={ handleHeaderClick } | ||
| { ...restProps } | ||
| > | ||
| <div className={ styles[ 'header-content' ] }>{ children }</div> | ||
| <TitleTextContext.Provider value={ titleTextContextValue }> | ||
| <div | ||
| ref={ headerContentRef } | ||
| className={ styles[ 'header-content' ] } | ||
| > | ||
| { children } | ||
| </div> | ||
| </TitleTextContext.Provider> | ||
| <div className={ styles[ 'header-trigger-wrapper' ] }> | ||
| <Collapsible.Trigger | ||
| ref={ triggerRef } | ||
| render={ ( props, state ) => ( | ||
| <IconButton | ||
| { ...props } | ||
| label={ __( 'Expand or collapse card' ) } | ||
| label={ triggerLabel } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not create a stable id here and pass that in the context? Additionally set the In general if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I definitely agree that the current approach feels more complex than you'd expect. I initially experimented with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I thought it would be better to explicitly announce the action associated with the button. Without it, screen readers announce something like "[Card title], button, collapsed", and tooltip would just show the "[Card title]" text. Maybe "Toggle" instead of "Expand or collapse" is a shorter, better alternative ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For screen readers "[Card title], button, collapsed" is good and we should avoid extra announced things if they are not needed.
Personally I think that since for visual users we have the title next to a chevron icon is enough to communicate the Do you think it's necessary to have the tooltip at all?
Assuming we want the tooltip, I think that's fine.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll drop "Expand/collapse" (or any other string in front of the card title). I'll do a bit more research into seeing if we can drop the tooltip, especially in the context of a separate The two PRs may affect each other and end up "merging" into one PR. |
||
| icon={ state.open ? chevronUp : chevronDown } | ||
| variant="minimal" | ||
| tone="neutral" | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.