Repository navigation
fix(bulma-ui): name the picker bases and the horizontal examples from a Field label - #1007
Conversation
DateInputBase, TimeInputBase and DateTimeInputBase composed in a labeled Field now take the id its label points at when the caller set none, as InputBase does, so the label names the input on the text and native paths alike. Inline there is no input, so they take nothing from the Field and build no ids on it.
…r label
The Context-Aware Rendering examples on the Input, Select and TextArea pages
put the control in an inner Field inside a horizontal Field's body. That
inner Field starts its own label scope, so the outer label's for matched
nothing and the control had no name. They now wire the label by hand with
labelProps={{ htmlFor }} and a matching id, the way the other control pages
do. The Field page and the bestax-form skill name the picker bases among
the controls a labeled Field names.
… to a name Each control in the hand-wired horizontal layout the Context-Aware Rendering examples show, with and without the inner Control, takes its name from the outer Field's label.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughPicker bases now use their containing Field label ID when no ID is supplied, except in inline mode. Horizontal nested Input, Select, and TextArea examples use explicit matching label targets and control IDs. Tests, stories, and documentation describe these association patterns. ChangesField label associations
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The changes connect picker and nested-control labels to their inputs while preserving the documented exceptions. The inspected code and guidance are consistent, so no actionable merge risk remains. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main changes: labeling the picker bases and correcting horizontal Field examples. |
| Description check | |
The description provides a detailed summary of the implementation, affected areas, tests, documentation updates, scope exclusions, and related issue. It does not reproduce the template sections for af… |
| Linked Issues check | |
Issue #968 has two coding requirements. The Input, Select, and TextArea horizontal examples now use matching labelProps={{ htmlFor }} and control id values. DateInputBase, TimeInputBase, and `… |
| Out of Scope Changes check | |
The implementation, tests, Field documentation, examples, story, and bestax-form guidance support the label-association requirements in #968. The generated data updates reflect those documentation c… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 7… |
✨ Finishing Touches
-
📝 Generate docstrings
-
-
- Commit to this branch
- Create a new PR
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Preview DeploymentPreview URL: https://830c2458.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 1 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Accessibility | Four other horizontal examples on the same two pages keep the unwired inner-Field shape (input.md:541, 560, 582, select.md:326) — neither fixed nor tracked |
docs/docs/api/form/input.md:637 |
Overall: The library change is sound and tightly scoped. const id = idProp ?? (inline ? undefined : fieldLabelId) lands in all three picker bases before any early return, the hook is read unconditionally, and because each base renders the native <input type="date|time|datetime-local"> and the role="combobox" text input in mutually exclusive branches, one id can never appear twice in the DOM. It matches InputBase's rest.id ?? fieldLabelId presence semantics exactly, so an explicit id={undefined} still adopts, and inline is excluded before popoverId derives anything, which preserves the line #939 drew. I ran the whole src/form suite (46 files, 2829 tests) and src/__tests__/stories-conformance.test.ts green, and tsc -p tsconfig.test.json clean. The riskiest part is not in the source: the three picker bases are the complete set of single-input bases that were missing this, and DateRangeInputBase (two inputs, a group) is correctly excluded by the convention the diff writes into src/form/CLAUDE.md. Look at the advisory first — it is the only thing I would want a decision on.
Residual risk:
- Another
*Baseleft unnamed. Refuted by enumeration: the folder holds exactly seven*Base.tsxfiles, and a grep foruseFieldLabelIdnow shows six of them consuming it. The seventh,DateRangeInputBase, renders two inputs rather than one, so the convention added tosrc/form/CLAUDE.mddoes not claim it, and its gap is tracked in #1005. - A picker mode with no input that adopts the id anyway. Refuted: each of the three files has exactly two early returns (
if (useNative),if (inline)) plus the main path, anduseNativeis gated on!inlinein all three.inlineis the only no-input mode, and it is the one excluded. - A derived id breaking a lookup. Refuted:
popoverIdis now built from the surroundingField'suseIdoutput (which carries:on React 18), but a grep overbulma-ui/srcfinds noquerySelectoron an id selector and nogetElementByIdin the picker internals — the id reaches only attributes (id,aria-controls), where a colon is a legal IDREF. The React 18 and React 19 matrix jobs are both green on this SHA. - Hydration mismatch on the native path. Refuted: the adopted id comes from the
Field'suseId, identical on server and client, and the server and first client render agree on the text path (useIsHydratedfalse,shouldUseNativefalse) before switching — the id does not change when the branch does. - Open, answered by documentation rather than by code: two picker bases composed in one plain labeled
Fieldnow collide on${id}-popoveras well as on the id itself. ThelabelTSDoc already says to give each anidof its own, and this is the same shapeInputBasehas had since #495, so it goes on the record rather than into a finding. The danglingforaninlinepicker leaves on the label is likewise unchanged and tracked in #1004. - One caveat on my evidence: the
Build and Testcheck was stillin_progresswhen I posted, so the rootscripts/suite and the docs-fence lint are covered only insofar asgate,Test and Preview Deploymentand both React-matrix jobs are green.
🏄 Clean little set wave, dude — three pickers finally paddle out with the name the label was shouting at them, and the gnarly
inlinerip got read right instead of fought. Only thing still floating out back is four old horizontal examples on the same two pages nobody roped in. Good to go once someone says where those are headed.
The Validation Error, Addons and Submit Row examples on the Input page and
the Select in Narrow Field example on the Select page hold the control in an
inner Field, which starts its own label scope, so the outer label named
nothing. Each now points the outer Field's label at the control with
labelProps={{ htmlFor }} and a matching id, the recipe the Context-Aware
Rendering examples already use. In the Addons example the label targets the
Input rather than the static country-code Button beside it.
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on 4dbe816. |
Preview DeploymentPreview URL: https://6e9a6294.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 1 resolved · 0 open
| # | Severity | Area | Finding | Location | Outcome |
|---|---|---|---|---|---|
| 1 | 🔵 Advisory | Accessibility | Four more horizontal examples left the control unnamed | docs/docs/api/form/input.md:643 |
✅ Verified fixed, resolved |
Overall: This pass settled threads and reviewed no commits. The single open advisory thread is resolved: 6fef2f65 wires all four examples it named — invalid-email, phone-number (targeting the Input, not the static +44 Button) and submit-row-name on the Input page, department-field on the Select page — and 5dd85eea carries the regenerated Input.json / Select.json. I confirmed the mechanism in source (Field.tsx:236-267 presence-semantics on htmlFor, the controlId = id ?? … resolution in useAutoLabelId, and Input.tsx:169 / Select.tsx:143 placing it after the props spread), checked that the ten example ids across the three pages are distinct so nothing collides when every tsx live fence renders on one page, and ran scripts/eslint-plugin-docs.test.mjs (77 pass), scripts/docs-fence-types.test.mjs (12 pass), pnpm check:conformance and pnpm gen:mcp:check — all green.
Residual risk: Not assessed — a verify pass settles open threads only and raises no findings. For a fresh look at the current code, post a steer comment starting deep-review: fresh and toggle the label.
🏄 All threads closed out clean, dude — those labels point at real controls now, and the generated index came along for the ride. Nothing left bobbing in the lineup.
github-actions
Bot
added
the
review-converged
allxsmith
added
the
needs-human-review
github-actions
Bot
removed
the
review-converged
📸 Story screenshots at handoff —
|
| Light | Dark |
|---|---|
 |
 |
form-field--with-help-text · Form/Field · With Help Text
| Light | Dark |
|---|---|
 |
 |
form-field--labels-custom-controls · Form/Field · Labels Custom Controls
| Light | Dark |
|---|---|
 |
 |
form-field--labels-composed-pickers · Form/Field · Labels Composed Pickers
| Light | Dark |
|---|---|
 |
 |
form-field--validation-states · Form/Field · Validation States
| Light | Dark |
|---|---|
 |
 |
form-field--label-sizes · Form/Field · Label Sizes
| Light | Dark |
|---|---|
 |
 |
github-actions
Bot
added
the
review-converged
# Conflicts: # bestax-mcp/data/components/Field.json # bulma-ui/src/form/Field.tsx # docs/docs/api/form/field.md
|
deep-review: fresh Main is merged in to clear a conflict with #1006 (range Slider thumb names) in |
github-actions
Bot
removed
the
review-converged
Preview DeploymentPreview URL: https://9e165fe9.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 1 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Accessibility | The Compound (dot-notation) example on this page leaves its Input unnamed — Field.Label is a div, and the Field passes no label prop; pre-existing, and published into Field.json |
docs/docs/api/form/field.md:492 (example at :476-484) |
Overall: The change is sound, and the merge resolution the steer asked about is correct on all four surfaces. id = idProp ?? (inline ? undefined : fieldLabelId) reproduces InputBase's convention exactly, hoisted to the top of the component only because popoverId derives from it, and all three bases return their native, inline and text paths exclusively, so no two inputs ever carry the adopted id. The merge kept both PRs' text everywhere I compared against main: the label TSDoc is main's sentence-for-sentence plus the picker-base list and the inline parenthetical; field.md's first accessibility bullet gained the bases and the inline sentence while #1006's range-Slider bullet survives intact beneath it; FormContext.tsx differs from main only in the FieldLabelIdContext doc, with the FieldLabelElementIdContext doc still carrying #981's Slider-thumb clause; and the form skill differs from main only by the composed-picker clause. The riskiest thing left is not in the code — it is that field.md itself was never swept for the unnamed-control shape the earlier round fixed on the three control pages, which is the one advisory. Human attention is best spent deciding whether that example is worth fixing here or in a follow-up.
Residual risk:
- The four picker-base paths (text input, native touch input, caller
idwins,inlinetakes nothing) — refuted empirically:Field.test.tsx155 pass, and theDateInput/TimeInput/DateTimeInput/DateRangeInput/FormContextsuites 546 pass. I read each render branch:DateInputBase.tsx:521(native, early return),:584(inline, early return) and:603(text) each carryid={id}at most once, andTimeInputBase/DateTimeInputBasehave the same shape. - Double adoption through nesting — refuted:
DateRangeInputBaserenders raw<input>s (:551,:596,:716), notDateInputBase, and nothing else inbulma-ui/srccomposes the three bases except their own convenience wrappers, which passid={controlId}explicitly after{...baseProps}(DateInput.tsx:183). A wrapper rendering its ownFieldsetslabelProps.htmlForby presence, so that Field hands out no id at all. - Derived popover ids now hang off the Field's generated id (
${id}-popover,${id}-cal) — two picker bases in one plain labeled Field would collide on the input id and on the popover id, where before this change each got an independentpicker-${reactId}. Refuted as a new hazard: it is the already-documented one ("Two controls in one plain labeled Field would both adopt the id, so give each anidof its own"), and no code resolves those ids through a selector — a grep forgetElementByIdandquerySelectoracrosssrc/form/_pickerInternals/returns only class, attribute anddata-*selectors, so a colon-bearinguseIdvalue cannot break a lookup either. - SSR / hydration — refuted:
useIsHydratedgatessupportsInputType, so the server and the first client render agree onhasNativeInput, and the adopted id is Field'suseId, stable across both. A later native/text switch moves the same id between inputs rather than duplicating it. - Other docs examples still unwired — refuted by a sweep of all of
docs/docsfor the outer-labeled-Fieldplus inner-Fieldshape, single-line and multi-linehorizontalalike: every one now carrieslabelProps. The twohorizontal label=without it (field.md:217,:222) hold the control directly, which keeps the association, andtextarea.md:174/dateinput.md:746are conveniencehorizontalprops that wire themselves. DateRangeInputBasein a labeled Field — open, not addressed: its group takes no name from the Field's label, and the label'sfordangles. Declared in the PR body and tracked in #1005, which I confirmed is open and describes exactly that.- Test vacuity — open, low — the inline assertion builds its selector from
labelEl(container).getAttribute('for') as string; wereFieldever to stop emittingforthere, the selector would become[id^="null"]and the test would pass without testing anything. Non-vacuous today:Field.tsx:236-239guarantees a generatedforfor a labeled, non-grouped Field, and that regression is covered many times over elsewhere in the file. - Generated artefacts and gates — refuted: re-ran
gen-mcp-index,gen-api-docs,gen-eslint-metaandgen-skills-rosters, andgit statuscame back clean;check-conformance.mjspasses every check, includingdocs-generated,skills-sync,story-per-componentandinline-style;typecheck:testsand prettier are clean on the changed files.bestax-mcp/data/skills.jsonis metadata only, so the skill prose edit needed no regeneration there.
🏄 Clean little cleanup set, dude — the pickers finally answer to their label, the merge kept both swells without wiping out #1006's Slider lines, and the tests actually ride the native path too. One dusty dot-notation example on the Field page is still paddling out unnamed, but nothing here blocks the set. Good to go.
Field.Label renders the label column, a div, and the Field in the Compound usage example has no label prop, so its "Name" named nothing and the Input had no accessible name. The example now puts a label.label inside Field.Label with htmlFor pointing at a matching id on the Input, and sizes the column is-normal, which is the markup the label prop renders for a horizontal Field. A sentence above it says why. The CompoundUsage story mirrors the example and gets the same change.
The "Horizontal field with explicit label/body" pattern put "Email" straight into Field.Label, which renders the label column (a div), so the InputBase had no accessible name and an agent copying the pattern built a form whose label names nothing. The pattern now puts a label.label inside Field.Label with htmlFor pointing at a matching id on the InputBase, with one sentence saying why. The SKILL.md composition tree, which said the label is rendered by Field.Label, now says it is a label you put inside it.
FieldLabel renders a div with the field-label class, so text placed straight in it names nothing, and its TSDoc example showed exactly that. The TSDoc now says so and points at the two ways to name the control: a label.label inside it with htmlFor at the control's id, or the horizontal Field's label prop. The example wires the label that way.
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on ddfb5d3. |
Preview DeploymentPreview URL: https://a76700b4.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 1 resolved · 0 open
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Accessibility | Compound (dot-notation) usage example named nothing — verified fixed, resolved | docs/docs/api/form/field.md:498 |
Overall: This pass settled threads and reviewed no commits. The one thread left open — the Field.Label compound example rendering bare text in a div.field-label, so the Input had no accessible name — is fixed on ddfb5d35. The example and the mirroring CompoundUsage story now put a <label className="label" htmlFor="compound-name"> inside <Field.Label size="normal"> with a matching id, which is the markup the horizontal label prop path emits (Field.tsx:233, :265-276), so the label weight and is-normal alignment match the other horizontal examples on the page. The same misreading was corrected at its two other homes — the bestax-form skill pattern for an explicit label/body, and the FieldLabel TSDoc, whose @example previously taught the unwired shape — and the generated Field.json carries the wired example. Nothing here is left for a human; a fresh review is what gets the current code looked at.
Residual risk: none identified for the settled thread — a grep for Field.Label and FieldLabel across docs/docs, skills and Field.stories.tsx leaves only the deliberately empty <Field.Label /> spacers (input.md:598, Field.stories.tsx:262), which hold no text to name, plus prose and the bestax-migrate maps. The bulma-ui Field suite is green locally (155 passed).
🏄 Paddled back out for the one wave still breaking, and it closed out clean — the label points at the right board now, the skill and the TSDoc quit telling folks the wrong line, and the generated stuff rode along. All threads settled, nothing left in the lineup.
github-actions
Bot
added
the
review-converged
|
🎉 This PR is included in version 5.27.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.16 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
The leftovers from #939, where a Field label still named nothing.
The picker bases.
InputBasecomposed in a labeledFieldtakes the id the label points at, so the label names it.DateInputBase,TimeInputBaseandDateTimeInputBasecomposed the same way didn't: the label'sforpointed at nothing and the input had no accessible name. They now read the Field's id the wayInputBasedoes and use it when you set noidof your own, on the text input and on the native input a touch device gets. The popover's ids build on it, the same as when the convenienceDateInputhands its id down. Aninlinepicker has no input, so it takes nothing from the Field and builds nothing on its id, which is the line #939 drew for the convenience pickers. The convenience wrappers behave as before: inside a Field of their own, their label always carries its ownhtmlFor, so that Field offers no id to take.The docs examples. The With Field Wrapper and With Field and Control Wrappers examples on the Input, Select and TextArea pages put the control in an inner
Fieldinside a horizontal Field's body. That inner Field starts its own label scope (#495), so the outer label'sformatched nothing and the control was unnamed. They now wire the label by hand withlabelProps={{ htmlFor }}and a matchingid, the recipe #939 put on the other control pages, with the same note above them.I looked at fixing that side in
Fieldinstead, so the markup as written would just work. The inner Field's scope is what stops a horizontal row of several inner Fields from handing one id to every control in it. Telling a lone inner Field from one of several at render time means either inspecting the body's children by type, which misses anything behind a wrapper component or a fragment, or having controls register themselves, which leaves the server render and first paint without the association. A label that names the control only while it has no siblings also seemed worse than wiring it explicitly. So the scope stays, and the examples wire it.Also in here:
labelTSDoc and the Field page's accessibility notes list the picker bases among what a labeled Field names, and say an inline picker takes nothing. The bestax-form skill says the same.src/form/CLAUDE.mdfor whoever adds the next base.Field > Controlis named by the label, on the native path too, a caller'sidstill wins, and an inline base takes no id from the Field and derives none from it. The hand-wired horizontal recipe is tested for Input, Select and TextArea, with and without the inner Control.Before changing anything I rendered every one of those examples and the composed bases on main, with #974 and #979 in, and checked the names with Testing Library. Each example had a
forthat matched nothing and an unnamed control, and each picker base was unnamed whereInputBasein the same spot was named.Left out:
DateRangeInputBasein a labeled Field. Its inputs keep their own Start date and End date names, but its group isn't pointed at the Field's label the way theDateRangeInputwrapper's is. That's tracked in #1005.pnpm allpasses locally.Fixes #968
Summary by CodeRabbit