Skip to content
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.

Sign up for GitHub

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

Jump to bottom

accessibility issue with using 2 charts on the same page. #4384

Closed
nermin97 opened this issue Apr 3, 2024 · 7 comments · May be fixed by gptkrsh/LinkFree#509, gptkrsh/LinkFree#520, gptkrsh/LinkFree#530, harshadashirsat6/Indexia-version-2.0#55 or headlamp-k8s/headlamp#2018
Closed

accessibility issue with using 2 charts on the same page. #4384

nermin97 opened this issue Apr 3, 2024 · 7 comments · May be fixed by gptkrsh/LinkFree#509, gptkrsh/LinkFree#520, gptkrsh/LinkFree#530, harshadashirsat6/Indexia-version-2.0#55 or headlamp-k8s/headlamp#2018
Labels
accessibility Issues associated with accessibility

Comments

@nermin97
Copy link

nermin97 commented Apr 3, 2024

I am facing an accessibility issue when using 2 charts next to eachother
image

I've added the aria-labeledby on both the charts but it is not reflecting on the recharts-wrapper it seems.
@ckifer ckifer added the accessibility Issues associated with accessibility label Apr 3, 2024
@ckifer
Copy link
Member

ckifer commented Apr 3, 2024

@julianna-langston thoughts?

@julianna-langston
Copy link
Contributor

julianna-langston commented Apr 3, 2024 •
edited

Both aria-label and aria-labelledby get passed along to the SVG element. However, that SVG element is wrapped in the <div role="region">, which never receives those extra attributes.

Note that the SVG element itself already has a role attribute, and it seems like it will always be the same role as the .recharts-wrapper. My preference is just to remove the role from the .recharts-wrapper container element. I believe having 2 identical nested roles is unnecessary, and the SVG element is already receiving the relevant aria labelling attributes.

@ckifer Is there a time when the .recharts-wrapper has multiple SVG elements nested inside of it? If so, can you point me to one of those examples? Otherwise, what are your thoughts on removing that role?

Anyway, @nermin97, I hope it's easy for you to re-run the scans that are giving you that error. The error message looks like aXe/Deque to me, but I wasn't able to reproduce the error using Lighthouse (which sort of uses aXe). Since I don't know what tool you used for the scan, I'm not sure what will actually get you a clean scan. These options are your best bet for clearing the error today:

  • Use the accessibilityLayer prop. This will assign a role of application (and make your chart tooltips accessible to keyboard-only and screen reader users). For example: <LineChart data={myData} accessibilityLayer>
  • Overwrite the role to something else, like "none". role="none effectively tells the screen reader that the content is just a pile of text (which, without accessibilityLayer, is true). For example: <LineChart data={myData} role="none">.

@ckifer
Copy link
Member

ckifer commented Apr 3, 2024 •
edited

@julianna-langston thanks for the detailed reply!

These are the usages of Surface which is where tags originate from

It looks like two SVGs are rendered in a chart with a Legend that uses DefaultLegendContent
Inspecting this example reveals that https://master--63da8268a0da9970db6992aa.chromatic.com/?path=/story/examples-barchart--stacked

Otherwise, only 1 <svg> per chart from what I can see

@julianna-langston julianna-langston mentioned this issue Apr 3, 2024
Merged
7 tasks
@julianna-langston
Copy link
Contributor

The Legend example shouldn't conflict with removing the role from the parent.

I've opened a PR for the change.

ckifer pushed a commit that referenced this issue Apr 3, 2024
@julianna-langston
Remove role attribute from recharts-wrapper (#4386)
4a6acc3 
<!--- Provide a general summary of your changes in the Title above -->

## Description

<!--- Describe your changes in detail -->

Before this PR, there was a `role` attribute on both the
`.recharts-wrapper` parent DIV, as well as the SVG element. This is
unnecessary, since the SVG element is enough. That extra role also
caused problems, as I outlined in
#4384

This PR removes the `role` attribute from the `.recharts-wrapper`
element. The SVG element still retains its role.

## Related Issue

<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an
issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps
to reproduce -->
<!--- Please link to the issue here: -->
#4384

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->

## How Has This Been Tested?

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

## Screenshots (if appropriate):

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [ ] I have added a storybook story or extended an existing story to
show my changes
ckifer pushed a commit that referenced this issue Apr 3, 2024
@julianna-langston
Remove role attribute from recharts-wrapper (#4386)
db1e020 
<!--- Provide a general summary of your changes in the Title above -->

<!--- Describe your changes in detail -->

Before this PR, there was a `role` attribute on both the
`.recharts-wrapper` parent DIV, as well as the SVG element. This is
unnecessary, since the SVG element is enough. That extra role also
caused problems, as I outlined in
#4384

This PR removes the `role` attribute from the `.recharts-wrapper`
element. The SVG element still retains its role.

<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an
issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps
to reproduce -->
<!--- Please link to the issue here: -->
#4384

<!--- Why is this change required? What problem does it solve? -->

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

<!--- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)

<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes.
- [ ] I have added a storybook story or extended an existing story to
show my changes
@ckifer
Copy link
Member

ckifer commented Apr 3, 2024

cherry-picked your commit into master as well. This should be out next patch

@ckifer
Copy link
Member

ckifer commented Apr 4, 2024

released in 2.12.4

@ckifer ckifer closed this as completed Apr 4, 2024
@nermin97
Copy link
Author

nermin97 commented Apr 4, 2024

Thanks for the help.

This was referenced May 23, 2024
Open
Open
Open
@syedabdullahali syedabdullahali mentioned this issue May 28, 2024
Open
@caniszczyk caniszczyk mentioned this issue May 31, 2024
Open
@Crihojames Crihojames mentioned this issue Jun 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accessibility Issues associated with accessibility
Projects
None yet
Milestone
No milestone
3 participants
@ckifer @nermin97 @julianna-langston