Conversation
The thumb's aria-controls was hardcoded to this mount's useId value, but the sync effect only stamps that id onto the scroll container when the container has none. When the host app supplies its own id, or when the component remounts onto a container a previous mount already labelled, aria-controls names an element that does not exist and assistive tech cannot resolve the scrollbar's target. Track the id actually present on the element instead.
The unit test drives the remount directly against PinnedScrollbar. Exercise the same path through DataTable's public API, where unpinning every column drops hasPinnedColumns and re-pinning mounts a fresh scrollbar over the wrapper the previous mount already labelled.
The sync effect returns at `if (!el)` before it can stamp an id or set controlsId, so no thumb is rendered and no control reference is emitted. Green on both source arms - it pins the early-return row of the boundary ledger, which had no coverage of its own.
✅ Deploy Preview for react-data-table-component ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jbetancur
left a comment
jbetancur
left a comment
There was a problem hiding this comment.
@askalf Thank you for identifying this bug! Just a few comments to address. Also please update the CHANGELOG.md. One generate nit is to avoid those em dashes if you can. Not a blocker and I will add this to the claude.md in the future
| }); | ||
|
|
||
| // Control: two scrollbars on the page at once, each on its own container. | ||
| // Green before and after — the id each thumb points at is per-instance state, |
There was a problem hiding this comment.
I know this is what the AI generated but can we clean this comment up. Like Green before and after. just indiciate what the tests accomplishes
There was a problem hiding this comment.
Done in 9913338: the comments now say what each test checks, nothing about how it was written.
| }); | ||
|
|
||
| // Control: the container has no id of its own, so the effect stamps it with | ||
| // this mount's useId and aria-controls matches either way. Green before and |
There was a problem hiding this comment.
I know this is what the AI generated but can we clean this comment up. Like Green before and after. just indiciate what the tests accomplishes
There was a problem hiding this comment.
Done in 9913338: the comments now say what each test checks, nothing about how it was written.
| rightInset, | ||
| }: PinnedScrollbarProps): JSX.Element | null { | ||
| const scrollContainerId = React.useId(); | ||
| // The id the thumb points at. Only equals scrollContainerId when this |
There was a problem hiding this comment.
This is kind of a nit and won't block the PR - and I should put it in the claude.md but try to remove em dashes.
There was a problem hiding this comment.
Done in 9913338, no em dashes left in the diff. CHANGELOG entry is in the same push.
Review feedback: the two control tests' comments now say only what each test pins and that it is green before and after the fix; every em dash this change introduced is gone; and the fix has a CHANGELOG entry under 8.10.0 / Bug fixes.
|
Thanks for the review. All three addressed in the latest push:
|
Containers the PinnedScrollbar tests append to document.body are now removed in afterEach rather than at the end of each test, so a failed assertion cannot leave them behind for the next test. The comments say what each case pins; the empty-ref case is synchronous; the changelog bullet sits under its heading with the blank line where it was.
|
|
||
| ### Bug fixes | ||
|
|
||
| - **Pinned scrollbar** now sets `aria-controls` to the scroll container's real id. When the container already carried an id, or was labelled again after an unpin and re-pin, the thumb kept pointing at a generated id no element had, so assistive technology could not resolve what the scrollbar controlled. |
There was a problem hiding this comment.
Sorry I should have been more clear. This needs to land in a new entry for 8.10.1 since 8.10.0 was already released
The repo's curly rule requires braces on every for-of, and CI lint failed on the one-line cleanup loop. attach now returns the element type it was given, so the swap harness gets an HTMLDivElement back and tsc --noEmit is green alongside lint.
|
Updated the test-only cleanup after the CI report: the |
This branch has not been deployed
2 participants
Summary
PinnedScrollbarused this mount'sReact.useId()foraria-controls, but assigned that id to the scroll container only whenel.idwas empty.scrollRefswap—therefore left the thumb pointing at a nonexistent id.el.idafter the existing stamp-or-preserve branch and rendersaria-controls={controlsId}. It changes 10 source lines and no public API.Decisions
PinnedScrollbarnow keeps acontrolsIdstate initialized fromscrollContainerId; in the existing synchronization effect,setControlsId(el.id)is called immediately after the existingif (!el.id) { el.id = scrollContainerId; }stamp-or-preserve branch, andaria-controls={controlsId}is rendered instead of the rawscrollContainerId. That way the element's actual id is used whether it was freshly stamped or already present.Rejected alternatives: mutating a ref directly wouldn't trigger a re-render of the thumb; overwriting
el.idunconditionally would clobber host-owned DOM attributes; adding aMutationObserverto track id changes during an already-active mount is a different, unrelated concern and is out of scope for this fix.Full package suite (
npm test, 23 files / 726 tests) andnpx tsc --noEmitboth pass with the fix; not run: any real-browser/layout-specific check, since the defect and its tests are DOM id/reference resolution, fully exercised under jsdom.AI assistance: this bug was found and the fix and tests were drafted with AI tooling in my workflow; the tests and checks above were executed as pasted. I'm responsible for the change and will handle review feedback.