Skip to content

Commit 0cbdca8

Browse files
yinlianghuiclaude
andauthored
fix(components): 内建 select 分支把 host id/aria-* 落到真实 trigger,label for 不再悬空 (#3976) (#3992)
手写 form schema 里的 `{ name: 'status', label: 'Status', type: 'select', options: [...] }`, 可见的「Status」标签指向一个没有任何元素携带的 id。修前实测(probe,未提交): `label for="_r_1_-form-item"` 在文档里找不到宿主,`getByLabelText(/status/i)` 0 命中, trigger 按钮上没有 id、没有 aria-describedby、没有 aria-invalid,连 `required: true` 的 aria-required 也没有 —— 点标签什么都不会发生,屏幕阅读器读到的是一个匿名 combobox: 没有字段名、没有错误消息关联、没有必填状态。 成因是内建 select 分支把整个 DOM pass-through 展开到 Radix `Select.Root`。Root 自己不渲染 任何 DOM 元素,不认识的 prop 被静默丢弃 —— 而 `FormControl`(Radix Slot)下发的 id / aria-describedby / aria-invalid 与调用点下发的 aria-required 全都走这个展开点。 这正是 #3306 在 widget 侧修掉的同一机制(`SelectField` 把 pass-through 落到 `SelectTrigger`); 两条路径分叉是因为 `BUILTIN_FIELD_TYPES` 含 'select':对象驱动的 field:select 解析到注册 widget,而手写的裸 type: 'select' 根本不查注册表,内建分支的同形缺陷一直留着。 修法是抽出 `BuiltinSelectControl`。组件边界不是风格选择而是必要条件:Slot 注入的是分支 返回的那个元素,只有组件才能接住这些 prop 并重新落位(与同文件 `BuiltinSelectEmptyState` 成为组件是同一理由)。pass-through 落到 `SelectTrigger` 渲染的 button[role=combobox], 并保留 #3306 的两个白名单例外:`name` 留在 Root(Root 真正消费的唯一 key,转发给参与表单 提交的隐藏 select),`disabled` 留在 Root(单一权威,同时禁用 trigger / items / 隐藏 select)。 `ref` 随 pass-through 走,内建 select 不再是唯一一个 react-hook-form 无法聚焦的内建控件; 作者写的 className 现在也能到达 trigger(修前它到不了任何元素),与分支自己的触控高度用 cn() 合并而非互相覆盖。渲染哪个组件没有变,只变了 host props 的落点。 双路径各自钉住,互不干扰: - `packages/components/.../form-builtin-select-host-id.test.tsx`(12 条):内建路径的 for 解析到 trigger、可访问名、aria-required(必填有/可选无)、aria-invalid 修前 false 修后 true、aria-describedby 闭合到消息元素 id、name 仍只在 Root(trigger 上没有)、 disabled 仍由 Root 单一权威、只有 for 没有第二条 aria-labelledby 通道、两个 select 各自命名;外加分叉钉——注册表里确实存在 field:select 时裸 select 仍走内建分支—— 以及注册 widget 路径拿到 host props 的阳性对照。 - `packages/fields/.../select-label-association-e2e.test.tsx`(13 条):用真 `SelectField` 把 field:select 与裸 select 并排跑同一组保证(describe.each), 注册路径是必须保持绿的阳性对照,分叉钉断言 SelectField 的 trigger locator 不出现 在裸 select 的 DOM 里。 反向验证按预判成立(先写预判再跑,变异未提交):把 pass-through 还原展开到 `Select.Root` 后 13 红 12 绿,与预判逐条一致 —— 红的全是两个文件里内建路径的命名与状态钉,绿的是 name/disabled 留在 Root、两条分叉钉,以及整条 field:select 注册路径(#3306 那半从未 经过 Root)。 同文件的 group labelling 分支(PR #3978)未触碰,diff 不相交;内建 select 的 `resolveFieldLabelling` 仍是 'control',因为 trigger 是可 label 的 button,测试里把 「只有 for、没有 aria-labelledby」钉住了。 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 022002a commit 0cbdca8

4 files changed

Lines changed: 613 additions & 26 deletions

File tree

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
"@object-ui/components": patch
3+
---
4+
5+
Built-in `select` fields: the form's label, validation message and required state now reach the control
6+
7+
A hand-written form field `{ name: 'status', label: 'Status', type: 'select', options: [...] }`
8+
rendered a visible "Status" label that pointed at nothing. Measured before the fix:
9+
`<label for="…-form-item">` resolved to no element at all, `getByLabelText(/status/i)`
10+
found zero matches, and the trigger button carried no `id`, no `aria-describedby`, no
11+
`aria-invalid` and no `aria-required`. Clicking the label did nothing and a screen reader
12+
announced an anonymous combobox with no field name, no error link and no required state.
13+
14+
Cause: the built-in `select` branch spread its whole DOM pass-through onto Radix's
15+
`Select.Root`. Root renders no DOM element of its own, so everything it does not
16+
recognise is silently dropped — and that is exactly where `<FormControl>`'s Slot puts the
17+
field's `id` / `aria-describedby` / `aria-invalid` and the renderer puts `aria-required`.
18+
The pass-through now lands on `SelectTrigger`, the focusable `button[role=combobox]` the
19+
user and their assistive tech actually operate, with the same two keys deliberately kept
20+
on Root as in the widget-side fix: `name` (the one key Root consumes — it forwards it to
21+
the hidden native `select` that takes part in form submission) and `disabled` (Root
22+
disables trigger, items and hidden select together, so the raw prop must not gain a second
23+
author). `ref` rides the pass-through too, so react-hook-form can finally focus a built-in
24+
select.
25+
26+
This is the built-in half of the same mechanism objectui#3306 fixed on the widget side.
27+
The two halves diverged because `'select'` is a `BUILTIN_FIELD_TYPES` member: an
28+
object-driven `field:select` resolves to the registered `SelectField` (fixed since #3306),
29+
while a hand-written bare `type: 'select'` never consults the registry and kept the
30+
defect. Both paths are now pinned side by side — the registered path as a positive control
31+
— so they cannot drift apart again. Which component renders a bare `select` is unchanged;
32+
only where its host props land.
33+
34+
Authored `className` on a built-in `select` now reaches the trigger (it previously reached
35+
no element), composed with the branch's touch-target height rather than replacing it.
Lines changed: 272 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,272 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* The BUILT-IN `select` branch hands the host's control props to the control it
11+
* actually renders — objectui#3976.
12+
*
13+
* Radix's `Select.Root` renders no DOM element of its own, so every prop it does
14+
* not recognise is silently DROPPED instead of reaching an element. The branch
15+
* used to spread its whole DOM pass-through onto that Root, which is where
16+
* `<FormControl>`'s Slot puts the field's `id` / `aria-describedby` /
17+
* `aria-invalid` and the call site puts `aria-required`. Measured before the
18+
* fix, for a plain `{ name: 'status', label: 'Status', type: 'select' }`:
19+
*
20+
* label for="_r_1_-form-item" -> no element carries that id
21+
* getByLabelText(/status/i) -> 0 matches
22+
* trigger button -> no id, no aria-describedby,
23+
* no aria-invalid, no aria-required
24+
*
25+
* i.e. the visible "Status" label pointed at nothing (clicking it did nothing),
26+
* the field had no accessible name, and neither the validation message nor the
27+
* required state was announced.
28+
*
29+
* This is the same mechanism objectui#3306 fixed on the WIDGET side, where
30+
* `SelectField` now routes its pass-through to `SelectTrigger`. The two `select`
31+
* paths diverge because `BUILTIN_FIELD_TYPES` contains `'select'`: a bare
32+
* `type: 'select'` renders the built-in branch and never consults the registry,
33+
* while an object-driven `field:select` (`mapFieldTypeToFormType`) resolves to
34+
* the widget. Only the built-in half was broken, which is why the fork itself is
35+
* pinned below — both halves have to keep working independently, and a "fix"
36+
* that quietly rerouted bare `select` to the registry would change which
37+
* component renders for every hand-written form schema in existence.
38+
*
39+
* Reverse verification (run before writing the fix, direction predicted first):
40+
* restoring the spread onto `<Select>` turns the built-in describe RED — the
41+
* label-resolution, accessible-name, aria-required, aria-invalid and
42+
* aria-describedby cases all fail — while the registry describe stays GREEN,
43+
* because that path never went through Root.
44+
*/
45+
46+
import { describe, it, expect, beforeAll, afterEach, vi } from 'vitest';
47+
import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react';
48+
import { ComponentRegistry } from '@object-ui/core';
49+
// Module scope, not `beforeAll` — the cold transform must not be billed to
50+
// `hookTimeout`. See object-ui/no-dynamic-import-in-test-hook (objectui#3010).
51+
import '../../../renderers';
52+
53+
const OPTIONS = [
54+
{ label: 'Open', value: 'open' },
55+
{ label: 'Closed', value: 'closed' },
56+
];
57+
58+
/**
59+
* Stands in for a registered option widget (`@object-ui/components` tests never
60+
* load `@object-ui/fields`) and mirrors the one mechanism the registry path
61+
* relies on: spread the leftover props onto the control it renders. Registered
62+
* under the REAL `field:select` key so the fork below is measured, not assumed —
63+
* the issue's finding was that the built-in branch wins even when that key
64+
* resolves.
65+
*/
66+
function SelectWidgetProbe(props: any) {
67+
const { name, value: _value, onChange: _onChange, field: _field, error: _error, options: _options, ...rest } = props;
68+
return (
69+
<button type="button" role="combobox" data-testid={`probe-${name}`} {...rest} />
70+
);
71+
}
72+
73+
beforeAll(() => {
74+
ComponentRegistry.register('select', SelectWidgetProbe, { namespace: 'field' });
75+
}, 30000);
76+
77+
afterEach(() => {
78+
cleanup();
79+
vi.restoreAllMocks();
80+
});
81+
82+
function renderForm(fields: any[], defaultValues: Record<string, unknown> = {}) {
83+
const Form = ComponentRegistry.get('form')!;
84+
return render(
85+
<Form
86+
schema={{
87+
type: 'form',
88+
mode: 'create',
89+
showSubmit: true,
90+
showCancel: false,
91+
submitLabel: 'Create',
92+
defaultValues,
93+
fields,
94+
onSubmit: () => {},
95+
}}
96+
/>,
97+
);
98+
}
99+
100+
const STATUS_FIELD = { name: 'status', label: 'Status', type: 'select', options: OPTIONS };
101+
102+
/** The trigger button, by role — fails loudly if it stops being a combobox. */
103+
function triggerOf(name: string): Element {
104+
const el = document.querySelector(`[data-field="${name}"] [role="combobox"]`);
105+
if (!el) throw new Error(`no select trigger rendered for field "${name}"`);
106+
return el;
107+
}
108+
109+
/** Every `<label for=…>` in the document, paired with its resolved target. */
110+
function labelTargets(): Array<{ text: string; forId: string; resolves: boolean }> {
111+
return Array.from(document.querySelectorAll('label'))
112+
.map((el) => ({
113+
text: (el.textContent ?? '').trim(),
114+
forId: el.getAttribute('for') ?? '',
115+
resolves: !!el.getAttribute('for') && !!document.getElementById(el.getAttribute('for')!),
116+
}))
117+
.filter((l) => l.forId !== '');
118+
}
119+
120+
const submit = () => fireEvent.click(screen.getByRole('button', { name: /create/i }));
121+
122+
describe('built-in select — the host id reaches the trigger (objectui#3976)', () => {
123+
it('emits no label pointing at an id nothing carries', () => {
124+
renderForm([STATUS_FIELD], { status: undefined });
125+
126+
const labels = labelTargets();
127+
expect(labels.length).toBeGreaterThan(0);
128+
// The defect verbatim: `for="_r_N_-form-item"` with no owner in the document.
129+
expect(labels.filter((l) => !l.resolves)).toEqual([]);
130+
});
131+
132+
it('resolves the visible label to the trigger, not to a wrapper', () => {
133+
renderForm([STATUS_FIELD], { status: undefined });
134+
135+
// `getAllByLabelText` walks `for`→id, so it guards the association itself:
136+
// before the fix it returned 0 matches (the id had no owner at all).
137+
const labelled = screen.getAllByLabelText('Status');
138+
expect(labelled).toHaveLength(1);
139+
expect(labelled[0]).toBe(triggerOf('status'));
140+
// …and the element carrying the id is the focusable control, not the row.
141+
expect(labelled[0].tagName).toBe('BUTTON');
142+
});
143+
144+
it('gives the combobox the label as its accessible name', () => {
145+
renderForm([STATUS_FIELD], { status: undefined });
146+
147+
// The user-visible consequence, asserted on the computed name rather than on
148+
// markup: pre-fix the control was reachable by role but ANONYMOUS, so a
149+
// screen reader announced "combobox" with no field name.
150+
const combobox = screen.getByRole('combobox', { name: 'Status' });
151+
expect(combobox).toHaveAccessibleName('Status');
152+
expect(combobox).toBe(triggerOf('status'));
153+
});
154+
155+
it('keeps `for` as the association channel — no second naming channel', () => {
156+
// The built-in `select` renders a labelable `<button>`, so `resolveFieldLabelling`
157+
// correctly classifies it as `'control'` (objectui#3961/#3978): plain `for`,
158+
// and NO `aria-labelledby`. Two channels for one label is the failure the
159+
// group-labelling work exists to avoid, and this fix must not introduce it.
160+
renderForm([STATUS_FIELD], { status: undefined });
161+
162+
expect(screen.getByText('Status').getAttribute('for')).toBe(
163+
triggerOf('status').getAttribute('id'),
164+
);
165+
expect(triggerOf('status')).not.toHaveAttribute('aria-labelledby');
166+
});
167+
168+
it('carries aria-required on the trigger, and nothing when optional', () => {
169+
renderForm([{ ...STATUS_FIELD, required: true }], { status: undefined });
170+
expect(triggerOf('status')).toHaveAttribute('aria-required', 'true');
171+
172+
cleanup();
173+
renderForm([STATUS_FIELD], { status: undefined });
174+
expect(triggerOf('status')).not.toHaveAttribute('aria-required');
175+
});
176+
177+
it('announces invalid on the trigger only AFTER validation fails', async () => {
178+
renderForm([{ ...STATUS_FIELD, required: true }], { status: undefined });
179+
180+
// The explicit "false" half is load-bearing (#3222/#3306): a valid field
181+
// SAYS it is valid rather than staying mute.
182+
expect(triggerOf('status')).toHaveAttribute('aria-invalid', 'false');
183+
184+
submit();
185+
186+
// Prove the failure actually rendered before reading aria off it.
187+
await waitFor(() => expect(screen.getAllByText('Status is required')).toHaveLength(1));
188+
expect(triggerOf('status')).toHaveAttribute('aria-invalid', 'true');
189+
});
190+
191+
it('links aria-describedby to the rendered error message', async () => {
192+
renderForm([{ ...STATUS_FIELD, required: true }], { status: undefined });
193+
194+
submit();
195+
196+
const message = await screen.findByText('Status is required');
197+
// The association is only real if the id chain closes: the trigger must
198+
// point at the exact element carrying the message text.
199+
expect(message.id).not.toBe('');
200+
const describedBy = triggerOf('status').getAttribute('aria-describedby') ?? '';
201+
expect(describedBy.split(/\s+/)).toContain(message.id);
202+
});
203+
204+
it('leaves `name` on Root, where the submitted hidden select is', () => {
205+
// The whitelist half of the fix, and a regression fence against "move
206+
// everything to the trigger": `name` is the ONE key Root genuinely consumes
207+
// — it forwards it to the hidden native `<select>` that takes part in form
208+
// submission. On the trigger it would sit uselessly on a non-submitter
209+
// `<button>`.
210+
renderForm([STATUS_FIELD], { status: undefined });
211+
212+
const hidden = document.querySelector('[data-field="status"] select[aria-hidden="true"]');
213+
expect(hidden).toHaveAttribute('name', 'status');
214+
expect(triggerOf('status')).not.toHaveAttribute('name');
215+
});
216+
217+
it('still keeps `disabled` a single-authority prop on Root', () => {
218+
// Root disables the trigger, the items and the hidden select together, so
219+
// the raw prop must not gain a second author on the trigger.
220+
renderForm([{ ...STATUS_FIELD, disabled: true }], { status: undefined });
221+
222+
expect(triggerOf('status')).toBeDisabled();
223+
});
224+
225+
it('two selects in one form each name their own trigger', () => {
226+
renderForm(
227+
[STATUS_FIELD, { name: 'stage', label: 'Stage', type: 'select', options: OPTIONS }],
228+
{ status: undefined, stage: undefined },
229+
);
230+
231+
expect(labelTargets().filter((l) => !l.resolves)).toEqual([]);
232+
// A shared/colliding id would make both labels resolve to the same control.
233+
expect(screen.getByRole('combobox', { name: 'Status' })).toBe(triggerOf('status'));
234+
expect(screen.getByRole('combobox', { name: 'Stage' })).toBe(triggerOf('stage'));
235+
});
236+
});
237+
238+
describe('the two select paths stay separate (objectui#3976 / #3231)', () => {
239+
it('a bare `type: select` renders the BUILT-IN branch even though field:select resolves', () => {
240+
// Measured, not inferred: `renderFieldComponent` skips the registry entirely
241+
// for a `BUILTIN_FIELD_TYPES` member, so the probe registered above under
242+
// the real `field:select` key does NOT win. That is why #3976 had to be
243+
// fixed in the built-in branch — rerouting bare `select` to the registry is
244+
// a design change (it swaps the rendered component for every hand-written
245+
// form schema), not a bug fix.
246+
expect(ComponentRegistry.get('field:select')).toBe(SelectWidgetProbe);
247+
248+
renderForm([STATUS_FIELD], { status: undefined });
249+
250+
expect(screen.queryByTestId('probe-status')).toBeNull();
251+
// The Radix trigger + its hidden native select — the built-in branch's DOM.
252+
expect(triggerOf('status')).toHaveAttribute('aria-autocomplete', 'none');
253+
expect(document.querySelector('[data-field="status"] select[aria-hidden="true"]')).not.toBeNull();
254+
});
255+
256+
it('a registered `field:select` widget still receives the host props (positive control)', () => {
257+
// The path #3306 already fixed. It must be untouched by this change: the
258+
// widget gets the id / aria-* as PROPS (the Slot clones the element it
259+
// returns) and puts them on the control it renders.
260+
renderForm(
261+
[{ name: 'choice', label: 'Choice', type: 'field:select', required: true, options: OPTIONS }],
262+
{ choice: undefined },
263+
);
264+
265+
const probe = screen.getByTestId('probe-choice');
266+
expect(labelTargets().filter((l) => !l.resolves)).toEqual([]);
267+
expect(screen.getByRole('combobox', { name: 'Choice' })).toBe(probe);
268+
expect(probe).toHaveAttribute('aria-required', 'true');
269+
expect(probe).toHaveAttribute('aria-invalid', 'false');
270+
expect(probe.getAttribute('id')).toMatch(/-form-item$/);
271+
});
272+
});

0 commit comments

Comments
 (0)