Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 31 additions & 0 deletions .changeset/multiselect-group-label-3975.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
---
'@object-ui/fields': patch
---

A form-hosted `multiselect` field is now NAMED by its visible label. It was the
residual of objectui#3961: that issue's probe audited six widgets and fixed
them, and re-running the same probe over the whole widget map afterwards put
`multiselect` on the byte-identical failure shape as `checkboxes` — the host's
`id` kept, but on the chip row's wrapper `div`, where a `<label for>` is inert
HTML (`HTMLLabelElement.control` is `null`, so it activates nothing and
contributes no accessible name). Measured on the tree that already carried
#3961's fix, one field per row:

```
checkboxes for=(none) ownId=…-group-label byLabelText=1(div[role=group])
multiselect for=…-form-item ownId=(none) byLabelText=0
```

Visually the field had a "Tags" label; a screen reader heard only "Alpha" /
"Beta" and nothing about what the set of chips was for.

No new mechanism — #3961's declaration, applied to one more widget:
`multiselect` declares `labelling: 'group'` at the registration boundary, so
the form renderer publishes its label's `id` and drops the dead `for`, and the
chip row answers with `role="group"` + the handed-down `aria-labelledby`.

Unchanged on purpose: each chip keeps its own accessible name from its text
content (the group name sits one level up and does not override it), and
STANDALONE rendering — the grid's inline cell editor, a bare SDUI node, where
nobody hands an id and there is no host label to point at — emits no role and
no IDREF, exactly as before.
48 changes: 46 additions & 2 deletions packages/fields/src/__tests__/composite-group-label-e2e.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import { CheckboxesField } from '../widgets/CheckboxesField';
import { RadioField } from '../widgets/RadioField';
import { RatingField } from '../widgets/RatingField';
import { FileField } from '../widgets/FileField';
import { MultiSelectField } from '../widgets/MultiSelectField';
import { TextField } from '../widgets/TextField';

