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
15 changes: 15 additions & 0 deletions .changeset/aria-required-five-more-sites.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
'@object-ui/components': patch
'@object-ui/app-shell': patch
'@object-ui/plugin-detail': patch
---

Deliver the required state to the control in the five renderers outside the object form that still painted it as an asterisk only (objectui#3299 — the same defect #3290/#3298 fixed in `form.tsx`).

Each site converges on the reference shape (`EmbeddableForm.tsx`): the control carries `aria-required={required || undefined}` and the asterisk is `aria-hidden="true"`, so assistive tech announces required once, as a state — instead of hearing a bare "asterisk" folded into the accessible name, or nothing at all.

- `@object-ui/app-shell` — `ActionParamDialog` (both the boolean row and the default branch, delivered through the real field widgets' `toDomProps` whitelist) and `CreateViewDialog` (display label, machine name, and every type-specific required-field selector).
- `@object-ui/components` — the custom `ActionParamDialog` (all five typed branches, including the Radix select trigger) and `FieldContainer`, whose existing Slot injection (`id` / `aria-describedby` / `aria-invalid`) now also injects `aria-required`, covering every consumer in one place.
- `@object-ui/plugin-detail` — `InlineCreateRelated`'s create-tab inputs.

Deliberately NOT the native `required` attribute (#3290 ruling): each of these hosts runs its own validation, and native `required` would arm the browser's constraint-validation bubble beside it. The SDUI controls that already use native `required` (`renderers/form/{input,textarea,select,checkbox}.tsx`, `basic/text-input.tsx`) are unchanged — they don't have a second validator, so their channel is already correct.
101 changes: 101 additions & 0 deletions packages/app-shell/src/views/ActionParamDialog.ariaRequired.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* ActionParamDialog (app-shell) — required must reach the field WIDGET's
* control as a state, not sit in the label as a bare `*` (objectui#3299; same
* shape as #3290/#3298).
*
* This dialog routes params through the real `@object-ui/fields` widgets
* (ADR-0059), so the state travels host → widget props → `toDomProps` (whose
* whitelist forwards `aria-*` by prefix, objectui#3291) → the rendered
* control. These tests drive that FULL delivery chain with real widgets — a
* widget-side strip of `aria-*` would fail here, not just a host-side
* omission.
*
* Deliberately NOT native `required` (#3290 ruling): the dialog runs its own
* required validation (`requiredError` messages); native required would arm
* the browser's constraint-validation bubble beside it.
*/

import { describe, it, expect, vi } from 'vitest';
import { render, screen, waitFor } from '@testing-library/react';
import type { ActionParamDef } from '@object-ui/core';
import { ActionParamDialog } from './ActionParamDialog';

/** Mount the dialog open with the given params (mirrors ActionParamDialog.test.tsx). */
function openDialog(params: ActionParamDef[]) {
const resolve = vi.fn();
render(
<ActionParamDialog
state={{ open: true, params, resolve }}
onOpenChange={() => {}}
/>,
);
return resolve;
}

const def = (over: Partial<ActionParamDef>): ActionParamDef => ({
name: 'p1',
label: 'Param One',
type: 'text',
...over,
});

describe('app-shell ActionParamDialog — `aria-required` reaches the widget control (objectui#3299)', () => {
it('sets aria-required="true" on a required text param, delivered through the real widget', async () => {
openDialog([def({ name: 'note', type: 'text', required: true })]);

const input = await screen.findByLabelText(/Param One/);
expect(input).toHaveAttribute('aria-required', 'true');
});

it('omits the attribute entirely on an optional param, rather than writing "false"', async () => {
openDialog([def({ name: 'note', type: 'text' })]);

const input = await screen.findByLabelText(/Param One/);
expect(input).not.toHaveAttribute('aria-required');
});

it('sets aria-required="true" on the boolean branch too (checkbox row)', async () => {
// The boolean branch is a SEPARATE render path (inline checkbox row) —
// fixing only the default branch would leave this one silent.
openDialog([def({ name: 'force', type: 'boolean', required: true })]);

const checkbox = await screen.findByRole('checkbox');
expect(checkbox).toHaveAttribute('aria-required', 'true');
});

it('keeps the asterisk OUT of the accessible name — state announced once, not "asterisk"', async () => {
// Pre-fix the bare `*` inside `<Label htmlFor>` folded into the control's
// accessible name ("Param One asterisk"). Only the computed name shows
// that regression, so that is what gets pinned.
openDialog([def({ name: 'note', type: 'text', required: true })]);

const input = await screen.findByLabelText(/Param One/);
expect(input).toHaveAccessibleName('Param One');

const label = document.querySelector('label[for="note"]');
expect(label).not.toBeNull();
const marker = label!.querySelector('span');
expect(marker).not.toBeNull();
expect(marker).toHaveTextContent('*');
expect(marker).toHaveAttribute('aria-hidden', 'true');
});

it('never sets the native `required` attribute (#3290: no double validation UI)', async () => {
openDialog([def({ name: 'note', type: 'text', required: true })]);

const input = (await screen.findByLabelText(/Param One/)) as HTMLInputElement;
// NOT `toBeRequired()` — jest-dom counts `aria-required="true"` as
// required, so it cannot distinguish the channel under test.
await waitFor(() => expect(input).toHaveAttribute('aria-required', 'true'));
expect(input).not.toHaveAttribute('required');
expect(input.required).toBe(false);
});
});
23 changes: 21 additions & 2 deletions packages/app-shell/src/views/ActionParamDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -236,11 +236,20 @@ export function ActionParamDialog({ state, onOpenChange }: ActionParamDialogProp
onChange={(checked: unknown) => updateValue(param.name, checked === true)}
field={field}
className="mt-0.5"
// Required is a STATE, so it rides the state channel to the
// control (objectui#3299, same shape as #3290/#3298). The
// widget's `toDomProps` whitelist forwards `aria-*` by
// prefix, so this lands on the rendered control. `|| undefined`
// so an optional param carries no attribute at all.
aria-required={param.required || undefined}
/>
</Suspense>
<Label htmlFor={param.name} className="font-normal cursor-pointer">
{param.label}
{param.required && <span className="text-destructive ml-1">*</span>}
{/* Visual-only: the state is announced via `aria-required` on
the control; without `aria-hidden` the bare `*` would fold
into the accessible name ("Label asterisk"). */}
{param.required && <span className="text-destructive ml-1" aria-hidden="true">*</span>}
</Label>
</div>
{errors[param.name] && (
Expand All @@ -257,7 +266,10 @@ export function ActionParamDialog({ state, onOpenChange }: ActionParamDialogProp
<div key={param.name} className="grid gap-2">
<Label htmlFor={param.name}>
{param.label}
{param.required && <span className="text-destructive ml-1">*</span>}
{/* Visual-only (objectui#3299): `aria-required` on the widget is
the announced channel; hiding the `*` keeps it out of the
control's accessible name. */}
{param.required && <span className="text-destructive ml-1" aria-hidden="true">*</span>}
</Label>

<Suspense fallback={<WidgetFallback />}>
Expand All @@ -267,6 +279,13 @@ export function ActionParamDialog({ state, onOpenChange }: ActionParamDialogProp
onChange={(v: unknown) => updateValue(param.name, v)}
field={field}
className={errors[param.name] ? 'border-destructive' : ''}
// State channel for required (objectui#3299) — deliberately NOT
// the native `required` attribute (#3290 ruling: that would arm
// the browser's constraint-validation bubble alongside the
// dialog's own `requiredError` messages — two validators, one
// field). Widgets forward `aria-*` via their `toDomProps`
// whitelist, so this reaches the real control.
aria-required={param.required || undefined}
{...uploadProps}
/>
</Suspense>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/**
* ObjectUI
* Copyright (c) 2024-present ObjectStack Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

/**
* CreateViewDialog — the three always-required controls (display label,
* machine name, and each type-specific required-field select) must announce
* required as a STATE, not as a bare `*` in the label (objectui#3299; same
* shape as #3290/#3298).
*
* Pre-fix, all three drew `<span>*</span>` inside `<label htmlFor>` with no
* `aria-hidden` and no state on the control — screen readers heard
* "…asterisk" in the name and "list required fields" navigation saw nothing.
*
* These fields are unconditionally required (the dialog's own submit gating
* treats them so), hence the static `aria-required="true"` rather than the
* conditional `required || undefined` shape the param dialogs use.
*/

import { describe, it, expect, afterEach, vi } from 'vitest';
import { render, screen, fireEvent, cleanup } from '@testing-library/react';
import { CreateViewDialog } from './CreateViewDialog';

afterEach(cleanup);

function openDialog() {
return render(
<CreateViewDialog open onOpenChange={() => {}} onCreate={vi.fn()} />,
);
}

/** The asterisk `<span>` inside the label pointing at `id` — must be a11y-hidden. */
function markerFor(id: string) {
const label = document.querySelector(`label[for="${id}"]`);
expect(label).not.toBeNull();
const marker = label!.querySelector('span');
expect(marker).not.toBeNull();
expect(marker).toHaveTextContent('*');
return marker!;
}

describe('CreateViewDialog — `aria-required` reaches the controls (objectui#3299)', () => {
it('sets aria-required="true" on the display-label input, with the asterisk a11y-hidden', () => {
openDialog();

const input = screen.getByTestId('create-view-name-input');
expect(input).toHaveAttribute('aria-required', 'true');
expect(markerFor('create-view-name-input')).toHaveAttribute('aria-hidden', 'true');
});

it('sets aria-required="true" on the machine-name input, with the asterisk a11y-hidden', () => {
openDialog();

const input = screen.getByTestId('create-view-machine-name-input');
expect(input).toHaveAttribute('aria-required', 'true');
expect(markerFor('create-view-machine-name-input')).toHaveAttribute('aria-hidden', 'true');
});

it('sets aria-required="true" on a type-specific required-field select (kanban group-by)', () => {
openDialog();

// The required-fields section only renders for types that declare
// required config — switch to kanban to mount its group-by selector.
fireEvent.click(screen.getByTestId('create-view-type-kanban'));

const select = screen.getByTestId('create-view-required-groupByField');
expect(select).toHaveAttribute('aria-required', 'true');
expect(markerFor('create-view-required-groupByField')).toHaveAttribute('aria-hidden', 'true');
});

it('keeps the asterisk OUT of the accessible name (state announced once)', () => {
openDialog();

// `t()` resolves to the key itself in tests; pre-fix the name would have
// been "console.objectView.title *" — "…asterisk" to a screen reader.
const input = screen.getByTestId('create-view-name-input');
expect(input).toHaveAccessibleName('console.objectView.title');
});

it('never sets the native `required` attribute (#3290: no double validation UI)', () => {
openDialog();

// NOT `toBeRequired()` — jest-dom counts `aria-required="true"` as
// required, so it cannot distinguish the channel under test.
for (const id of ['create-view-name-input', 'create-view-machine-name-input']) {
const input = screen.getByTestId(id) as HTMLInputElement;
expect(input).toHaveAttribute('aria-required', 'true');
expect(input).not.toHaveAttribute('required');
expect(input.required).toBe(false);
}
});
});
12 changes: 9 additions & 3 deletions packages/app-shell/src/views/CreateViewDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -507,10 +507,14 @@ export function CreateViewDialog({
className="text-xs font-medium"
>
{t(rf.i18nKey)}
<span className="ml-1 text-destructive">*</span>
{/* Visual-only (objectui#3299): required is announced as a
STATE via `aria-required` on the control; hiding the `*`
keeps "asterisk" out of the accessible name. */}
<span className="ml-1 text-destructive" aria-hidden="true">*</span>
</label>
<select
id={`create-view-required-${rf.key}`}
aria-required="true"
data-testid={`create-view-required-${rf.key}`}
value={selectedFieldValue}
onChange={(e) => setRequiredValue(rf.key, e.target.value)}
Expand Down Expand Up @@ -554,10 +558,11 @@ export function CreateViewDialog({
<div className="space-y-1">
<label htmlFor="create-view-name-input" className="text-xs font-medium">
{t('console.objectView.title')}
<span className="ml-1 text-destructive">*</span>
<span className="ml-1 text-destructive" aria-hidden="true">*</span>
</label>
<Input
id="create-view-name-input"
aria-required="true"
data-testid="create-view-name-input"
autoFocus
value={label}
Expand All @@ -576,10 +581,11 @@ export function CreateViewDialog({
<div className="space-y-1">
<label htmlFor="create-view-machine-name-input" className="text-xs font-medium">
{t('console.objectView.viewName')}
<span className="ml-1 text-destructive">*</span>
<span className="ml-1 text-destructive" aria-hidden="true">*</span>
</label>
<Input
id="create-view-machine-name-input"
aria-required="true"
data-testid="create-view-machine-name-input"
value={name}
onChange={(e) => { setName(e.target.value); setNameTouched(true); }}
Expand Down
Loading
Loading