Skip to content

fix: prevent aria props from being passed to wrapper div #1190

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
Pareder wants to merge 1 commit into
base: master
Choose a base branch
from
+38 −8
Open

fix: prevent aria props from being passed to wrapper div #1190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion src/BaseSelect/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,11 @@ export interface BaseSelectPrivateProps {

export type BaseSelectPropsWithoutPrivate = Omit<BaseSelectProps, keyof BaseSelectPrivateProps>;

export interface BaseSelectProps extends BaseSelectPrivateProps, React.AriaAttributes {
export interface BaseSelectProps
extends
BaseSelectPrivateProps,
React.AriaAttributes,
Pick<React.HTMLAttributes<HTMLElement>, 'role'> {
// Style
className?: string;
style?: React.CSSProperties;
Expand Down
5 changes: 4 additions & 1 deletion src/SelectInput/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { clsx } from 'clsx';
import type { ComponentsConfig } from '../hooks/useComponents';
import { getDOM } from '@rc-component/util/lib/Dom/findDOMNode';
import { composeRef } from '@rc-component/util/lib/ref';
import pickAttrs from '@rc-component/util/lib/pickAttrs';

export interface SelectInputRef {
focus: (options?: FocusOptions) => void;
Expand Down Expand Up @@ -207,6 +208,8 @@ export default React.forwardRef<SelectInputRef, SelectInputProps>(function Selec

// ===================== Render =====================
const domProps = omit(restProps, DEFAULT_OMIT_PROPS as any);
const ariaProps = pickAttrs(domProps, { aria: true });
Copy link

@gemini-code-assist gemini-code-assist bot Dec 24, 2025

Choose a reason for hiding this comment

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

high

The pickAttrs utility with the { aria: true } option doesn't include the role attribute. This means role will not be added to ariaKeys and will not be omitted from the wrapper div, which contradicts the goal of this PR and the associated tests. To ensure role is also filtered out, you should include role: true in the pickAttrs options.

Suggested change
const ariaProps = pickAttrs(domProps, { aria: true });
const ariaProps = pickAttrs(domProps, { aria: true, role: true });

const ariaKeys = Object.keys(ariaProps) as (keyof typeof domProps)[];

// Create context value with wrapped callbacks
const contextValue = {
Expand All @@ -228,7 +231,7 @@ export default React.forwardRef<SelectInputRef, SelectInputProps>(function Selec
return (
<SelectInputContext.Provider value={contextValue}>
<div
{...domProps}
{...omit(domProps, ariaKeys)}
// Style
ref={rootRef}
className={className}
Expand Down
1 change: 0 additions & 1 deletion src/hooks/useBaseProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export interface BaseSelectContextProps extends BaseSelectProps {
triggerOpen: boolean;
multiple: boolean;
toggleOpen: (open?: boolean) => void;
role?: React.AriaRole;
}

export const BaseSelectContext = React.createContext<BaseSelectContextProps>(null);
Expand Down
28 changes: 28 additions & 0 deletions tests/Accessibility.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,34 @@ describe('Select.Accessibility', () => {
}
});
});

it('should pass aria and role attributes to Input component', async () => {
const { container } = render(
<Select
role="group"
aria-required="true"
options={[
{
value: '123',
},
{
value: '1234',
},
{
value: '12345',
},
]}
/>,
);

const wrapper = container.querySelector('.rc-select');
expect(wrapper).not.toHaveAttribute('role');
expect(wrapper).not.toHaveAttribute('aria-required');

const input = container.querySelector('input');
expect(input).toHaveAttribute('role', 'group');
expect(input).toHaveAttribute('aria-required', 'true');
});
Comment on lines +217 to +243
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 24, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

WAI-ARIA Select component appropriate role combobox listbox

💡 Result:

Short answer

  • Use role="combobox" on the control when you have a single control that opens a popup for choosing a value (editable or select‑only). The popup element should use role="listbox" (or tree/grid/dialog if appropriate) and each item uses role="option". Manage aria-expanded, aria-controls, aria-activedescendant, and aria-autocomplete as required. [1][2]

When to use listbox without combobox

  • If the UI is simply a persistent list of options (not a single control that opens/closes a popup), use role="listbox" for the list and role="option" for items — no combobox required. [3]

Key ARIA requirements (summary)

  • combobox element: role="combobox", aria-expanded (true/false), aria-controls → id of popup, aria-haspopup defaults to "listbox". [1][2]
  • popup: role="listbox" and children role="option"; when focus remains on the combobox use aria-activedescendant to indicate the highlighted option. [1][2]
  • If editable/autocomplete, set aria-autocomplete on the combobox/textbox. [1]

References

  • WAI-ARIA 1.2 (combobox role & structural rules). [1]
  • WAI-ARIA Authoring Practices — Combobox pattern & examples. [2]
  • MDN — listbox role guidance. [3]

🏁 Script executed:

# Find and examine the Select component implementation around line 265
find . -type f -name "*.tsx" -o -name "*.ts" | head -20

Repository: react-component/select

Length of output: 716

🏁 Script executed:

# Search for the Select component implementation to understand default role
rg -t tsx -t ts "role.*combobox|role.*group" --context 3

Repository: react-component/select

Length of output: 93

🏁 Script executed:

# Look for the test file to understand the full context
find . -path "*/tests/*Accessibility*" -type f

Repository: react-component/select

Length of output: 96

🏁 Script executed:

# Examine the Accessibility test file around line 265
cat -n ./tests/Accessibility.test.tsx | sed -n '260,280p'

Repository: react-component/select

Length of output: 936

🏁 Script executed:

# Search for Select component implementation and default role
rg "role.*=.*combobox|role.*=.*group" --context 2

Repository: react-component/select

Length of output: 3100

🏁 Script executed:

# Look for the main Select component file
find . -type f -name "*.tsx" -o -name "*.ts" | grep -i select | head -10

Repository: react-component/select

Length of output: 392

🏁 Script executed:

# Search for role attribute assignments in the codebase
rg "role\s*=" --context 2 | head -50

Repository: react-component/select

Length of output: 2732

测试验证了属性传播机制，但使用的 ARIA role 不符合无障碍标准

新测试用例正确验证了属性在 Select 组件中的传播行为：包装器 div 不接收 role 或 aria-required，而内部 input 元素接收这些属性。

但根据 WAI-ARIA 规范，Select 组件的 input 元素应使用 role="combobox"，而非 role="group"。role="group" 通常用于分组相关元素，并不适合 Select 组件（见 line 265 的默认实现）。建议使用 role="combobox" 重写该测试，以遵循无障碍最佳实践。

🤖 Prompt for AI Agents 
In tests/Accessibility.test.tsx around lines 217 to 243, the test uses
role="group" which is incorrect for a Select input; update the test to pass
role="combobox" into the Select props and change the assertions to expect the
internal input to have role "combobox" (while still asserting the wrapper does
not have role or aria-required), so the test aligns with the component's default
ARIA role and accessibility expectations.

});

describe('Select Input attributes', () => {
Expand Down
3 changes: 1 addition & 2 deletions tests/Select.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,11 @@ describe('Select.Basic', () => {
expect(container.firstChild).toMatchSnapshot();
});

// [Legacy] Should not use `role` since it's meaningless
it('renders role prop correctly', () => {
const { container } = render(
genSelect({
role: 'button',
} as any),
}),
);
expect(container.firstChild).toMatchSnapshot();
});
Expand Down
3 changes: 0 additions & 3 deletions tests/__snapshots__/Select.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ exports[`Select.Basic filterOption could be true as described in default value 1

exports[`Select.Basic render renders aria-attributes correctly 1`] = `
<div
aria-label="some-label"
aria-labelledby="test-id"
class="antd select-test antd-single antd-allow-clear antd-show-search"
>
<div
Expand Down Expand Up @@ -249,7 +247,6 @@ exports[`Select.Basic render renders dropdown correctly 1`] = `null`;
exports[`Select.Basic render renders role prop correctly 1`] = `
<div
class="antd select-test antd-single antd-allow-clear antd-show-search"
role="button"
>
<div
class="antd-content"
Expand Down
Loading