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 8 commits into
base: master
Choose a base branch
from
Open

feat: accessibility improvements #363

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 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
5360802
chore: merge master
zombieJ Jun 20, 2025
dbad27a
prop fixes
jon-cullison Jun 27, 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
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@
"@babel/runtime": "^7.10.1",
"@rc-component/util": "^1.0.1",
"classnames": "2.x",
"rc-motion": "^2.3.4"
"rc-motion": "^2.3.4",
"uuid": "^11.1.0"
},
"devDependencies": {
"@rc-component/father-plugin": "^2.0.1",
Expand Down
8 changes: 8 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 { v4 as uuid4 } from 'uuid';

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

Choose a reason for hiding this comment

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

Currently we can simply use h3 instead of configuable headingLevel

id,
} = props;

const [collapseId] = React.useState<string>(id ?? uuid4());

const collapseClassName = classNames(prefixCls, className);

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

// ======================== Render ========================
Expand All @@ -87,6 +94,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) => {
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}
Copy link
Member

Choose a reason for hiding this comment

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

this should move into collapsibleProps

>
{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.

Copy link
Author

Choose a reason for hiding this comment

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

@zombieJ Can you please review my latest changes with my previous comment in mind?

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}
Copy link
Member

Choose a reason for hiding this comment

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

all the id will not be empty

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}
>
{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,
};

// 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