Repository navigation
fix(bulma-ui): name a composed DateRangeInputBase's group from a Field label - #1015
Conversation
…d label DateRangeInputBase composed in a labeled Field now points its role="group" root's aria-labelledby at that Field's label, through useAutoLabelledBy, the way the DateRangeInput wrapper already does. It does it in every mode, inline and native included, since the group renders in all of them, and each input keeps its own "Start date" or "End date" name. An aria-label or aria-labelledby the caller gives the group still wins. The wrapper always passes an aria-labelledby key of its own, so the base leaves the group to it and the wrapper's naming doesn't change.
The DateRangeInput and Field pages and the bestax-form skill now count a DateRangeInputBase composed in a labeled Field among the groups the label names through aria-labelledby, inline or not.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 50 minutes. View limit details
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 |
Preview DeploymentPreview URL: https://741aaa40.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 · 0 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| — | — | — | No blocking defects found. | — |
Overall: The change is sound and tightly scoped: DateRangeInputBase now calls useAutoLabelledBy with no label of its own and renders aria-labelledby ahead of {...cleanRest} on all three of its role="group" roots (native bulma-ui/src/form/DateRangeInputBase.tsx:603, inline :676, popover :725) — exactly the placement Radios.tsx:151, Checkboxes.tsx:149 and Rate.tsx:550 already use, so the caller-wins and presence-of-aria-labelledby semantics from #974 come for free rather than being re-implemented. The riskiest part was a regression in the DateRangeInput wrapper, since the wrapper's naming previously worked only through the spread — on main the base emitted no aria-labelledby of its own at all (verified: git show origin/main:.../DateRangeInputBase.tsx has zero matches, which is also the bug #1005 reports). That is closed by the wrapper always passing an aria-labelledby key, which the hook's presence check honours, and by the new wrapper test covering both the outer-Field and own-label paths. A human should focus on whether naming the group is the right answer for inline, where the group wraps only a calendar and no input — the PR argues it is because the group element exists in that mode, unlike the single-input bases, which I confirmed render a roleless <div> inline (DateInputBase.tsx:586).
Verified here: DateRangeInput.test.tsx + Field.test.tsx (255 passed) and typecheck:tests (clean). This session's sandbox refused file writes, so I could not add scratch probe tests; the scenarios below were settled by tracing the real code paths plus the repo's own suites, and the React 18/19 matrix jobs are green on this SHA.
Residual risk:
- Another group component still missing the hook. Refuted —
role="group"/"radiogroup"appears only inRadios,Checkboxes,RateandDateRangeInputBase(plus prose in theDateRangeInputwrapper), and all four now calluseAutoLabelledBy. The failure class is closed for groups. callerProps: restdiverging from the spreadcleanRest. Refuted —useBulmaClassesdestructures only Bulma helper props and never touchesaria-*(noariamatch anywhere inhelpers/useBulmaClasses.tsx), so the presence ofaria-label/aria-labelledbyis identical in both objects.- Id churn from the extra
useId(). Refuted —reactId/popoverIdare computed before the new hook call, and the hook's own generated id is unused whenlabelis undefined, so no existing id oraria-controlstarget shifts. No early return precedes the hook, so hook order is stable in every mode. - A render path left uncovered. Refuted — the component has exactly three returns (
useNative,inline, default); all three carry the attribute and all three are asserted by the newit.eachinDateRangeInput.test.tsx. grouped/hasAddons, or alabelProps.htmlForthe caller wired, leaving the group unnamed. By design and unchanged:Fieldpublishes the label element id only whentargetIdis set (Field.tsx:335), the same gate every other group sits behind, anddocs/docs/api/form/field.md:501states it.- Open, not fixed here: the Field's
<label for>still points at a generated id nothing adopts, so clicking the label focuses neither input and a validator sees a danglingfor. Pre-existing for every group in a labeledField, stated on the Field page, and tracked by #1004 — and still strictly better thanmain, where the composed base got no name at all.
🏄 Four lines of attribute plus one hook call, dropped right into the lane the other groups were already surfing — no wipeouts, the wrapper never even wobbled, and the tests ride all three modes. Clean set, good to go.
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
|
🎉 This PR is included in version 5.27.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The merge of main took this page whole from the branch side, which dropped #1015's addition to the hand-written group bullet.
|
🎉 This PR is included in version 1.14.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
The
DateRangeInputBasepiece left over from #968.<Field label="Stay"><DateRangeInput /></Field>names the range's group "Stay": the wrapper points itsrole="group"at the Field's label througharia-labelledby.DateRangeInputBasecomposed the same way, in a labeledFieldandControl, didn't. Its Start date and End date inputs kept their own names, but the group had no name at all.Before changing anything I rendered it on main with Testing Library, next to the wrapper and
Radiosin the same spot. The base's group had noaria-labelledbyon the popover path, the native path orinline, while the wrapper andRadiosboth pointed at the label's id. Anaria-labelon the base did name it, so nothing was in the way of the caller's own name.Now the base calls
useAutoLabelledBywith no label of its own and its remaining props ascallerProps, the wayRadios,Checkboxes,Rateand the wrapper do, and puts the result on its group root ahead of the props spread. That covers every mode,inlineand native included, since the group renders in all of them. It's different from the single-input picker bases, which skip the Field inline because they have no input to give the id to. Here the Field's label names the group, not an input, and the group is still there. Each input keeps its own name.A caller's
aria-labeloraria-labelledbystill wins, as #974 set for groups, and anaria-labelledbykey counts even when it's undefined, since it spreads over the group's own. That's also why the wrapper doesn't change: it always hands the base anaria-labelledbykey of its own, so the base leaves the group to it and the wrapper keeps deciding between its own label and a surrounding Field's.About the label's
for: in this spot it still matches nothing. That's true today for the wrapper,Radiosand every other group in a labeled Field too, and the Field page already says so. The group pattern doesn't avoid it, and the general fix, letting content that takes nofortell its Field, is #1004. Giving the start input the Field's id would have matched thefor, but it would tie the label to one end of the range rather than to the group, and the base would behave differently from the wrapper.Also in here:
labelTSDoc, the Field page's accessibility notes and the DateRangeInput page's accessibility notes count a composedDateRangeInputBaseamong the groups a labeled Field names,inlineor not. The bestax-form skill says the same.DateRangeInputBasesaying it takes a surrounding Field's label and that a caller's name wins, and a line insrc/form/CLAUDE.mdfor the next base that is a group.DateRangeInputBaseunder a "Stay" label.Field > Controlis named by the label on the popover path, the native path andinline, and each input keeps its own name. A caller'saria-labeloraria-labelledbywins, and so does an undefinedaria-labelledbykey, which is what the wrapper passes. The base takes no name from an unlabeled Field. The wrapper still names its group from the outer Field's label, or from its own. The Field suite's shared group tests (named in a Field and in a Control, caller names winning, an undefinedaria-labelpassed through) now run over the base as well.pnpm allpasses locally.Fixes #1005