fix: pagination keyboard controls#984
Conversation
🦋 Changeset detectedLatest commit: 6efe025 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Deploying solid-primitives with
|
| Latest commit: |
6efe025
|
| Status: | 🚫 Build failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.storybook/ui/controls.tsx:
- Around line 1-8: Update the Button props’ onClick and onKeyUp types to use
Solid’s JSX.EventHandlerUnion for HTMLButtonElement with the corresponding event
types, or reuse JSX.ButtonHTMLAttributes, so tuple-form handlers passed from
pagination remain assignable.
In `@packages/pagination/src/index.ts`:
- Around line 171-183: Update goPage’s parent lookup to target the pagination
component directly, using the nearest nav ancestor or an equivalent
pagination-specific data attribute instead of the childElementCount < 3
heuristic. Keep focus selection scoped to that component so multiple paginations
and single-page configurations focus the interacted component’s aria-current
page.
In `@packages/pagination/test/index.test.tsx`:
- Around line 197-227: Make the “sets the focus after going to a new page” test
failure-safe by wrapping setup, assertions, and event dispatches in try/finally.
In the finally block, synchronously dispose the rendered root, remove the test
container from document.body, and restore HTMLElement.prototype.focus, ensuring
cleanup occurs even when an expectation throws.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eeb323cd-bd54-4160-b350-8323d451bf09
📒 Files selected for processing (6)
.changeset/bright-tigers-switch.md.storybook/ui/controls.tsxpackages/i18n/src/index.tspackages/pagination/src/index.tspackages/pagination/stories/pagination.stories.tsxpackages/pagination/test/index.test.tsx
| import type { JSX } from "@solidjs/web"; | ||
| import { colors, font, radii } from "./tokens.js"; | ||
|
|
||
| export const Button = (props: { | ||
| onClick?: () => void; | ||
| "aria-current"?: "page"; | ||
| "aria-label"?: string; | ||
| onClick?: (ev: MouseEvent) => void; | ||
| onKeyUp?: (ev: KeyboardEvent) => void; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if TypeScript compilation passes with the spread
rg -n 'EventHandlerUnion' --type=ts packages/pagination/src/index.tsRepository: solidjs-community/solid-primitives
Length of output: 313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the button component and pagination prop usage
ast-grep outline .storybook/ui/controls.tsx --view expanded
printf '\n---\n'
ast-grep outline packages/pagination/src/index.ts --view expanded
printf '\n--- button usages ---\n'
rg -n 'Button\s*<|<Button|PaginationProps|onClick\??:|onKeyUp\??:' .storybook/ui/controls.tsx packages/pagination/src/index.tsRepository: solidjs-community/solid-primitives
Length of output: 4540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the Button component implementation and all call sites
sed -n '1,40p' .storybook/ui/controls.tsx
printf '\n---\n'
rg -n '<Button\b|Button\(' .storybook/ui -g '*.tsx' -g '*.ts'
printf '\n--- pagination prop construction ---\n'
sed -n '190,320p' packages/pagination/src/index.tsRepository: solidjs-community/solid-primitives
Length of output: 1452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any spread of pagination props into Button or other custom components
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'\.\.\.(paginationProps|pages\(\)|.*props)' .storybook packages
printf '\n---\n'
# Inspect the pagination tuple-handler construction in context
sed -n '200,315p' packages/pagination/src/index.tsRepository: solidjs-community/solid-primitives
Length of output: 6295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the pagination story and any Button wrapper usage around it
sed -n '1,120p' packages/pagination/stories/pagination.stories.tsx
printf '\n---\n'
sed -n '1,260p' .storybook/ui/primitives.tsx
printf '\n---\n'
sed -n '1,140p' .storybook/ui/controls.tsxRepository: solidjs-community/solid-primitives
Length of output: 14176
Widen Button's event handler props .storybook/ui/controls.tsx types onClick/onKeyUp as plain callbacks, but packages/pagination/src/index.ts passes Solid’s tuple handler form through <Button {...props} />. Make these props JSX.EventHandlerUnion<HTMLButtonElement, MouseEvent> / JSX.EventHandlerUnion<HTMLButtonElement, KeyboardEvent> (or reuse JSX.ButtonHTMLAttributes) so the pagination props stay assignable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.storybook/ui/controls.tsx around lines 1 - 8, Update the Button props’
onClick and onKeyUp types to use Solid’s JSX.EventHandlerUnion for
HTMLButtonElement with the corresponding event types, or reuse
JSX.ButtonHTMLAttributes, so tuple-form handlers passed from pagination remain
assignable.
| const goPage = (p: number | ((p: number) => number), ev: KeyboardEvent) => { | ||
| // select the parent before we might get detached | ||
| let parent = ev.currentTarget, current; | ||
| while (parent instanceof HTMLElement && parent.childElementCount < 3) parent = parent.parentNode; | ||
| setPage(p); | ||
| if ("currentTarget" in ev) | ||
| (ev.currentTarget as HTMLElement).parentNode | ||
| ?.querySelector<HTMLElement>('[aria-current="true"]') | ||
| ?.focus(); | ||
| flush(); | ||
| // select the current page | ||
| if (parent) { | ||
| while (parent instanceof HTMLElement && !(current = parent.querySelector('[aria-current="page"]'))) | ||
| parent = parent.parentNode; | ||
| current instanceof HTMLElement && current.focus(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
goPage parent-finding heuristic is fragile in edge cases.
The childElementCount < 3 walk-up works when the pagination container has 3+ buttons, but with maxPages=1 (and no showFirst/showPrev/etc.) the <nav> has a single child, causing the loop to walk to body. If multiple pagination components exist on the page, querySelector('[aria-current="page"]') from body would match the first one in DOM order — not necessarily the one the user is interacting with.
Consider using a more targeted traversal, e.g. closest <nav> ancestor or a data-* attribute on the pagination container, instead of the child-count heuristic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pagination/src/index.ts` around lines 171 - 183, Update goPage’s
parent lookup to target the pagination component directly, using the nearest nav
ancestor or an equivalent pagination-specific data attribute instead of the
childElementCount < 3 heuristic. Keep focus selection scoped to that component
so multiple paginations and single-page configurations focus the interacted
component’s aria-current page.
|
|
||
| test("sets the focus after going to a new page", async () => { | ||
| const originalFocus = HTMLElement.prototype.focus; | ||
| let current = document.body; | ||
| HTMLElement.prototype.focus = function() { | ||
| current = this; | ||
| originalFocus.call(this); | ||
| }; | ||
| const container = document.createElement('div'); | ||
| document.body.appendChild(container); | ||
| const Pagination = () => { | ||
| const [pagination] = createPagination({ pages: 10, maxPages: 5, initialPage: 1 }); | ||
| return <nav> | ||
| <For each={pagination()}> | ||
| {(props) => <button type="button" {...props} />} | ||
| </For> | ||
| </nav>; | ||
| }; | ||
| const dispose = render(() => <Pagination />, container); | ||
| const activeButton = container?.querySelector('nav button[aria-current="page"]'); | ||
| activeButton instanceof HTMLElement && activeButton.focus(); | ||
| for (var i = 0; i < 5; i++) { | ||
| await new Promise(r => setTimeout(r, 15)); | ||
| current?.dispatchEvent(new KeyboardEvent("keydown", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 })); | ||
| current?.dispatchEvent(new KeyboardEvent("keyup", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 })); | ||
| await new Promise(r => setTimeout(r, 45)); | ||
| expect(current.getAttribute("aria-current")).toBe("page"); | ||
| } | ||
| queueMicrotask(dispose); | ||
| HTMLElement.prototype.focus = originalFocus; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Test cleanup is not failure-safe — monkey-patched focus can leak across tests.
HTMLElement.prototype.focus is restored only at the end of the test body. If any expect throws, the prototype stays patched and the container div stays in document.body, potentially breaking subsequent tests. dispose is also deferred via queueMicrotask, so a failure means the Solid root is never disposed.
Wrap the test body in try/finally to guarantee restoration:
🛡️ Proposed fix for test cleanup
test("sets the focus after going to a new page", async () => {
const originalFocus = HTMLElement.prototype.focus;
let current = document.body;
HTMLElement.prototype.focus = function() {
current = this;
originalFocus.call(this);
};
- const container = document.createElement('div');
- document.body.appendChild(container);
- const Pagination = () => {
- const [pagination] = createPagination({ pages: 10, maxPages: 5, initialPage: 1 });
- return <nav>
- <For each={pagination()}>
- {(props) => <button type="button" {...props} />}
- </For>
- </nav>;
- };
- const dispose = render(() => <Pagination />, container);
- const activeButton = container?.querySelector('nav button[aria-current="page"]');
- activeButton instanceof HTMLElement && activeButton.focus();
- for (var i = 0; i < 5; i++) {
- await new Promise(r => setTimeout(r, 15));
- current?.dispatchEvent(new KeyboardEvent("keydown", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 }));
- current?.dispatchEvent(new KeyboardEvent("keyup", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 }));
- await new Promise(r => setTimeout(r, 45));
- expect(current.getAttribute("aria-current")).toBe("page");
- }
- queueMicrotask(dispose);
- HTMLElement.prototype.focus = originalFocus;
+ const container = document.createElement('div');
+ document.body.appendChild(container);
+ let dispose: (() => void) | undefined;
+ try {
+ const Pagination = () => {
+ const [pagination] = createPagination({ pages: 10, maxPages: 5, initialPage: 1 });
+ return <nav>
+ <For each={pagination()}>
+ {(props) => <button type="button" {...props} />}
+ </For>
+ </nav>;
+ };
+ dispose = render(() => <Pagination />, container);
+ const activeButton = container?.querySelector('nav button[aria-current="page"]');
+ activeButton instanceof HTMLElement && activeButton.focus();
+ for (let i = 0; i < 5; i++) {
+ await new Promise(r => setTimeout(r, 15));
+ current?.dispatchEvent(new KeyboardEvent("keydown", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 }));
+ current?.dispatchEvent(new KeyboardEvent("keyup", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 }));
+ await new Promise(r => setTimeout(r, 45));
+ expect(current.getAttribute("aria-current")).toBe("page");
+ }
+ } finally {
+ dispose?.();
+ HTMLElement.prototype.focus = originalFocus;
+ container.remove();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("sets the focus after going to a new page", async () => { | |
| const originalFocus = HTMLElement.prototype.focus; | |
| let current = document.body; | |
| HTMLElement.prototype.focus = function() { | |
| current = this; | |
| originalFocus.call(this); | |
| }; | |
| const container = document.createElement('div'); | |
| document.body.appendChild(container); | |
| const Pagination = () => { | |
| const [pagination] = createPagination({ pages: 10, maxPages: 5, initialPage: 1 }); | |
| return <nav> | |
| <For each={pagination()}> | |
| {(props) => <button type="button" {...props} />} | |
| </For> | |
| </nav>; | |
| }; | |
| const dispose = render(() => <Pagination />, container); | |
| const activeButton = container?.querySelector('nav button[aria-current="page"]'); | |
| activeButton instanceof HTMLElement && activeButton.focus(); | |
| for (var i = 0; i < 5; i++) { | |
| await new Promise(r => setTimeout(r, 15)); | |
| current?.dispatchEvent(new KeyboardEvent("keydown", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 })); | |
| current?.dispatchEvent(new KeyboardEvent("keyup", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 })); | |
| await new Promise(r => setTimeout(r, 45)); | |
| expect(current.getAttribute("aria-current")).toBe("page"); | |
| } | |
| queueMicrotask(dispose); | |
| HTMLElement.prototype.focus = originalFocus; | |
| }); | |
| test("sets the focus after going to a new page", async () => { | |
| const originalFocus = HTMLElement.prototype.focus; | |
| let current = document.body; | |
| HTMLElement.prototype.focus = function() { | |
| current = this; | |
| originalFocus.call(this); | |
| }; | |
| const container = document.createElement('div'); | |
| document.body.appendChild(container); | |
| let dispose: (() => void) | undefined; | |
| try { | |
| const Pagination = () => { | |
| const [pagination] = createPagination({ pages: 10, maxPages: 5, initialPage: 1 }); | |
| return <nav> | |
| <For each={pagination()}> | |
| {(props) => <button type="button" {...props} />} | |
| </For> | |
| </nav>; | |
| }; | |
| dispose = render(() => <Pagination />, container); | |
| const activeButton = container?.querySelector('nav button[aria-current="page"]'); | |
| activeButton instanceof HTMLElement && activeButton.focus(); | |
| for (let i = 0; i < 5; i++) { | |
| await new Promise(r => setTimeout(r, 15)); | |
| current?.dispatchEvent(new KeyboardEvent("keydown", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 })); | |
| current?.dispatchEvent(new KeyboardEvent("keyup", { bubbles: true, charCode: 0, code: "ArrowRight", key: "ArrowRight", keyCode: 39 })); | |
| await new Promise(r => setTimeout(r, 45)); | |
| expect(current.getAttribute("aria-current")).toBe("page"); | |
| } | |
| } finally { | |
| dispose?.(); | |
| HTMLElement.prototype.focus = originalFocus; | |
| container.remove(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/pagination/test/index.test.tsx` around lines 197 - 227, Make the
“sets the focus after going to a new page” test failure-safe by wrapping setup,
assertions, and event dispatches in try/finally. In the finally block,
synchronously dispose the rendered root, remove the test container from
document.body, and restore HTMLElement.prototype.focus, ensuring cleanup occurs
even when an expectation throws.
2 participants
When navigating the pagination with the keyboard, we could get into the situation where a button/link that previously had focus would be detached by the page switch, thus no longer having a parent reference to focus the current page. This is now fixed. Also, the stories broke whenever one switched the parameters. This is now rectified, too.
Addresses #286
Summary by CodeRabbit
Bug Fixes
Accessibility
Tests