Skip to content

feat: accessibility improvements #363

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

feat: accessibility improvements #363

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c61fff
improves accessibility
jon-cullison Apr 3, 2025
9f789a2
adds default id
jon-cullison Apr 8, 2025
f8676e7
remove panel role prop, change header wrapper to div
jon-cullison Apr 10, 2025
089346c
remove uuid package
jon-cullison Apr 11, 2025
af2984e
use rc-util useId hook to handle id
jon-cullison May 7, 2025
55bc778
rename id to parentId in useItems hook, refactor heading element
jon-cullison May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions assets/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
border-top: none;
}

> .@{prefixCls}-header {
> .@{prefixCls}-header,
> .@{prefixCls}-header-wrapper > .@{prefixCls}-header {
display: flex;
align-items: center;
line-height: 22px;
Expand Down Expand Up @@ -76,10 +77,14 @@
}
}

& > &-item-disabled > .@{prefixCls}-header {
cursor: not-allowed;
color: #999;
background-color: #f3f3f3;

& > &-item-disabled {
> .@{prefixCls}-header,
> .@{prefixCls}-header-wrapper > .@{prefixCls}-header {
cursor: not-allowed;
color: #999;
background-color: #f3f3f3;
}
}

&-panel {
Expand All @@ -105,7 +110,8 @@
}

& > &-item-active {
> .@{prefixCls}-header {
> .@{prefixCls}-header,
> .@{prefixCls}-header-wrapper > .@{prefixCls}-header {
.arrow {
position: relative;
top: 2px;
Expand Down
7 changes: 7 additions & 0 deletions src/Collapse.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import useItems from './hooks/useItems';
import type { CollapseProps } from './interface';
import CollapsePanel from './Panel';
import pickAttrs from '@rc-component/util/lib/pickAttrs';
import useId from '@rc-component/util/lib/hooks/useId';

function getActiveKeysArray(activeKey: React.Key | React.Key[]) {
let currentActiveKey = activeKey;
Expand Down Expand Up @@ -34,8 +35,11 @@ const Collapse = React.forwardRef<HTMLDivElement, CollapseProps>((props, ref) =>
items,
classNames: customizeClassNames,
styles,
headingLevel,
id,
} = props;

const collapseId = useId(id);
const collapseClassName = classNames(prefixCls, className);

const [activeKey, setActiveKey] = useMergedState<React.Key | React.Key[], React.Key[]>([], {
Expand Down Expand Up @@ -77,6 +81,8 @@ const Collapse = React.forwardRef<HTMLDivElement, CollapseProps>((props, ref) =>
activeKey,
classNames: customizeClassNames,
styles,
headingLevel,
id: collapseId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not id but parentId

});

// ======================== Render ========================
Expand All @@ -87,6 +93,7 @@ const Collapse = React.forwardRef<HTMLDivElement, CollapseProps>((props, ref) =>
style={style}
role={accordion ? 'tablist' : undefined}
{...pickAttrs(props, { aria: true, data: true })}
id={collapseId}
>
{mergedChildren}
</div>
Expand Down
45 changes: 34 additions & 11 deletions src/Panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import classNames from 'classnames';
import CSSMotion from 'rc-motion';
import KeyCode from '@rc-component/util/lib/KeyCode';
import type { PropsWithChildren } from 'react';
import React from 'react';
import type { CollapsePanelProps } from './interface';
import PanelContent from './PanelContent';
Expand All @@ -25,6 +26,8 @@ const CollapsePanel = React.forwardRef<HTMLDivElement, CollapsePanelProps>((prop
openMotion,
destroyInactivePanel,
children,
headingLevel,
id,
...resetProps
} = props;

Expand Down Expand Up @@ -85,20 +88,38 @@ const CollapsePanel = React.forwardRef<HTMLDivElement, CollapsePanelProps>((prop
...(['header', 'icon'].includes(collapsible) ? {} : collapsibleProps),
};

const HeaderWrapper = ({ children: headerWrapperChildren }: PropsWithChildren) => {
Copy link
Member

@zombieJ zombieJ May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HeaderWrapper not satisfied with our semantic rule. You can just assign the heading role on the rc-collapse-title element instead of create new element.

https://github.com/react-component/collapse/pull/363/files#r2092418393

if (!headingLevel) {
return <>{headerWrapperChildren}</>;
} else {
return (
<div className={`${prefixCls}-header-wrapper`} role="heading" aria-level={headingLevel}>
{headerWrapperChildren}
</div>
);
}
};

// ======================== Render ========================
return (
<div {...resetProps} ref={ref} className={collapsePanelClassNames}>
<div {...headerProps}>
{showArrow && iconNode}
<span
className={classNames(`${prefixCls}-title`, customizeClassNames?.title)}
style={styles?.title}
{...(collapsible === 'header' ? collapsibleProps : {})}
<div {...resetProps} ref={ref} className={collapsePanelClassNames} id={id}>
<HeaderWrapper>
<div
{...headerProps}
id={id ? `${id}__header` : undefined}
aria-controls={id ? `${id}__content` : undefined}
>
{header}
</span>
{ifExtraExist && <div className={`${prefixCls}-extra`}>{extra}</div>}
</div>
{showArrow && iconNode}
<span
className={classNames(`${prefixCls}-title`, customizeClassNames?.title)}
Copy link
Member

@zombieJ zombieJ Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace this with HeadingElement instead of span directly. It's no need to have additional HeaderWrapper.

Copy link
Author

@jon-cullison jon-cullison Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change HeaderWrapper, but replacing the span would not satisfy the a11y standards for collapse / accordion components. The heading element should wrap the header button, which is where HeaderWrapper is in this PR. Would you prefer it to be a div that is always present around the button, instead of conditionally rendered, but it only would have the heading role and aria-level when the prop is defined?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. So you can add on the rc-collapse-header directly.

Copy link
Author

@jon-cullison jon-cullison May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update - the rc-collapse-header element gets the button role, so to satisfy the a11y standards for the header, I changed to a div element that always wraps rc-collapse-header and will conditionally have the heading role and aria-level props set.

style={styles?.title}
{...(collapsible === 'header' ? collapsibleProps : {})}
>
{header}
</span>
{ifExtraExist && <div className={`${prefixCls}-extra`}>{extra}</div>}
</div>
</HeaderWrapper>
<CSSMotion
visible={isActive}
leavedClassName={`${prefixCls}-panel-hidden`}
Expand All @@ -110,6 +131,8 @@ const CollapsePanel = React.forwardRef<HTMLDivElement, CollapsePanelProps>((prop
return (
<PanelContent
ref={motionRef}
id={id ? `${id}__content` : undefined}
aria-labelledby={id ? `${id}__header` : undefined}
prefixCls={prefixCls}
className={motionClassName}
classNames={customizeClassNames}
Expand Down
3 changes: 3 additions & 0 deletions src/PanelContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const PanelContent = React.forwardRef<
role,
classNames: customizeClassNames,
styles,
id,
} = props;

const [rendered, setRendered] = React.useState(isActive || forceRender);
Expand All @@ -33,6 +34,8 @@ const PanelContent = React.forwardRef<
return (
<div
ref={ref}
id={id}
aria-labelledby={props['aria-labelledby']}
className={classnames(
`${prefixCls}-panel`,
{
Expand Down
17 changes: 16 additions & 1 deletion src/hooks/useItems.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ import CollapsePanel from '../Panel';

type Props = Pick<
CollapsePanelProps,
'prefixCls' | 'onItemClick' | 'openMotion' | 'expandIcon' | 'classNames' | 'styles'
| 'prefixCls'
| 'onItemClick'
| 'openMotion'
| 'expandIcon'
| 'classNames'
| 'styles'
| 'headingLevel'
| 'id'
> &
Pick<CollapseProps, 'accordion' | 'collapsible' | 'destroyInactivePanel'> & {
activeKey: React.Key[];
Expand All @@ -23,6 +30,8 @@ const convertItemsToNodes = (items: ItemType[], props: Props) => {
expandIcon,
classNames: collapseClassNames,
styles,
headingLevel,
id,
} = props;

return items.map((item, index) => {
Expand Down Expand Up @@ -71,6 +80,8 @@ const convertItemsToNodes = (items: ItemType[], props: Props) => {
collapsible={mergeCollapsible}
onItemClick={handleItemClick}
destroyInactivePanel={mergeDestroyInactivePanel}
headingLevel={headingLevel}
id={id ? `${id}__item-${index}` : undefined}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use rc-util useId hooks in Panel. It support to use user pass id and fallback with ReactID if not have.

>
{children}
</CollapsePanel>
Expand Down Expand Up @@ -99,6 +110,8 @@ const getNewChild = (
expandIcon,
classNames: collapseClassNames,
styles,
headingLevel,
id,
} = props;

const key = child.key || String(index);
Expand Down Expand Up @@ -142,6 +155,8 @@ const getNewChild = (
onItemClick: handleItemClick,
expandIcon,
collapsible: mergeCollapsible,
headingLevel,
id: id ? `${id}__item-${index}` : undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use key instead of index

};

// https://github.com/ant-design/ant-design/issues/20479
Expand Down
4 changes: 4 additions & 0 deletions src/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { CSSMotionProps } from 'rc-motion';
import type * as React from 'react';

export type CollapsibleType = 'header' | 'icon' | 'disabled';
export type HeadingLevelType = 1 | 2 | 3 | 4 | 5 | 6;

export interface ItemType
extends Omit<
Expand Down Expand Up @@ -39,6 +40,8 @@ export interface CollapseProps {
items?: ItemType[];
classNames?: Partial<Record<SemanticName, string>>;
styles?: Partial<Record<SemanticName, React.CSSProperties>>;
headingLevel?: HeadingLevelType;
id?: string;
}

export type SemanticName = 'header' | 'title' | 'body' | 'icon';
Expand All @@ -64,4 +67,5 @@ export interface CollapsePanelProps extends React.DOMAttributes<HTMLDivElement>
role?: string;
collapsible?: CollapsibleType;
children?: React.ReactNode;
headingLevel?: HeadingLevelType;
}
13 changes: 13 additions & 0 deletions tests/__snapshots__/index.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@
exports[`collapse props items should work with nested 1`] = `
<div
class="rc-collapse"
id="collapse-test-id"
>
<div
class="rc-collapse-item rc-collapse-item-disabled"
id="collapse-test-id__item-0"
>
<div
aria-controls="collapse-test-id__item-0__content"
aria-disabled="true"
aria-expanded="false"
class="rc-collapse-header rc-collapse-collapsible-disabled"
id="collapse-test-id__item-0__header"
role="button"
tabindex="-1"
>
Expand All @@ -30,11 +34,14 @@ exports[`collapse props items should work with nested 1`] = `
</div>
<div
class="rc-collapse-item"
id="collapse-test-id__item-1"
>
<div
aria-controls="collapse-test-id__item-1__content"
aria-disabled="false"
aria-expanded="false"
class="rc-collapse-header"
id="collapse-test-id__item-1__header"
role="button"
tabindex="0"
>
Expand All @@ -61,11 +68,14 @@ exports[`collapse props items should work with nested 1`] = `
</div>
<div
class="rc-collapse-item important"
id="collapse-test-id__item-2"
>
<div
aria-controls="collapse-test-id__item-2__content"
aria-disabled="false"
aria-expanded="false"
class="rc-collapse-header"
id="collapse-test-id__item-2__header"
role="button"
tabindex="0"
>
Expand All @@ -85,11 +95,14 @@ exports[`collapse props items should work with nested 1`] = `
</div>
<div
class="rc-collapse-item"
id="collapse-test-id__item-3"
>
<div
aria-controls="collapse-test-id__item-3__content"
aria-disabled="false"
aria-expanded="false"
class="rc-collapse-header"
id="collapse-test-id__item-3__header"
role="button"
tabindex="0"
>
Expand Down
Loading
Loading