name 33 more form controls and stop the a11y allowlist from rotting - #4316
Merged
Conversation
…ing (#4156) Continues the unlabeled-control sweep on the Chief of Staff, MeatSpace POST and Voice settings surfaces, and closes two blind spots that were keeping correctly-labeled controls on the allowlist: - The htmlFor-forwarding wrapper recognizer hardcoded a prop named `htmlFor`. StackerNews's `Field({ id, label })` does the same job through `id`, so its 13 already-labeled controls looked unnamed. Both prop names are now read out of the wrapper, and both must still be parameters of it. - A FormField whose only child is a conditional (`{isSelect ? <select/> : <input/>}`) was not credited, though React clones the generated id onto whichever branch renders. Rendered lists stay excluded: Children.map flattens an array and clones only its first element. VoiceTab carried a page-local clone of the shared FormField; it is deleted in favour of the real one, which is what the guard already understands. A new test fails on any allowlist entry that no longer matches an unnamed input, so the burn-down can only shrink. It immediately caught 9 entries left behind by #4313. PREEXISTING_INPUT_NAME_ALLOWLIST: 142 -> 71.
…label JobsTab's forms (#4156) Review of the previous commit surfaced two defects in its own fixes and three cleanups: - JobsTab's create/edit forms are stacked full-width fields whose only visible affordance was a placeholder. Naming them with aria-label satisfied the guard while leaving sighted users with no label the moment they typed — the placeholder-as-label defect preserved and certified as fixed. They now use FormField, like the rest of the diff. - Two aria-labels truncated the `title` text that was already serving as the accessible name ("Cron expression" for "Cron expression: minute hour dayOfMonth month dayOfWeek"). Folded the detail back in. - isNestedInLabeledFormField credited any element literally spelled <FormField>, unverified, across 313 call sites — the guard's widest exemption. It now requires the file to import the shared component. - The two allowlist rules re-scanned every tracked file independently. They share one memoized scan: same question, opposite comparison. Suite time for the file drops ~30%, and the two can no longer drift on what "unnamed" means. - localLabelWrapperNames and localHtmlForForwarders had byte-identical declaration-walking prologues; both now consume forEachLocalComponent. isEnclosedInListCall reuses balancedCallAt instead of hand-rolling a fourth string-aware paren scanner. Follow-ups filed: #4317 (unify the wrapper recognizers; arrow-function and imported wrappers are both invisible today) and #4318 (an apostrophe in JSX text breaks matchingBraceEnd, which is why one AIProviders field stays on the allowlist while its four identical siblings came off).
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 free
to 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.
Summary
Continues the unlabeled-form-control sweep from #4156, and closes the guard blind spots that were keeping already-correct controls on the backlog.
Controls named (33 across 12 files) — Chief of Staff (
TaskAddForm,AgentCard,ConfigRow,JobsTab,MemoryEditModal,TaskItem,ScheduleEditor), MeatSpace POST (EpigeneticTrackerand the drill runners), andMorseTrainer's slider rows. A real visible label wherever the surface had room —FormFieldfor JobsTab's stacked create/edit forms,<label htmlFor>/idpairing forConfigRowandSliderRow(which also makes the row text a click target for a 16px checkbox) — andaria-labelonly in compact toolbars, inline table-edit rows and centered drill answer fields, where context supplies the name and there is no room for a label.Three guard shapes taught, not worked around — adding an
aria-labelto a control that already has a visible label shadows that text, so these were fixed in the recognizer instead:htmlFor.StackerNews.jsx'sField({ id, label, children })does the same job throughid, so its 13 already-labeled controls read as unnamed. Both prop names are now captured from the wrapper's own source, and both must still be parameters of it.FormFieldwhose only child is a conditional ({isSelect ? <select/> : <input/>}) was not credited, though React clones the generated id onto whichever branch renders. Rendered lists stay excluded —Children.mapflattens an array and clones only its first element, so crediting every control in a.map()would exempt all but one. A.map()in the other ternary branch does not disqualify the shape.isNestedInLabeledFormFieldcredited any element literally spelled<FormField>with alabel=, unverified, across 313 call sites — the guard's widest exemption. It now requires the file to import the shared component, so a page-localFormFieldcannot silently exempt everything under it.One duplicate deleted —
VoiceTab.jsxcarried a page-local clone of the sharedFormField(samelabel/hint/children/classNamecontract, sameuseId+cloneElementid injection). It is replaced by the real component, which the guard already understands; spacing is preserved (the clone'sspace-y-1andFormField'smb-1on label and hint are equivalent, and every call site has a single child).The allowlist can now only shrink — a new test fails on any
PREEXISTING_INPUT_NAME_ALLOWLISTentry that no longer matches an unnamed input. A stale entry is not harmless: it silently re-exempts the next control to land on that same anchor. It immediately caught 9 entries left behind by #4313 when the label-wrapping recognizer landed.PREEXISTING_INPUT_NAME_ALLOWLIST: 142 → 71, exactly half the remaining backlog. No entry was broadened.Review pass
The second commit addresses a
/simplifyreview of the first, including two defects in its own fixes:aria-labelsatisfied the guard while leaving sighted users with no label the moment they typed — preserving the placeholder-as-label defect and certifying it as fixed. They now useFormField.aria-labels truncated thetitletext that was already serving as the accessible name ("Cron expression"for"Cron expression: minute hour dayOfMonth month dayOfWeek"). The detail is folded back in.localLabelWrapperNamesandlocalHtmlForForwardershad byte-identical declaration-walking prologues, now behindforEachLocalComponent.isEnclosedInListCallreusesbalancedCallAtinstead of hand-rolling a fourth string-aware paren scanner.Test plan
cd client && npm test— 651 files, 7955 tests, all passing.cd client && npm run lint— clean (biome, 2048 files,--error-on-warnings).cd client && npm run build— clean.antigravityagainst the full diff, focused on false positives in the recognizers — no findings.client/src/a11yConventions.test.js:keeps no stale entries in the pre-existing input allowlist.credits a FormField whose only child is a conditional, but not a list— probes the ternary shape, a.map()in the sibling branch, a rendered list, a control nested one element deeper, an unlabeled FormField, and aFormFieldthat is not the shared one.label=, and anhtmlFor=attribute that is not the prop the wrapper forwards.Remaining
71 entries. Largest clusters:
pipeline/arcCanvas/**(9),universeBuilder/**(9),digital-twin/tabs/**(8),src/pages/**(7),calendar/**(6),apps/tabs/**(5), then 1–3 per directory. Same caveat as before — verify each candidate in context, do not mass-apply.Follow-ups filed from the review:
aria-labelover the correct visible label for them.matchingBraceEnd, which is why oneAIProviders.jsxfield stays on the allowlist while its four structurally identical siblings came off.<select>and<textarea>remain outside the guard's scope entirely, tracked in #4309.Refs #4156