/**
Expand All @@ -81,6 +82,17 @@ import { TextField } from '../widgets/TextField';
* file input). It needs IDREF labelling because a `div` cannot be
* `for`-labelled, not because it is a group, so no `role="group"` wrapper is
* invented for it.
* - `multiselect` is `group`, and arrived with objectui#3975 rather than #3961:
* #3961's probe covered the six above, and re-running it over the whole widget
* map afterwards put `multiselect` on the byte-identical `checkboxes` shape —
* host id kept, on the chip row's wrapper `div`, `for` inert. Measured on the
* tree that already had #3961's fix, i.e. this is its residual, not a
* regression of it:
*
* ```
* checkboxes for=(none) ownId=…-group-label byLabelText=1(div[role=group]) ← #3961
* multiselect for=…-form-item ownId=(none) byLabelText=0 ← #3975
* ```
*/
const HOSTED: Array<[type: string, role: string]> = [
['address', 'group'],
Expand All @@ -89,6 +101,7 @@ const HOSTED: Array<[type: string, role: string]> = [
['radio', 'radiogroup'],
['rating', 'group'],
['file', 'button'],
['multiselect', 'group'],
];

const OPTIONS = [
Expand All @@ -103,6 +116,7 @@ const WIDGETS: Record<string, any> = {
radio: RadioField,
rating: RatingField,
file: FileField,
multiselect: MultiSelectField,
};

beforeAll(() => {
Expand Down Expand Up @@ -155,7 +169,7 @@ function labelTargets(): Array<{ text: string; forId: string; resolves: boolean;

function fieldConfig(type: string) {
const config: any = { name: `f_${type}`, label: `Group Label ${type}`, type };
if (type === 'checkboxes' || type === 'radio') config.options = OPTIONS;
if (type === 'checkboxes' || type === 'radio' || type === 'multiselect') config.options = OPTIONS;
return config;
}

Expand Down Expand Up @@ -225,7 +239,7 @@ describe('a form-hosted composite field is NAMED by its host label (objectui#396
expect(labelTargets().filter((l) => !l.labelable)).toEqual([]);
});

it('all six in ONE form: every group is named, every `for` still resolves', () => {
it('all of them in ONE form: every group is named, every `for` still resolves', () => {
renderForm(HOSTED.map(([type]) => fieldConfig(type)));

for (const [type, role] of HOSTED) {
Expand Down Expand Up @@ -268,6 +282,21 @@ describe('the group name does not swallow the sub-controls\' own names (objectui
expect(screen.getByLabelText('Alpha')).toHaveAttribute('role', 'checkbox');
expect(screen.getByLabelText('Beta')).toHaveAttribute('role', 'checkbox');
});

it('multiselect chips keep their own names (objectui#3975)', () => {
// The chips name themselves from their text CONTENT, not from a sub-label,
// so the group's `aria-labelledby` sits one level up and must not reach them.
// `aria-labelledby` on a chip would REPLACE "Alpha" with the group name —
// the same override this file refuses for `address`'s street box.
renderForm([fieldConfig('multiselect')]);

const alpha = screen.getByRole('button', { name: 'Alpha' });
expect(alpha).toHaveAccessibleName('Alpha');
expect(alpha).not.toHaveAttribute('aria-labelledby');
expect(screen.getByRole('button', { name: 'Beta' })).toBeTruthy();
// The group is the CONTAINER, not one of the chips.
expect(screen.getByRole('group', { name: 'Group Label multiselect' })).toBe(alpha.parentElement);
});
});

describe('an undeclared single-control field keeps the plain `for` association (objectui#3961)', () => {
Expand Down Expand Up @@ -348,6 +377,21 @@ describe('STANDALONE composite widgets are unchanged (objectui#3961)', () => {
expect(screen.getByTestId('checkboxes-tags')).not.toHaveAttribute('role');
});

it('multiselect: the chip row stays a plain div (objectui#3975)', () => {
render(
<MultiSelectField
value={[]}
onChange={() => {}}
field={{ name: 'tags', label: 'Tags', type: 'multiselect', options: OPTIONS } as any}
/>,
);

expect(screen.queryAllByRole('group')).toHaveLength(0);
expect(screen.getByTestId('multiselect-tags')).not.toHaveAttribute('role');
// The chips are still reachable by their own names with no group around them.
expect(screen.getByRole('button', { name: 'Alpha' })).toBeTruthy();
});

it('rating: the container stays a plain div', () => {
const { container } = render(
<RatingField
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,21 @@ import { registerAllFields } from '../index';
* Every field type whose host label must be associated by IDREF. Two shapes, one
* declaration — see `FIELD_TYPES_GROUP_LABELLED` in `../index`:
* real composites, plus `file`, whose single control is a `div[role="button"]`.
*
* `multiselect` (objectui#3975) is the seventh, added after #3961 shipped: its
* chip row is the same wrapper `div` holding the host id that `checkboxes` had.
* It is listed here rather than left to the e2e file alone because the omission
* of a declaration is exactly the failure that degrades silently.
*/
const GROUP_LABELLED = ['address', 'geolocation', 'checkboxes', 'radio', 'rating', 'file'] as const;
const GROUP_LABELLED = [
'address',
'geolocation',
'checkboxes',
'radio',
'rating',
'file',
'multiselect',
] as const;

/**
* Single-control widgets: the host's `<label for>` reaches a real labelable
Expand Down
19 changes: 14 additions & 5 deletions packages/fields/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2234,8 +2234,9 @@ const FIELD_TYPES_SKIP_FALLBACK = new Set([
* (`ComponentMeta.labelling`, objectui#3961). Two shapes, one declaration:
*
* - real composites — `address` / `geolocation` render several inputs under one
* container, and `checkboxes` / `radio` / `rating` a set of choice controls;
* the host's label names the GROUP, each sub-control keeps its own sub-label.
* container, and `checkboxes` / `radio` / `rating` / `multiselect` a set of
* choice controls; the host's label names the GROUP, each sub-control keeps
* its own name (a sub-label, or the chip's own text content).
* - `file` is not composite at all: it has exactly ONE control, the dropzone,
* which is a `div[role="button"]` (it is the keyboard path to the hidden file
* input). It is here because a `div` cannot be `for`-labelled, not because it
Expand All @@ -2245,9 +2246,15 @@ const FIELD_TYPES_SKIP_FALLBACK = new Set([
* Measured, not assumed: every entry was verified in a real form to be a widget
* whose host label either resolved to nothing (`address` / `geolocation`, whose
* sub-input ids overwrote the host id — objectui#3343) or resolved to an element
* that cannot carry it (`checkboxes` / `radio` / `rating` / `file`). In both
* shapes the visible group label was, before this declaration, the accessible
* name of NOTHING.
* that cannot carry it (`checkboxes` / `radio` / `rating` / `file` /
* `multiselect`). In both shapes the visible group label was, before this
* declaration, the accessible name of NOTHING.
*
* `multiselect` arrived one issue later (objectui#3975) for a coverage reason,
* not a mechanism one: #3961's probe covered the six above, and re-running it
* over the full widget map afterwards found `multiselect` on the byte-identical
* failure shape as `checkboxes` — the host id kept, on the chip row's wrapper
* `div`, where a `for` is inert. Same declaration, same container answer.
*
* A widget NOT listed here takes the single-control path unchanged. That is the
* safe default: the host keeps emitting `for`, and a composite that forgot to
Expand All @@ -2261,6 +2268,8 @@ const FIELD_TYPES_GROUP_LABELLED = new Set([
'radio',
'rating',
'file',
// objectui#3975 — the residual after #3961's six, same shape as `checkboxes`.
'multiselect',
]);

export function registerField(fieldType: string): void {
Expand Down
10 changes: 10 additions & 0 deletions packages/fields/src/widgets/MultiSelectField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,20 @@ export function MultiSelectField({
name: _domName,
...groupDomProps
} = toDomProps(props);
// When the host associated its visible label with this container by IDREF
// (`aria-labelledby`, objectui#3961 / #3975) the container IS the labelled
// group, so it answers with the matching role — without one, the name sits on
// a generic `div` and contributes nothing: `label for` pointing here was
// already inert (`HTMLLabelElement.control` is null for a div), which is the
// whole defect. Each chip keeps its own accessible name from its text content,
// which the group name does not override. Absent (standalone: the inline grid
// editor, a bare SDUI node) nothing is emitted and the markup is unchanged.
const isLabelledGroup = groupDomProps['aria-labelledby'] != null;

return (
<div
{...groupDomProps}
role={isLabelledGroup ? 'group' : undefined}
className={cn('flex flex-wrap gap-1.5', className)}
data-testid={fieldName ? `multiselect-${fieldName}` : undefined}
>
Expand Down
Loading