Uh oh!
There was an error while loading. Please reload this page.
fix(fields): drive the grid widget's per-cell aria-invalid from form-level failure - #5429
Merged
Merged
Conversation
…level failure A required `grid` submitted while EMPTY failed at the form level, rendered its "is required" message, and marked nothing: every row was a ghost row, and ghost rows were skipped by the per-cell validity channel. A sighted user saw red text; a screen-reader user was told nothing at all. The container's deliberate strip of the host `aria-invalid` (objectui#4857) STANDS — a container is not a control. What was missing is the other half that strip implied: the host's failure now drives the per-cell channel the widget already owns. When the `error` slot is set on an empty grid, the ghost entry row stops being skipped and its required cells flag. The mark also moves off the `<td>` and onto the cell's own control (Input, SelectTrigger, and LookupField via its published `error` slot). A `td` is not focusable, and assistive tech reads validity from the control, not the wrapper. Also tightens the registry sweep's DELIVERING assertion to require a FOCUSABLE carrier (objectui#5223), closing the hole where a wrapper mark satisfied a row-wide query while the control announced nothing. Measured first, per the ruling's sequencing caveat: all 37 previously-green rows already carry on a focusable element, so the tightening reds none of them. NOT_YET_DELIVERED is now empty (29 -> 20 -> 9 -> 1 -> 0) and a new case asserts it stays empty, making the header's long-standing "do not add to this list" prose mechanical. Part of #3318 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This was referenced Aug 20, 2026
os-support-ai
marked this pull request as ready for review
August 20, 2026 17:18
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#3318
Also closes#5223, deliberately — flagging it here rather than leaving it to be discovered at merge time. That card's proposed shape is verbatim what the sweep tightening below implements ("require that the element carrying
aria-invalidbe focusable"), including its "measure before committing, then triage whatever goes red" caveat, and it says the tightening should be settled with this card rather than before it. Nothing went red. PM: if you would rather keep #5223 open for a separate pass, drop that keyword before landing.Implements the maintainer's 2026-08-19 ruling on the card (verbatim: 「全部接受」) — Option C for the residual
gridrow, paired with the sweep tightening, measured first.First: the card body's sizing is stale, and this corrects it
Per the ruling's secondary item. The card body still reads "29 of 46 widgets" and lists 29 types. That reading predates two merged PRs and should not be used to size anything:
slidercleared, 7 reclassified, 1 leftThe real denominator is 45, not 46 —
ownerwas retired by #4814. Verified onorigin/mainatbd977f86fbefore editing anything:NOT_YET_DELIVEREDheld exactly one row,grid, and the sweep ran 50/50 green. The ledger file remains the source of truth; the body is history.The defect
A required
gridsubmitted while still empty failed at the form level, rendered its "is required" message, and marked nothing. Every row was a ghost row, and ghost rows were skipped by the widget's per-cell validity channel. A sighted user saw red text; a screen-reader user was told nothing at all.GridFieldwas never a widget that "forgot to forward ARIA". Since #4857 it already spreadstoDomProps(props)onto its container and then deliberately strips the hostaria-invalid, with the reason stated in place: "control-channel state; this grid reports validity per CELL with its own inline marks."The fix (Option C)
That strip stands — this PR upholds #4857 rather than overturning it. A container is not a control. What was missing is the other half the strip implied but nobody had built: the host's failure now drives the per-cell channel the widget already owns.
errorslot is set on an empty grid (hostFailedEmpty), the ghost entry row stops being skipped and its required cells flag — that is the line the user would actually type into to fix the error.tdand onto the cell's own control: the text/number input, theSelectTrigger, andLookupFieldvia its publishederrorslot. Atdis not focusable, and assistive tech reads validity from the control, not the wrapper.The
tdkeeps the visual ring, thetitle, and theline-items-invalid-*test hook, so the existing unit test ande2e/live/grid-conditional-rules.spec.tsare unaffected.Rejected options, not implemented: A (container-div mark — a green trap the sweep could not distinguish from real delivery, and a silent overturn of #4857); B (the "Add line" button — #4857 already rejected routing field identity there); D (a third ledger verdict).
One honest gap, tracked not hidden: a required
filecolumn still marks only the cell's visual ring.FileCellhas a closed prop set and owns its own control, so wiring it needs that component's contract to change — out of scope here, and noted in a comment at the call site rather than half-done.The measurement the ruling required FIRST
The ruling made this sequencing binding: measure the tightening's impact on the currently-green rows first; if it reds legitimate rows, report the list rather than landing it blind.
Measured with a throwaway probe over all 45 registered types (probe not committed), reading the actual carrier element of every
aria-invalid="true"after a real failure:All 37 previously-green DELIVERING rows already carry on a focusable element. The tightening reds none of them. No list to report.
Representative readings, including every case I expected to be at risk:
A finding that changed the shape of Option C
Option C says "put
aria-invalidon the cell's own input". Measured, the sweep's own harness gave it no input to land on:A grid with no
columnsrenders an empty table. Its only focusable element is the auxiliary "Add line" button — the exact carrier option B was rejected for. So the sweep was asking a widget that renders no controls whether it marks its control.The harness now gives
grida two-column config, exactly the precedent already in that file for the option widgets (OPTION_TYPESgetOPTIONS, commented "Option widgets render an unfillable placeholder unless offered a list"). This makes the assertion stricter, not looser: the sweep now has to find the mark on a genuine cell control. Called out here because it is a change to the harness's render state and a reviewer should see it rather than find it.Sweep tightening (the #5223 fix)
The
DELIVERINGassertion now requires thearia-invalid="true"carrier to be focusable. Previously it was a query over the whole row, so a mark on a wrapper passed as "delivered" while the control the user edits announced nothing — the cheapest way to turn a ledger row green, reading clean in review, with nothing else in the file to catch it (the wrapper check guardsNOT_APPLICABLErows only).Reverse verification
Predicted before each run, and each matched:
1. The ratchet fires. With the fix landed and
gridstill in the ledger, its reverse-asserting row must go red:The carrier is the ghost row's own cell input — a focusable control. Option C delivered.
2. The tightening can actually fail. Ablation: re-introduced the wrapper mark (the rejected option A) by putting
aria-invalidback on thetdand removing it from the input:This is the proof that matters: the old assertion would have passed that exact state, because a
tdis found by a row-wide query. The hole is closed, and the new half is not a phantom check. Both ablation legs are source-resolved (the root vitest config aliases every@object-ui/*tosrc), so no rebuild is involved and no staledistcan fake a green.3. After removing the ledger row:
Tests 51 passed (51).The ledger is empty — and now stays empty
NOT_YET_DELIVEREDgoes 29 → 20 → 9 → 1 → 0. The set stays in the file because it is the ratchet, not a to-do list.Bounded in-place fix, named explicitly: I added one assertion,
expect([...NOT_YET_DELIVERED]).toEqual([]). The file header has always said "do NOT add to this list to make a new widget pass", but that was prose — a new widget could quietly acquire a ledger row and the sweep would happily assert it still fails, which reads in review as a considered decision rather than an unfixed gap. Reaching zero is what makes that claim enforceable, so it is now enforced. Same defect class, same file, same gate family, and the failure message tells the next author their three roads (deliver it, measure it intoNOT_APPLICABLE, or edit this assertion as a visible act).Tests
All run from the repo root, under the shared verify lock. Gate union re-run at the final commit
7023649b9:widget-aria-invalid-registry-e2e.test.tsx— 51 passed (51) (38 DELIVERING + 0 ledgered + 7 NOT_APPLICABLE + 6 structural)packages/fields/— 107 files / 1803 passedpackages/plugin-detail/— 87 files / 824 passed (master-detail renders this widget)packages/plugin-form/— 56 files / 574 passedpackages/components/src/renderers/form/— 49 files / 316 passedturbo run type-check --filter=@object-ui/fields— 11/11 successfulcheck:control-bytesOK;check-changeset-presenceOK;check-changeset-no-majorOKeslinton both changed files: 0 errors.GridField.tsxcarries 27 warnings both before and after — measured againstorigin/main, this PR adds none.Changeset:
@object-ui/fieldspatch (nevermajor, fixed-group gate).⛔ Auto-merge not enabled — the PM lands this.
Generated by Claude Code