Repository navigation
Commit 557abc2
fix(app-shell,plugin-designer): keep the required
Fixes #10367
Clause-②: no
Dispatched by the `domain:ui#4` PM seat for objectui#10367; implemented
in session `https://claude.ai/code/session_01BP8CMtACxTdLjqR6rhd33C`.
**File surface, amended twice.** The PR is three rounds, one commit per
patch round.
- **Round 0** (`663fb5b2f`, `b48e7ae92`, `963d288c0`): the three sites
the card names.
- **Round 1** (`2f3d01273`): adds
`packages/app-shell/src/views/metadata-admin/widgets.tsx`. The seat
answered A to the report's open question and amended the claim in
comment 5825438356 on objectui#10367. Without round 1, the PR would have
hidden the `*` of the 11 registered `labelling: 'control'` widgets while
they expose no required state.
- **Round 2** (`91c075fa4`): adds
`packages/app-shell/src/views/metadata-admin/color-variant-field.tsx`,
through amendment 5825786390. It closes the same regression shape on
`color-picker`, which round 1's report measured. Its `radiogroup` would
otherwise have lost the `*` with no replacement.
Each patch round adds one pin case.
## What changed
Three forms drew the required `*` as a bare span inside the label that
names the control, so assistive tech read it as part of the name ("Title
*"), and no control reported that it was required. The fix at each site
follows objectui#10361 (the console `FormPage`, landed `069ce12b`) and
objectui#3299 (`ActionParamDialog`): the marker span gets
`aria-hidden="true"`, and the control gets `aria-required`.
- **`packages/app-shell/src/views/ScreenView.tsx`** (flow screen
fields). The `*` span inside `Label htmlFor=ff-NAME` is hidden.
`ScreenFieldInput` puts `aria-required` on every control arm it renders
(Input, Textarea, Checkbox, SelectTrigger) when the field is required.
The attribute is omitted when the field is optional, so it is never
written as "false".
- **`packages/app-shell/src/views/metadata-admin/SchemaForm.tsx`**.
- Both `showRequiredStar` spans (the boolean row and the default row)
are hidden.
- So is the grid repeater's column-header `th` marker. Every cell
control is named from that header through `aria-labelledby`, so its `*`
reached every cell's name.
- `FieldControl` takes a new internal `required` prop. `FieldRow` passes
`showRequiredStar`, and a grid cell passes its column's `required`, so
the state always matches the visible marker.
- Every control `FieldControl` renders itself carries `aria-required`
from that prop: both Select triggers, the Switch, the number and text
Inputs, the long-string Textarea, the comma-separated list Input, and
`RawJsonEditor`'s textarea.
- `FieldControl`'s registered-widget branch passes the same prop to the
widget (round 1), in a row and in a grid cell alike.
- **`packages/app-shell/src/views/metadata-admin/widgets.tsx`** (rounds
1 and 2).
- `WidgetProps` gains an optional `required` member.
- The file-local `controlNaming()` helper emits `aria-required` beside
`id` / `aria-labelledby` when `required` is true, and omits it
otherwise.
- Ten of the eleven `labelling: 'control'` widgets pass `required` into
`controlNaming()` at every call site they have. That covers the loading,
failure and empty-catalog placeholder inputs, because they are the
control on screen at that moment. The ten are `ref:object`,
`ref:component`, `object-selector`, `field-selector`, `field-ref`,
`view-ref`, `icon`, `color-input`, `string-tags` and `secret`.
- None of the eleven spreads its naming props outside `controlNaming()`.
`filter-builder` reaches the helper indirectly, through
`FilterBuilderField`, and is the one deliberate exception (see below).
- Round 2: `ColorSwatchGroupWidget` (the `color-picker` key, a
`labelling: 'group'` widget) passes `required` to `ColorVariantPicker`.
- Only an attribute is added. No widget's visual output or behaviour
changes.
-
**`packages/app-shell/src/views/metadata-admin/color-variant-field.tsx`**
(round 2). `ColorVariantPicker` takes an optional `required` and puts
`aria-required` on its `role="radiogroup"` element when it is true,
omitting it otherwise. ARIA 1.2 supports the state on `radiogroup`. Its
callers:
- `ColorSwatchGroupWidget` (`widgets.tsx`): passes the field's flag.
- `DashboardWidgetInspector`'s widget colour field: passes nothing, so
it is unchanged.
- `PageBlockInspector`'s `ColorPropField`: passes nothing, so it is
unchanged.
- `color-variant-field.labelling.test.tsx`, whose compile-time naming
pins still hold, because an added optional prop changes no naming shape.
- **`packages/plugin-designer/src/AppCreationWizard.tsx`**. The App name
and Title markers are hidden, and both inputs carry
`aria-required="true"`. The separating space moved inside the hidden
span. With the space outside it, Chromium 141 named the inputs `"App
Name "` and `"Title "` (trailing space). With it inside, the names are
exact. The rendered label looks the same.
Every marker also carries `data-required-marker="true"`. I chose it for
consistency, not because it was required. It is the locator the shared
form renderer's marker in `@object-ui/components` already carries, and
objectui#10428 uses the same span shape. It gives the pins a locator
that is not the a11y attribute under test. Each site keeps its existing
visual classes.
Plus three pins beside the sources, and one changeset
(`@object-ui/app-shell`, `@object-ui/plugin-designer`: patch). Rounds 1
and 2 extended the changeset body to state the widget half and the
colour picker; its frontmatter is unchanged.
## Which surfaces may carry `aria-required` (why `filter-builder` does
not)
`filter-builder`'s control is a popover trigger rendered as a plain
`button`. ARIA 1.2 does not support `aria-required` on role `button`. I
measured each surface shape with axe-core 4.13.0 (`aria-allowed-attr`)
in headless Chromium 141, with `aria-required="true"` on it:
| surface | axe `aria-allowed-attr` |
|---|---|
| `button` (the `filter-builder` trigger) | **violation**: "ARIA
attribute is not allowed: aria-required" |
| `button` with `role="combobox"` (`icon`, `object-selector`,
`field-ref`, `view-ref`) | pass |
| `input type=text` (`string-tags`, and the `ref:*` / `field-selector`
inputs) | pass |
| `input type=password` (`secret`) | pass |
| `input type=color` (`color-input`) | pass |
| `div role="radiogroup"` (`color-picker`, round 2; the real rendered
SchemaForm DOM, not a synthetic shape) | pass, 0 violations on the page
|
So `FilterBuilderField` keeps calling `controlNaming({ id,
ariaLabelledBy })` without `required`, and a comment at that call site
says why. Giving the trigger a role that supports the state would change
its semantics, which is outside the "attributes only" scope.
## A1 — submit behaviour, per site (why `aria-required` everywhere,
never native `required`)
| site | form element / `noValidate` | would native `required` change
behaviour? |
|---|---|---|
| `ScreenView` | no form element in `ScreenView` or either host
(`FlowRunner` wraps it in a Dialog with a button-driven `submit`;
`ScreenPreview` in a card) | Yes, for the checkbox. `FlowRunner`'s
required check counts an unchecked `false` as an answer, while native
`required` on a checkbox means "must be checked". Any host form element
would also add a browser verdict beside the runner's own check. |
| `SchemaForm`, its widgets and `ColorVariantPicker` | no form element
in any in-repo host; `SchemaForm` is exported from the package root |
Inside a host form element, the browser would block submit on its own
verdict, beside the form's own validation. A `radiogroup` of buttons has
no native `required` at all. |
| `AppCreationWizard` | no form element; the wizard gates Next and
Create itself (`step1Valid` / `canProceed`) | Same: a host form element
would gain a second, browser-side verdict. The component is exported and
registered. |
No validation logic changed.
## Measured on main before the change (BASE `9ec8ceba0`)
The three new pin files (round-0 cases), run against the three source
files checked out at BASE (armed trap, restore proven by blob hash ==
HEAD and empty `git diff HEAD`): **`Tests 6 failed | 3 passed (9)`**.
The six required cases failed, and the three optional-field controls
passed. Accessible names reported by testing-library on main: `App Name
*`, `Title *` (wizard, ScreenView, SchemaForm), `Note *`, `Agree *`,
`Kind *`, `Enabled *`, and the grid cell `Field *`. No control carried
`aria-required`.
## The pins
-
`packages/app-shell/src/views/ScreenView.requiredMarker-10367.test.tsx`
- The card's probe: `getByRole('textbox', { name: 'Title' })` finds the
required text field, which is `toBeRequired()` through `aria-required`
and has no native `required`.
- The marker is still drawn and is `aria-hidden`.
- The Textarea, Checkbox and Select arms are found by their bare names
and carry `aria-required`.
- Control: optional fields have no marker and no `aria-required`.
-
`packages/app-shell/src/views/metadata-admin/SchemaForm.requiredMarker-10367.test.tsx`
- The card's probe: a required string property's textbox has the
computed name exactly `Title` and is `aria-required`.
- The boolean row's required switch (no default) has the same shape.
- A grid cell under a required column header is named `Field` and is
`aria-required`. The optional column beside it is the in-render control.
- **Round 1:** a required `string-tags` row is found as textbox `Tags`,
and a required `icon` row as combobox `Glyph`. Both are named without
`*` and carry `aria-required="true"`. An optional `string-tags` row in
the same render (`Extra`) carries none. A grid cell whose required
column renders `string-tags` is named `Tags` and carries
`aria-required="true"`.
- **Round 2:** a required `color-picker` field (schema palette `enum:
['red', 'blue']`) is a radiogroup named exactly `Accent` with
`aria-required="true"`. An optional `color-picker` beside it (`Tint`)
carries none.
- Control: an optional string property has no marker and no
`aria-required`.
-
`packages/plugin-designer/src/AppCreationWizard.requiredMarker-10367.test.tsx`
- The card's probe: `getByRole('textbox', { name: 'App Name' })` and `{
name: 'Title' }` both find their inputs, which are `aria-required` and
have no native `required`.
- Both markers are hidden.
- Control: the optional Description textbox has no `aria-required`.
The markers are located by `data-required-marker`, never by
`aria-hidden` itself.
**Every registered widget that can carry the state, measured (throwaway
probes, not committed).** Each widget was rendered as a required and as
an optional field, and I read its surface:
| widget | surface | required | optional |
|---|---|---|---|
| `ref:object`, `ref:component`, `field-selector` | `input` |
`aria-required="true"` | absent |
| `string-tags` | `input type=text` | `aria-required="true"` | absent |
| `secret` | `input type=password` | `aria-required="true"` | absent |
| `color-input` | `input type=color` | `aria-required="true"` | absent |
| `object-selector`, `field-ref`, `view-ref`, `icon` | `button
role=combobox` | `aria-required="true"` | absent |
| `filter-builder` | `button` | absent (by design, above) | absent |
| `color-picker` (round 2) | `div role=radiogroup` |
`aria-required="true"` | absent |
## A2 — reverse proof (each fix committed first; mutations via
`ablation-replace.mjs`, restore proven each time)
- **`aria-hidden` removed** from the `ScreenView` marker, on `663fb5b2f`
(anchor 1 → 0, blob `bf60e49c5a3d` → `95117bc41e00`):
`ScreenView.requiredMarker-10367.test.tsx` **`Tests 2 failed | 1 passed
(3)`**. `Unable to find an accessible element with the role "textbox"
and name "Title"` (and `"Note"`). The control case stayed green.
Restored: blob == HEAD, `git diff HEAD` empty.
- **`aria-required` removed** from `SchemaForm`'s string Input, on
`663fb5b2f` (anchor 1 → 0, blob `593cc82da19e` → `574671beef1a`; on-disk
`aria-required={ariaRequired}` count 7 → 6):
`SchemaForm.requiredMarker-10367.test.tsx` **`Tests 2 failed | 2 passed
(4)`**. The string-property case and the grid string cell both failed on
`toHaveAttribute('aria-required', 'true')`, because they share that
control. Restored: blob == HEAD, `git diff HEAD` empty.
- **Round 1: the `aria-required` emit removed from `controlNaming()`**,
on `2f3d01273` (anchor `'aria-required': required ? true : undefined };`
1 → 0, blob `b9bfb1c8ab4f` → `7c3f51bebcab`, on-disk emit count 0):
**`Tests 1 failed | 4 passed (5)`**. Only the registered-widget case
failed, on `expect(tags).toHaveAttribute('aria-required', 'true')`, as
predicted before the run. Restored: blob == HEAD (`b9bfb1c8ab4f`), `git
diff HEAD` empty.
- **Round 2: the `aria-required` emit removed from
`ColorVariantPicker`**, on `91c075fa4` (anchor `aria-required={required
? true : undefined}` 1 → 0 in `color-variant-field.tsx`, blob
`72518440a3f5` → `b46ef83b3b28`, on-disk emit count 0): **`Tests 1
failed | 5 passed (6)`**. Only the `color-picker` case failed, on
`toHaveAttribute('aria-required', 'true')`, as predicted before the run.
Restored: blob == HEAD (`72518440a3f5`), `git diff HEAD` empty.
- Declared, not hidden: my first round-0 leg-1 attempt was refused by
the tool before running anything. The replacement string was a substring
of the anchor, so its count could not rise. The file was restored and
the leg was re-run with a distinct replacement.
## Real browser (headless Chromium 141, `/opt/pw-browsers/chromium`)
The DOM these forms render was dumped from throwaway renders (not
committed) and loaded into Chromium. These are the CDP
accessibility-tree names.
| form | this branch | same markup with the markers' `aria-hidden`
removed (the shape on main) |
|---|---|---|
| ScreenView | textbox `Title` (required=true), checkbox `Agree`,
combobox `Kind` (required=true) | `Title *`, `Agree *`, `Kind *` |
| SchemaForm | textbox `Title` (required=true), switch `Enabled`, grid
cell textbox `Field` (required=true) | `Title*`, `Enabled*`, `Field*` |
| AppCreationWizard | textbox `App Name` (required=true), textbox
`Title` (required=true), `Description` / `Icon` (required=false) | `App
Name *`, `Title *` |
| SchemaForm `color-picker` (round 2) | radiogroup `Accent`
(required=true), radiogroup `Tint` (required=false) | not dumped |
The round-1 widget surfaces were checked in Chromium through axe (the
surface table above), not through a name dump.
## A3 — existing tests
- I grepped for tests that assert these labels with a `*` in the name.
The one hit is `ScreenPreview.test.tsx`:
`expect(screen.getByText('*'))`. It still passes unchanged, because
`getByText` does not filter `aria-hidden` and the marker is still drawn.
No test was edited or deleted.
- No source-text anchor pin (`readFileSync`-style) quotes a changed
line. `column-identity.ratchet.test.ts` reads both `SchemaForm.tsx` and
`widgets.tsx`; it was re-run green on `91c075fa4` (`7 passed`).
## Gates
Round-2 gates all ran on head **`91c075fa4`**, after its only commit:
- **Closure build and type-check:** `turbo run build
--filter='@object-ui/app-shell^...' --concurrency=2` then `pnpm --filter
@object-ui/app-shell type-check` (`tsc --noEmit && tsc -p
tsconfig.test.json`): `Tasks: 28 successful, 28 total` (all cache hits;
no dependency of app-shell changed), then exit 0.
- The test tsconfig includes `color-variant-field.labelling.test.tsx`,
whose compile-time pins say an unnamed or doubly-named picker must not
compile. They still hold.
- Outside `widgets.tsx`, the metadata-admin `WidgetProps` is named only
in app-shell's metadata-admin (`form-spec.ts`, `SchemaForm.tsx` and four
tests). It is not re-exported from any package barrel.
- The one non-test direct use of a widget, `ActionDefaultInspector`'s
`IconPickerWidget`, passes no `required` and is unchanged.
- **Tests (`pnpm exec vitest run --maxWorkers=2`):** 257 files (255 in
app-shell, the plugin-designer pin, and `packages/types`'
`phase2-schemas.test.ts`, which names `PageBlockInspector` in a
comment), in three chunks: `94 passed (94) / 1082 tests`, `82 passed
(82) / 878 tests`, `81 passed (81) / 971 tests`.
- The set is the round-1 union (files naming `SchemaForm` / `ScreenView`
/ `FlowRunner` / `ScreenPreview`, a `SchemaForm` host, `widgets`, a
`*Widget` component, `WidgetProps`, `controlNaming` or `WIDGETS`) plus
every test naming `ColorVariantPicker`, `color-variant-field`,
`ColorSwatchGroupWidget`, `DashboardWidgetInspector` or
`PageBlockInspector`.
- That includes `color-variant-field.test.tsx`,
`color-variant-field.labelling.test.tsx`,
`SchemaForm.widgetLabelling.test.tsx`,
`SchemaForm.optionVisibleWhen.test.tsx` and
`PageBlockInspector.i18n.test.tsx`.
- **Check scripts, each exit 0:**
- `check-changeset-presence`: `8 source file(s) of 2 released package(s)
changed, and this change declares 1 changeset(s)`.
- `check-changeset-no-major`, `check-changeset-overwrite`,
`check:pending-changeset-literals`, `check:control-bytes`.
- `check:new-line-citations`: `0 new citation(s)`.
- `check-governed-queue-guard --test`: NOT GOVERNED.
- **`check:changeset-claims`:** it flagged five pending changesets that
name `SchemaForm.tsx` or `widgets.tsx` (6247, 5596, 5040, 7125, 8218). I
read each paragraph, and none is falsified.
- By symbol, no pending changeset names `ColorVariantPicker` or
`color-variant-field`.
- `ColorSwatchGroupWidget` appears only in 6247, whose option-filtering
claims are untouched.
- `WidgetProps` appears only in 5040, and `controlNaming` only in 9889.
Both still hold.
- **Lint:** the per-rule warning counts are identical before and after
round 2 on `color-variant-field.tsx` (0e/2w), `widgets.tsx` (0e/37w),
`SchemaForm.tsx` (0e/16w) and the pin (0e/0w).
- Round-0 lint on the other files is unchanged, because those files did
not move.
- The population and invariance reasoning is as in round 0: the root
`eslint.config.js` has no type-aware linting, and no rule in
`eslint-rules/` reads the file system.
- The repo-wide `pnpm lint` is left to CI.
- **Round-0 gates for files rounds 1 and 2 did not touch**
(`ScreenView.tsx`, `AppCreationWizard.tsx` and their pins): measured on
`963d288c0`.
- `pnpm --filter @object-ui/plugin-designer type-check`: exit 0.
- The whole `packages/plugin-designer/`: `25 passed (25) / 182 tests`.
- plugin-designer does not depend on app-shell, so rounds 1 and 2 cannot
move either reading.
## Acceptance notes
- **`filter-builder`** is the one `labelling: 'control'` widget left
without a required state, because its surface is a `button` (see above).
The seat recorded this and did not fold it in.
- **Structured faces and the remaining `group` widgets** (composite,
repeater, record, nested form, object rows; multiselect, condition and
the like) render `role="group"`, which does not support `aria-required`
in ARIA 1.2. Their hidden marker leaves no required channel there.
`color-picker` is the one `group` widget whose surface (a `radiogroup`)
supports it, and round 2 covers it.
- **Chromium's CDP tree reports no `required` for the checkbox
(ScreenView) or the switch (SchemaForm)**, although both carry
`aria-required="true"` in the DOM. objectui#10361 recorded the same
reading for the `FormPage` checkbox.
- The master-detail widget's table header in `widgets.tsx` also draws a
bare `*`. No cell there references that header by IDREF, so the `*`
reaches no control's name. This is an observation, not this defect, and
no round changes it.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01BP8CMtACxTdLjqR6rhd33C)_
---------
Co-authored-by: Claude <noreply@anthropic.com>* out of three more forms' accessible names (#10450)1 parent 544aca2 commit 557abc2
9 files changed
Lines changed: 614 additions & 46 deletions
File tree
- .changeset
- packages
- app-shell/src/views
- metadata-admin
- plugin-designer/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
216 | 216 | | |
217 | 217 | | |
218 | 218 | | |
219 | | - | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
220 | 225 | | |
221 | 226 | | |
222 | 227 | | |
| |||
238 | 243 | | |
239 | 244 | | |
240 | 245 | | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
241 | 253 | | |
242 | 254 | | |
243 | 255 | | |
244 | 256 | | |
245 | | - | |
| 257 | + | |
246 | 258 | | |
247 | 259 | | |
248 | 260 | | |
| |||
252 | 264 | | |
253 | 265 | | |
254 | 266 | | |
255 | | - | |
| 267 | + | |
256 | 268 | | |
257 | 269 | | |
258 | | - | |
| 270 | + | |
259 | 271 | | |
260 | 272 | | |
261 | 273 | | |
262 | 274 | | |
263 | 275 | | |
| 276 | + | |
264 | 277 | | |
265 | 278 | | |
266 | 279 | | |
| |||
0 commit comments