fix: name 80 more unlabeled form controls in the Goals, Brain, MeatSpace and Agent tabs (#4156) - #4313
Merged
Conversation
…ace and Agent tabs (#4156) Continues the accessible-name sweep past the page-level slice shipped in #4307, working four whole component subtrees rather than scattered hits: client/src/components/goals, brain/tabs, meatspace/tabs and agents/tabs. Each candidate was read in context first — controls already named by a wrapper were left alone. Where the surface had room the control gets a visible label wired with htmlFor/id (the shared FormField, or an existing orphan <label> given the pairing it was missing); compact toolbars and inline table-edit rows get aria-label instead. MemoryTab's five add/edit forms moved to FormField so every field carries a visible label rather than a placeholder alone. LifestyleTab's page-local FieldGroup rendered a <label> that pointed at nothing. It now takes an htmlFor prop for its three single-control groups and falls back to role="group" + aria-labelledby for the button sets, which have no labelable control. Wrapping was not an option there — an implicit <label> would pull the live value readout and the hint paragraph beside each slider into the accessible name. The a11y guard learns that shape: hasMatchingExplicitLabel now also credits a same-file wrapper that forwards htmlFor onto a <label> it fills with its own `label` prop, under the same limits as the existing label-wrapper recognizer. A probe test pins the bypasses shut — a forwarder whose <label> holds no text, and a call site with no `label=`, both still count as unnamed. All 80 corresponding PREEXISTING_INPUT_NAME_ALLOWLIST entries are removed, so these controls are now enforced by the guard instead of exempted.
…mposition rows to Milestone (#4156) Three findings from the review pass: - `label={true}` (and its `<Field label>` shorthand) read as a usable label attribute, but React renders `{true}` as no text at all — so a wrapper call site could claim a name it never renders. Both the existing label-wrapper check and the new htmlFor forwarder now share one isUsableLabelAttributeValue predicate that rejects it. No call site in the tree passes `label={true}`, so this closes a hole rather than changing a verdict. - GoalPlanSection's proposedDecomposition rows are milestones — the panel itself says "N milestones proposed" — not sub-goals. Renamed the two aria-labels so screen readers say what the row is. - The forwarder recognizer relies on the input scan handing it maskComments(src), which is what keeps a commented-out wrapper from registering. That was load-bearing and undocumented; it now says so, and the probe test asserts it against masked source instead of looking safe for the wrong reason.
… the shared predicate too (#4156) isNestedInLabeledFormField was the third site carrying the same inline label-validity check and the only one left accepting `<FormField label>` / `label={true}` as a usable name. All three now share isUsableLabelAttributeValue, so the rule cannot drift apart per call path.
atomantic
force-pushed
the
claim/issue-4156
branch
from
August 15, 2026 19:43
c7bbd16 to
96935c3
Compare
atomantic
added a commit
that referenced
this pull request
Aug 15, 2026
…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.
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
1 participant
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 accessible-name sweep past the page-level slice shipped in #4307. That pass worked the named pages; this one works four whole component subtrees, so the result is coherent per-area rather than a scatter of individual fixes:
client/src/components/goals/**(8 files)client/src/components/brain/tabs/**(6 files)client/src/components/meatspace/tabs/**(5 files)client/src/components/agents/tabs/**(2 files)Every candidate was read in context before being touched — controls already named by a wrapper were left alone. 80 genuinely-unnamed controls got names:
htmlFor/idwhere the surface had room. The sharedFormFieldforGoalEditForm's title/description and all five ofMemoryTab's add/edit forms (people, projects, ideas, admin, memories), which previously ran on placeholders alone. Existing orphan<label>elements —GoalEditForm's "Time slot"/"Duration",AlcoholTab's "Volume" — got the pairing they were missing rather than a new one bolted on.aria-labelfor compact toolbars, inline table-edit rows, and repeated list rows (GoalPlanSection's phase/sub-goal rows index their labels), where a visible label would break the layout.LifestyleTab's page-localFieldGrouprendered a<label>that pointed at nothing. It now takes anhtmlForprop for its three single-control groups, and falls back torole="group"+aria-labelledbyfor the button sets, which have no labelable control. Wrapping was not an option there: an implicit<label>would pull the live value readout and the hint paragraph beside each slider into the accessible name.The a11y guard learns that shape.
hasMatchingExplicitLabelnow also credits a same-file wrapper that forwardshtmlForonto a<label>it fills with its ownlabelprop, under the same limits as the existing label-wrapper recognizer (same-filefunctiondeclarations only, so a missed one is a false negative). A probe test pins the bypasses shut — a forwarder whose<label>holds no text, and a call site with nolabel=, both still count as unnamed.No allowlist was broadened. All 80 corresponding
PREEXISTING_INPUT_NAME_ALLOWLISTentries inclient/src/a11yConventions.test.jsare removed, so these controls are enforced by the repo-wide guard instead of exempted — 222 entries down to 142.Test plan
cd client && npm test— 650 files / 7939 tests pass.cd client && npm run lint— clean.client/src/a11yConventions.test.jspasses with the 80 allowlist entries deleted, which is the actual regression check: re-adding any entry is no longer needed for these files, and removing a fix without the entry fails the suite.only credits an htmlFor-forwarding wrapper that really names the controlasserts the new recognizer fires for a wrapper that labels, and does not fire for a text-less<label>, a call site missinglabel=, or a non-matching id.Remaining
This is a deliberate slice of a long tail, not the whole backlog.
PREEXISTING_INPUT_NAME_ALLOWLISTstill carries 142 entries, largest clusters:src/pages/**(26, mostlyAIProviders.jsxandStackerNews.jsx)src/components/meatspace/post/**(14)src/components/cos/tabs/**(14)src/components/settings/**(11)src/components/universeBuilder/**(9),src/components/pipeline/arcCanvas/**(9),src/components/music/**(8),src/components/digital-twin/tabs/**(8), and a long tail of 1–6 per directory<select>and<textarea>also remain outside the guard's scope entirely (it scans<input>only); that extension is tracked separately per the earlier discussion on this issue. This PR did name the<select>/<textarea>controls it encountered inside the four swept subtrees, but they are not yet guarded.Refs #4156