-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts] Introduce keyboard navigation #19155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Deploy preview: https://deploy-preview-19155--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
a3f49fd
to
d6dd42e
Compare
CodSpeed Performance ReportMerging #19155 will not alter performanceComparing Summary
Footnotes |
export const selectorChartsFocusedSeriesType = createSelector( | ||
[selectKeyboardNavigation], | ||
(item) => item?.type, | ||
); | ||
|
||
export const selectorChartsFocusedSeriesId = createSelector( | ||
[selectKeyboardNavigation], | ||
(item) => item?.seriesId, | ||
); | ||
|
||
export const selectorChartsFocusedDataIndex = createSelector( | ||
[selectKeyboardNavigation], | ||
(item) => item?.dataIndex, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we already account for non-regular identifiers here? 😉
This feels a bit static if we consider the identifier is configurable. Maybe we can have a more generic selector with a proper memo function? 🤔
Maybe we should have a compareIdentifier
in the series config 💡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit static if we consider the identifier is configurable
I assume you have scatter charts in mind ;)
The identifier will not be the biggest issue. it's the keyboard actions. For those who does not enter in the (seriesId, dataIndex)
identifier framework, it might be easier to create a custom plugin for their keyboard navigation.
...rc/internals/plugins/featurePlugins/useChartKeyboardNavigation/useChartKeyboardNavigation.ts
Outdated
Show resolved
Hide resolved
...rc/internals/plugins/featurePlugins/useChartKeyboardNavigation/useChartKeyboardNavigation.ts
Outdated
Show resolved
Hide resolved
...rc/internals/plugins/featurePlugins/useChartKeyboardNavigation/useChartKeyboardNavigation.ts
Show resolved
Hide resolved
} | ||
|
||
element.addEventListener('keydown', keyBoardHandler); | ||
element.addEventListener('focus', focusNextItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think focus should do anything 🤔
With focus whenever a user clicks anywhere in the chart, the first item will be selected, which I think is quite odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's inspired by ARIA guidelines from the data grid. When the element get focused, it set the focus to it's first sub element.
I could have a rule saying that if charts is focused, but has no internal element focus, I dislay the outlined on the entire SVG. But when user click on th charts I have to show it's getting the focus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be less disruptive I think. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you do anything regarding this comment? I still see the blue around it when clicking on the chart
...ernals/plugins/featurePlugins/useChartKeyboardNavigation/useChartKeyboardNavigation.types.ts
Show resolved
Hide resolved
...plugins/featurePlugins/useChartKeyboardNavigation/useChartKeyboardNavigation.helpers.test.ts
Show resolved
Hide resolved
} | ||
|
||
element.addEventListener('keydown', keyBoardHandler); | ||
element.addEventListener('focus', focusNextItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be less disruptive I think. 🤔
|
||
## Keyboard support | ||
|
||
Set `enableKeyboardNavigation` to `true` to enable the keyboard navigation on your charts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a global enable/disable toggle for this? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a sentence about how to use theme provider to do so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a hard time using this feature in the docs. I can't seem to focus the chart. Do you know how I can do that? Do I need to enable some specific feature?
|
||
## Keyboard support | ||
|
||
Set `enableKeyboardNavigation` to `true` to enable the keyboard navigation on your charts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a demo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
fill: color, | ||
skipAnimation, | ||
layout, | ||
'data-focused': isFocused || undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base UI uses data-highlighted
, but that might conflict with our current highlighted logic.

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-highlighted
could clearly conflict. But maybe data-active
to use similar wording as aria-activedescendent
...rc/internals/plugins/featurePlugins/useChartKeyboardNavigation/useChartKeyboardNavigation.ts
Outdated
Show resolved
Hide resolved
Yes, you need to add |
I see. Should we add a demo, then? Otherwise people won't be able to easily test it from the docs |
Seems to work very well 😄 Screen.Recording.2025-09-09.at.08.42.23.movDo we want to show the tooltip when a mark is focused and the user presses some key? |
Co-authored-by: Bernardo Belchior <[email protected]> Co-authored-by: Jose C Quintas Jr <[email protected]> Signed-off-by: Alexandre Fauquette <[email protected]>
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
||
# Accessibility | ||
|
||
<p class="description">Learn how the Charts implement accessibility features and guidelines, including keyboard navigation that follows international standards.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<p class="description">Learn how the Charts implement accessibility features and guidelines, including keyboard navigation that follows international standards.</p> | |
<p class="description">Learn how Charts implement accessibility features and guidelines, including keyboard navigation that follows international standards.</p> |
This feature is currently supported by line, bar, pie, scatter, and sparkline charts. | ||
|
||
This makes the SVG component focusable thanks to [`tabIndex`](https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attributes/tabindex). | ||
When focused, the charts highlight a value item that can be modified with arrow navigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When focused, the charts highlight a value item that can be modified with arrow navigation. | |
When focused, the chart highlights a value item that can be modified with arrow navigation. |
'&:focus': { | ||
outline: 'none', // By default don't show focus on the SVG container | ||
}, | ||
'&:focus-visible': { | ||
// Show focus outline on the SVG container only when using keyboard navigation | ||
outline: 'auto', | ||
'&[data-has-focused-item=true]': { | ||
// But not if the chart has a focused children item | ||
outline: 'none', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JCQuintas I went further with the focus display.
By relying on the :focus-visible
the outlined is not display when the element get focused by click, but is visible when focused by Tab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, interactions feels good now, thanks :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small docs suggestions, everything else looks good
Introduce keyboard navigation
The data format is fairly similar to the highlight plugin
Rendering aspect
outlined: 'auto'
property to show the focus position. And they rely ondata-focused=true
for that.Focus management
aria-activedescendent
to keep the focus on the SVG and target specific items like I'm already doing it withdata-focused=true
. I will probably add anid
attribute only on the element of interest.