Repository navigation
fix(bulma-ui): name each range Slider thumb from its label - #1006
Conversation
A range thumb's default aria-label outranked the label's for, so neither the Slider's own label nor a surrounding Field's reached a thumb's name and every range slider announced "Minimum value" and "Maximum value". Each thumb now points aria-labelledby at the label and at a hidden span holding its own text, so it reads as "Price range Minimum value". An ariaLabel entry is still that thumb's whole name, and an aria-label or aria-labelledby the caller spreads onto the low thumb still wins there. The label's for keeps landing on the low thumb. Fixes #981
Adds a labeled range example to the Slider page, rewrites its Accessibility notes for the new thumb names and how ariaLabel and a hand-wired Field label interact with them, notes the range Slider on the Field page, and teaches the form skill the same.
|
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 26 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (15)
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://44db27b5.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 · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | API | A caller's aria-label/aria-labelledby renames only the low thumb, so the pair reads with no shared stem ("Budget floor" / "Price Maximum value") — deliberate per the PR body, on the record |
bulma-ui/src/form/Slider.tsx:373 |
| 2 | 🔵 Advisory | Coverage | The 'aria-labelledby' in callerAria presence semantics are unenforced — only a defined value is tested, so a truthiness rewrite stays green; the group path pins this at Field.test.tsx:912 |
bulma-ui/src/form/__tests__/Slider.test.tsx:1426 |
Overall: The change is sound and the mechanism is the right one. I chased #981's claim to its source and confirmed the failure shape (a range thumb's unconditional aria-label outranking label[for]), then confirmed the fix empirically: the full src/form suite is green (2827 tests), tsc -p tsconfig.test.json is clean, and coverage on the three touched files is 100% statements/lines, with useAutoLabelId.ts and Field.tsx at 100% branches. Putting the wiring in useAutoLabelId rather than in Slider follows this folder's stated rule, and needsLabelId defaults to false, so every other caller's behaviour is byte-identical (ownLabelId stays undefined, and fieldLabelProps collapses to the old ownLabelProps). The riskiest part is the aria-labelledby → <span hidden> technique, since jsdom cannot tell you what a real screen reader does; that is where a human should look first, and the second place is the precedence table, which is now four-way (own label / Field label / ariaLabel entry / spread aria-*) and asymmetric across the two thumbs.
Residual risk:
<span hidden>as a name part. Refuted as a spec matter: AccName 1.2 step 2A skips a hidden node only when it is not directly referenced byaria-labelledby, which is exactly what the code's comment claims, and the tests resolve "Price range Minimum value" throughdom-accessibility-api. Refuted as a CSS matter too:[hidden] { display: none }is a UA rule, and nothing inscss/form/_slider.scsssetsdisplayon a bare descendant — the only authordisplaydeclarations are on.slider,.slider-inputand.slider-output. The vertical modifier makes the wrapperinline-flex, which does not resurrect adisplay: nonechild, since such a child generates no box and is never a flex item.- DOM-position fragility from the two new spans. Refuted:
_slider.scsscontains no+,~,:nth-child,:first-childor:last-childselector, andSlider.tsxdoes nochildren/nextElementSibling/querySelectortraversal — tooltip placement is computed fromprogressandwrapperRef.getBoundingClientRect(). - Dangling IDREFs. Refuted by construction:
Fieldonly publishesFieldLabelElementIdProvider value={targetId ? labelId : undefined}, andtargetIdis non-undefinedonly when it also renders the<label id={labelId}>; the own-label path handsownLabelIdto that same<label>throughfieldLabelProps. The spans are emitted under the identicalwiredflag that produces the reference. - Presence-vs-truthiness on the caller's keys. Refuted for
rest:useBulmaClassesderives it by object rest destructuring (useBulmaClasses.tsx:167), which preserves a key whose value isundefined, so'aria-labelledby' in callerAriabehaves as intended. Left open only as the test gap in finding 2. - Open — a hand-wired
Fieldlabel still leaves a range Slider's thumbs indistinguishable.Fieldpublishes no label element id whenlabelProps.htmlForis present, even when the caller also suppliedlabelProps.id, so<Field label="Price" labelProps={{ htmlFor: 'price', id: 'price-label' }}>keeps "Minimum value"/"Maximum value". This is pre-existing (the #939 group path has the same gap), is pinned by a test, and the Slider docs prescribeariaLabelthere — but it is the shape of the Slider page's own horizontal "Inside Field" example, so a reader following that pattern withrangereproduces #981's symptom. The PR scopes it out without naming a tracking issue. - Open — two range Sliders in one plain labeled
Field. Both low thumbs adopt the same generated id and all four thumbs resolve to the same two names, which is #981's exact failure mode. Pre-existing and documented on the Field page ("Two controls in one plain labeled Field would both adopt the id");grouped/hasAddonsfields likewise publish no id, so a range Slider there keeps the defaults. - Generated artefacts.
skills.json'spatterns.mdbyte count (7058) matches the file on disk, andcatalog.json's SliderexampleCountmoves 11 → 13 for the two new docs examples. The two new MCP examples share the title "Range Slider" because they sit under one###heading — harmless, sinceget_examplesfilters rather than looks up, andSlider.jsonalready shipped a duplicate title ("Context-Aware Rendering") before this PR. - Unverified by me:
pnpm check:conformance,gen:mcp:checkand the React 18 half of the matrix — those commands were not runnable in this session, so I am relying on the PR's "pnpm allpasses locally" plus CI for them. Nothing in the diff looks likely to trip them: no inlinestyle={{}}in the new stories or docs examples, and no React-19-only API (useIdis 18+).
🏄 Clean little set of waves here — the label finally rides all the way out to both thumbs instead of wiping out on a hardcoded
aria-label, and the tests actually paddle out and check it. Only froth left is that one gnarly asymmetry when you spread your ownaria-label, plus a missing test on the opt-out. Good to go.
… range Slider thumbs An `aria-label` or `aria-labelledby` passed to a range Slider reached only the low thumb, through the rest spread, so it renamed that thumb while the high one kept the label's name: `<Slider range label="Price" aria-label="Budget floor" />` read "Budget floor" and "Price Maximum value", two unrelated names for one widget. The Slider now routes those two attributes itself instead of spreading them. Either one names the Slider in the label's place, so both thumbs read it before their own text: "Budget Minimum value" and "Budget Maximum value". An `aria-labelledby` reaches the thumbs the way the label does, through `aria-labelledby` and the hidden span, and an `aria-label` is joined to each thumb's text. A non-empty `aria-labelledby` wins over an `aria-label`, as it does in the browser. An `ariaLabel` entry is still its thumb's whole name and wins over both. The presence rule from groups (#974) carries over: any `aria-labelledby` key, even an undefined one, takes the naming over, so `aria-labelledby={undefined}` keeps every label out and leaves both thumbs "Minimum value" and "Maximum value". An empty `aria-label` still counts as unset. Single-thumb Sliders keep spreading the caller's attributes as before.
The range Slider decides whether the caller named it the way a group does (#974): any `aria-labelledby` key counts, even an undefined one, while an `aria-label` counts only when it is non-empty. Every test so far passed a defined `aria-labelledby`, which a plain truthiness check satisfies just as well, so swapping the presence test for one would have kept the suite green while dropping the opt-out. Pin both sides. `aria-labelledby={undefined}`, under the Slider's own label and under a surrounding Field's, leaves both thumbs "Minimum value" and "Maximum value" with no `aria-labelledby` and no hidden spans, while the label's `for` still lands on the low thumb. An empty `aria-label` leaves the label in both names.
…lider thumbs
The Slider page's Accessibility section said an `aria-label` or
`aria-labelledby` passed to a range Slider lands on the low thumb and wins
there. It now names the whole Slider, in the label's place, so the section says
that, with the example names, and gives `aria-labelledby={undefined}` as the way
to keep a label out of both thumbs' names. The Field page's range Slider line
gains the same clause, and both pages' Props tables pick up the new TSDoc.
|
deep-review: verify Every open thread has a reply: fixed, with the commit, or refuted with a reason. This settles them on 908544c. |
Preview DeploymentPreview URL: https://fb4deba3.bestax.pages.dev |
There was a problem hiding this comment.
Deep review (verify) — 2 resolved · 0 open
| # | Severity | Area | Thread | Location | Outcome |
|---|---|---|---|---|---|
| 1 | 🔵 Advisory | API | A caller's aria-label/aria-labelledby renamed only the low thumb, leaving a mismatched pair |
bulma-ui/src/form/Slider.tsx:373 |
✅ Verified fixed (6f82455, b8846f0) |
| 2 | 🔵 Advisory | Coverage | Nothing pinned the aria-labelledby presence semantics on a range thumb |
bulma-ui/src/form/__tests__/Slider.test.tsx:1488 |
✅ Verified fixed (46f7cd4) |
Overall: This pass settled threads and reviewed no commits. Both previously-open advisory threads are addressed in the current code and are now resolved. Thread 1 was answered with a code change rather than only the documentation line it asked for: Slider.tsx:351-382 destructures a caller's aria-label/aria-labelledby out of rest, derives a single stemRef/stemText, and feeds it to nameThumb for both thumbs, while thumbRest keeps the low-thumb-only spread for everything else — so a range Slider can no longer announce two unrelated names. Thread 2's missing case is now an it.each at Slider.test.tsx:1515 covering aria-labelledby={undefined} under both the Slider's own label and a surrounding Field label, with a companion test at :1543 pinning that an empty aria-label counts as unset; together they fence both halves of the presence-vs-truthiness rule. Verified empirically: 286 Slider + Field tests pass on 908544c.
Residual risk: not assessed — a verify pass settles open threads and raises no new findings. Ask for a fresh review (a steer starting deep-review: fresh) if you want the commits pushed since the original review looked at on their own.
🏄 Clean set, both waves ridden out — the author did not just paddle around the mismatched-name problem, he turned and took it. Threads closed, tests green, 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--validation-states · Form/Field · Validation States
| Light | Dark |
|---|---|
 |
 |
form-field--label-sizes · Form/Field · Label Sizes
| Light | Dark |
|---|---|
 |
 |
form-field--horizontal · Form/Field · Horizontal
| Light | Dark |
|---|---|
 |
 |
form-slider--default · Form/Slider · Default
| Light | Dark |
|---|---|
 |
 |
form-slider--with-output · Form/Slider · With Output
| Light | Dark |
|---|---|
 |
 |
form-slider--colors · Form/Slider · Colors
| Light | Dark |
|---|---|
 |
 |
form-slider--sizes · Form/Slider · Sizes
| Light | Dark |
|---|---|
 |
 |
form-slider--rounded · Form/Slider · Rounded
| Light | Dark |
|---|---|
 |
 |
form-slider--circle · Form/Slider · Circle
| Light | Dark |
|---|---|
 |
 |
github-actions
Bot
added
the
review-converged
|
🎉 This PR is included in version 5.27.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.15 🎉 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
A range Slider never let its label into a thumb's name. Each thumb carried an
aria-label, either fromariaLabelor the "Minimum value" / "Maximum value" defaults, andaria-labelbeatslabel[for]. So<Slider range label="Price range" />and<Field label="Price range"><Slider range /></Field>both announced plain "Minimum value" and "Maximum value", and a form with two range sliders gave a screen reader user no way to tell their thumbs apart. The label'sforonly ever reached the low thumb anyway, and even there it lost.Now, when a label names a range Slider, each thumb points
aria-labelledbyat that label and at a small hidden span holding the thumb's own text. The names come out as "Price range Minimum value" and "Price range Maximum value". A hidden element still counts whenaria-labelledbypoints straight at it, so the spans need no CSS and render nothing. This works the same for the Slider's ownlabelprop and for a surroundingField's label, including through aControl.The wiring lives in
useAutoLabelIdrather than in Slider, since the form folder's rule is that inputs go through the label hooks instead of juggling ids themselves. It takes a newneedsLabelIdoption and hands back the id of whichever label names the control: its own label (which getslabelProps.idor a generated id) or the Field's label id that #974 already provides for groups. The hook only returns one when that label actually names the control, so a Slider with its ownidinside a labeled Field, or a label opted out withhtmlFor: undefined, keeps the old names.On precedence, I kept the caller in charge:
ariaLabelentry is that thumb's whole name, with no label in front. Someone who writesariaLabel={['Lowest price', 'Highest price']}has said exactly what they want read. Each entry stands on its own, and an empty one counts as unset.aria-labeloraria-labelledbypassed straight to the Slider names the whole Slider, taking the label's place at the start of both thumbs' names:aria-label="Budget"reads "Budget Minimum value" and "Budget Maximum value". A non-emptyaria-labelledbybeatsaria-label, as it does in the browser, andaria-labelledby={undefined}keeps every label out, so both thumbs read just "Minimum value" and "Maximum value". Before this, a caller's attribute rode the rest props onto the low thumb alone and left the pair named two different ways.The label's
forstill points at the low thumb, so clicking the label focuses it like before and nothing is left pointing at an element that isn't there. Single-thumb Sliders are untouched: noaria-labelledby, no id on the label, no spans.One case stays as it was. A Field label wired by hand with
labelProps={{ htmlFor }}, like the horizontal layouts in the Slider docs, hands out no label id, so a range Slider there keeps its default thumb names. The docs say so and point atariaLabelfor that layout. Teaching Field to pass its label id through a hand-wired setup felt like its own change.Worth knowing for anyone's tests:
getByLabelText('Price range')on a range Slider now matches both thumbs, so it needsgetAllByLabelText, orgetByRole('slider', { name: 'Price range Minimum value' }).Docs: the Slider page gets a labeled range example plus one with
ariaLabel, and its Accessibility section is rewritten around the new names. The Field page and itslabel/labelPropsTSDoc mention the range Slider, and the form skill teaches it too (its range pattern now has a label). There are two new stories, a range Slider with its own label and one inside a labeled Field.Tests cover both label sources,
ariaLabelwinning over each, a caller'saria-labelandaria-labelledbywinning on the low thumb,labelProps.idon both the Slider and the Field, the hand-wired and opted-out cases keeping the defaults, and single thumbs staying as they were.pnpm allpasses locally.Fixes #981