fix: resolve keyboard navigation and tooltip issues for Pie charts (#6921)#7140
Merged
PavelVanecek merged 3 commits intoMar 22, 2026
Merged
Conversation
…#6921) - Changed keyboardEventsMiddleware to use selectTooltipEventType instead of hardcoding 'axis' - Fixes keyboard navigation for Pie charts which use 'item' tooltip type - All existing keyboard navigation tests still pass Partially fixes recharts#6921 - keyboard navigation now uses correct event type for all chart types. Full resolution requires additional work to initialize tooltip state on focus without defaultIndex.
Contributor
WalkthroughReplaces hard-coded 'axis' with a selector-derived Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant DOM as Chart DOM
participant KB as KeyboardEventsMiddleware
participant Sel as Selectors (selectTooltipEventType, others)
participant Store as Redux Store
participant Tooltip as Tooltip renderer
User->>DOM: keydown (Enter / Arrow)
DOM->>KB: keyboard event
KB->>Sel: selectTooltipEventType(state)
KB->>Store: read displayedData, tooltipTicks
KB->>KB: compute dataLength, nextIndex, coordinate (using tooltipEventType)
KB->>Store: dispatch activeCoordinate / activeIndex
Store->>Tooltip: update tooltip payload/position
Tooltip->>DOM: render tooltip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/state/keyboardEventsMiddleware.ts (1)
71-99: Add regression tests for the new keyboard paths.Please add focused tests for: (1) empty
tooltipTickswith non-emptydisplayedData, and (2) focus initialization foritemtooltip charts (Pie). This change is behavior-critical and currently relies on manual verification.As per coding guidelines:
src/**/*.{ts,tsx}: Aim for 100% unit test code coverage when writing new code.Also applies to: 150-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/keyboardEventsMiddleware.ts` around lines 71 - 99, Add unit tests for keyboardEventsMiddleware to cover the two missing keyboard paths: (1) when tooltipTicks is empty but selectTooltipDisplayedData returns a non-empty array ensure arrow key navigation uses displayedData length and prevents out-of-bounds movement, and (2) focus initialization for item tooltip charts (e.g., Pie) where pressing Enter sets keyboardInteraction.activeIndex/activeCoordinate via selectCoordinateForDefaultIndex; exercise selectors selectTooltipDisplayedData, selectTooltipEventType, selectCoordinateForDefaultIndex and the dispatched action setKeyboardInteraction to assert correct payloads and that behavior differs for 'hover' vs item/tooltips.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/keyboardEventsMiddleware.ts`:
- Around line 94-95: The computation of dataLength in
keyboardEventsMiddleware.ts wrongly uses the nullish coalescing operator
(tooltipTicks?.length ?? displayedData.length) so an empty tooltipTicks (length
0) prevents falling back to displayedData; change it to use a truthy fallback
such as (tooltipTicks?.length || displayedData.length) or an explicit
conditional (tooltipTicks && tooltipTicks.length > 0 ? tooltipTicks.length :
displayedData.length) so nextIndex navigation uses displayedData when
tooltipTicks is empty; update the line where dataLength is defined and keep
references to tooltipTicks, displayedData, and nextIndex.
- Around line 72-76: Remove the unused tooltipEventType parameter from the
selectCoordinateForDefaultIndex selector and all its call sites (currently
passing tooltipEventType in calls near keyboard handling and other usages);
update the selector's combiner function combineCoordinateForDefaultIndex to drop
the tooltipEventType argument and adjust its parameter list and internal
references accordingly, and then remove the extra argument from every
selectCoordinateForDefaultIndex(...) invocation so the selector only receives
the actual state-derived inputs it uses (chart dimensions, layout, axis ticks,
tooltip settings).
---
Nitpick comments:
In `@src/state/keyboardEventsMiddleware.ts`:
- Around line 71-99: Add unit tests for keyboardEventsMiddleware to cover the
two missing keyboard paths: (1) when tooltipTicks is empty but
selectTooltipDisplayedData returns a non-empty array ensure arrow key navigation
uses displayedData length and prevents out-of-bounds movement, and (2) focus
initialization for item tooltip charts (e.g., Pie) where pressing Enter sets
keyboardInteraction.activeIndex/activeCoordinate via
selectCoordinateForDefaultIndex; exercise selectors selectTooltipDisplayedData,
selectTooltipEventType, selectCoordinateForDefaultIndex and the dispatched
action setKeyboardInteraction to assert correct payloads and that behavior
differs for 'hover' vs item/tooltips.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e89b0c8-c0ab-4443-bd2f-7f7aa82e0fd0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/state/keyboardEventsMiddleware.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/state/keyboardEventsMiddleware.ts (1)
131-132:⚠️ Potential issue | 🟠 Major
dataLengthfallback is still wrong whentooltipTicksis an empty array.The nullish coalescing operator (
??) only replacesnull/undefined, not0. WhentooltipTicksis[],tooltipTicks?.lengthreturns0, and0 ?? displayedData.lengthevaluates to0—notdisplayedData.length. This prevents keyboard navigation even whendisplayedDatahas items.Proposed fix
- const dataLength = tooltipTicks?.length ?? displayedData.length; + const dataLength = tooltipTicks?.length || displayedData.length;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/keyboardEventsMiddleware.ts` around lines 131 - 132, The fallback for dataLength incorrectly treats an empty tooltipTicks array as valid because `??` doesn't replace 0; update the expression to fall back when tooltipTicks is missing or empty by using an explicit check: replace `const dataLength = tooltipTicks?.length ?? displayedData.length;` with a conditional that uses `tooltipTicks?.length > 0 ? tooltipTicks.length : displayedData.length` so navigation uses displayedData when tooltipTicks is an empty array; keep the surrounding logic that checks `dataLength === 0 || nextIndex >= dataLength || nextIndex < 0` unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/state/keyboardEventsMiddleware.ts`:
- Around line 131-132: The fallback for dataLength incorrectly treats an empty
tooltipTicks array as valid because `??` doesn't replace 0; update the
expression to fall back when tooltipTicks is missing or empty by using an
explicit check: replace `const dataLength = tooltipTicks?.length ??
displayedData.length;` with a conditional that uses `tooltipTicks?.length > 0 ?
tooltipTicks.length : displayedData.length` so navigation uses displayedData
when tooltipTicks is an empty array; keep the surrounding logic that checks
`dataLength === 0 || nextIndex >= dataLength || nextIndex < 0` unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3896102c-648d-4184-ad79-3686133bffc7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/state/keyboardEventsMiddleware.ts
Collaborator
|
We're going to need a test for this new code - to prevent regressions. Also please double check the robot comment |
Bundle ReportChanges will increase total bundle size by 22.85kB (0.45%) ⬆️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-treeshaking-polarAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-treeshaking-treemapAssets Changed:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7140 +/- ##
==========================================
+ Coverage 89.59% 89.61% +0.02%
==========================================
Files 536 536
Lines 40401 40483 +82
Branches 5498 5520 +22
==========================================
+ Hits 36197 36279 +82
Misses 4196 4196
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…on for item-based tooltips
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/chart/AccessibilityLayer.spec.tsx (1)
648-735: Align new keyboard tests with the fake-timer testing pattern.These updated tests should use fake timers and pending-timer advancement to match the project’s Redux/requestAnimationFrame test contract.
As per coding guidelines, "Use
vi.useFakeTimers()in all tests due to Redux autoBatchEnhancer dependency on timers andrequestAnimationFrame" and "Callvi.runOnlyPendingTimers()to advance timers after renders when not usingcreateSelectorTestCasehelper, and avoidvi.runAllTimers()to prevent infinite loops".Also applies to: 750-833
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/chart/AccessibilityLayer.spec.tsx` around lines 648 - 735, The new keyboard tests (e.g., the "When chart receives focus..." / "Arrow keys navigate between sectors" / "Arrow keys stop at boundaries" / "Tooltip closes when PieChart loses focus" tests) must use fake timers and advance only pending timers: at the start of each test (or in the test setup) call vi.useFakeTimers(), then after render and after any act(...) that triggers async timers call vi.runOnlyPendingTimers() to advance requestAnimationFrame/Redux-related timers (do not use vi.runAllTimers()); update each test that uses getMainSurface(), svg.focus(), arrowRight(), arrowLeft(), expectTooltipPayload(), and expectTooltipNotVisible() to include these vi calls so the Tooltip updates deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/chart/AccessibilityLayer.spec.tsx`:
- Around line 817-820: The test stops one step short of the right-edge clamping:
after calling arrowRight(svg) and asserting tooltip shows 'Page F : 189' and no
additional mouse movements, add a second arrowRight(svg) call and repeat the
assertions to verify no further movement/clamping at the rightmost point; update
the block containing arrowRight(svg), expectTooltipPayload(container, '', ['Page
F : 189']), and expect(mockMouseMovements.mock.instances).toHaveLength(0) to
include the extra arrowRight call and identical expectations to ensure E->F
transition and then no-op at F are both validated.
---
Nitpick comments:
In `@test/chart/AccessibilityLayer.spec.tsx`:
- Around line 648-735: The new keyboard tests (e.g., the "When chart receives
focus..." / "Arrow keys navigate between sectors" / "Arrow keys stop at
boundaries" / "Tooltip closes when PieChart loses focus" tests) must use fake
timers and advance only pending timers: at the start of each test (or in the
test setup) call vi.useFakeTimers(), then after render and after any act(...)
that triggers async timers call vi.runOnlyPendingTimers() to advance
requestAnimationFrame/Redux-related timers (do not use vi.runAllTimers());
update each test that uses getMainSurface(), svg.focus(), arrowRight(),
arrowLeft(), expectTooltipPayload(), and expectTooltipNotVisible() to include
these vi calls so the Tooltip updates deterministically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d0d8e4d-3c96-4720-ad0d-8d47e0edb75b
📒 Files selected for processing (3)
src/state/keyboardEventsMiddleware.tssrc/state/selectors/combiners/combineTooltipPayloadConfigurations.tstest/chart/AccessibilityLayer.spec.tsx
Comment on lines
817
to
820
| // Ignore right arrow when you're already at the right | ||
| arrowRight(svg); | ||
| expect(tooltip).toHaveTextContent(''); | ||
| expectTooltipPayload(container, '', ['Page F : 189']); | ||
| expect(mockMouseMovements.mock.instances).toHaveLength(0); |
Contributor
There was a problem hiding this comment.
Boundary assertion is incomplete at the right edge.
This step validates E -> F, but it doesn’t verify clamping once already at F. Add one more ArrowRight assertion to prevent regressions.
✅ Suggested test patch
// Ignore right arrow when you're already at the right
arrowRight(svg);
expectTooltipPayload(container, '', ['Page F : 189']);
+ arrowRight(svg);
+ expectTooltipPayload(container, '', ['Page F : 189']);📝 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.
Suggested change
| // Ignore right arrow when you're already at the right | |
| arrowRight(svg); | |
| expect(tooltip).toHaveTextContent(''); | |
| expectTooltipPayload(container, '', ['Page F : 189']); | |
| expect(mockMouseMovements.mock.instances).toHaveLength(0); | |
| // Ignore right arrow when you're already at the right | |
| arrowRight(svg); | |
| expectTooltipPayload(container, '', ['Page F : 189']); | |
| arrowRight(svg); | |
| expectTooltipPayload(container, '', ['Page F : 189']); | |
| expect(mockMouseMovements.mock.instances).toHaveLength(0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/chart/AccessibilityLayer.spec.tsx` around lines 817 - 820, The test
stops one step short of the right-edge clamping: after calling arrowRight(svg)
and asserting tooltip shows 'Page F : 189' and no additional mouse movements,
add a second arrowRight(svg) call and repeat the assertions to verify no further
movement/clamping at the rightmost point; update the block containing
arrowRight(svg), expectTooltipPayload(container, '', ['Page F : 189']), and
expect(mockMouseMovements.mock.instances).toHaveLength(0) to include the extra
arrowRight call and identical expectations to ensure E->F transition and then
no-op at F are both validated.
PavelVanecek
approved these changes
Mar 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
keyboardEventsMiddlewareto dynamically use selectTooltipEventType instead of hardcoding 'axis'.displayedData.lengthwhentooltipTicksis empty.coordinatecheck infocusActionthat prevented Pie charts from initializing focus.Fixes #6921
Description
The keyboard navigation middleware was previously hardcoded to use the
'axis'tooltip event type and relied strictly ontooltipTicksand a resolvedcoordinate. This worked well for Cartesian charts but failed forPieChartand other polar/non-axis charts which use the'item'tooltip event type and calculate coordinates dynamically.This PR updates the middleware to:
tooltipEventTypefrom the state instead of defaulting to'axis'.displayedData.lengthfor upper-bound checks whentooltipTicksare not present.coordinateguard during the focus phase, allowing the tooltip state to initialize properly even for charts that resolve their coordinates later.Related Issue
Fixes #6921
Motivation and Context
Keyboard users were unable to navigate
PieChartcomponents or view tooltips when rendering with theaccessibilityLayerenabled. This change ensures that the accessibility features work seamlessly across all chart types, not just Cartesian ones, improving the overall a11y support in Recharts.How Has This Been Tested?
PieChartwithaccessibilityLayer, navigating via theTabkey to focus, and usingLeft/Right Arrowkeys to navigate between sectors.npm test) and verified that all existing keyboard navigation and interaction tests continue to pass without regression.Screenshots (if appropriate):
(Feel free to add a quick screen recording here showing you tabbing through a Pie chart if you have one!)
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests