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.

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

VictoryTooltip does not support accessible SVG labels #2660

Closed
noranda opened this issue Oct 12, 2023 · 5 comments
Closed

VictoryTooltip does not support accessible SVG labels #2660

noranda opened this issue Oct 12, 2023 · 5 comments
Assignees
Labels
Type: Enhancement ✏️ An enhancement or feature proposal that will be addressed after the next release

Comments

@noranda
Copy link
Contributor

noranda commented Oct 12, 2023

Describe the bug
VictoryTooltip does not have an option for providing an accessible label to it's SVG element. This is causing accessibility errors.

For the actual chart SVG, I can pass in a containerId to my container to provide accessible labels. I cannot do the same with VictoryTooltip.

containerComponent={
  <VictoryVoronoiContainer containerId={idPrefix} />
}

Victory version
36.6.10

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://formidable.com/open-source/victory/docs/victory-tooltip
  2. Inspect source code
  3. View tooltip svg (see screenshot below)

Expected behavior
I should be able to pass in a containerId to VictoryTooltip in order to make the SVG accessible, like I can with VictoryBar and VictoryLine.

Screenshots
Screenshot 2023-10-12 at 1 26 28 PM

Desktop (please complete the following information):

  • OS: iOS 13.5.1
  • Browser: Arc
  • Version: 1.11.0
@carbonrobot carbonrobot added Type: Enhancement ✏️ An enhancement or feature proposal that will be addressed after the next release Note: Good first issue 🤩 Good issue for new contributors labels Oct 18, 2023
@carbonrobot carbonrobot self-assigned this Oct 18, 2023
@carbonrobot
Copy link
Contributor

Thanks for the issue @noranda . The code around containers is a little tricky and we could handle this in a couple different ways.

Just a couple questions as we move along:

  1. Are you looking for an id to be set on the SVG container element or on each individual label?

  2. Which do see as the most preferable API for this?

// option1: cascade the prefix to the label component
<VictoryBar
  containerId="myprefix"
  data={sampleData}
  labels={({ datum }) => `y: ${datum.y}`}
  labelComponent={<VictoryTooltip  flyoutWidth={90} />}
/>

// option2: explicitly set the prefix on the tooltip
<VictoryBar
  containerId="containerprefix"
  data={sampleData}
  labels={({ datum }) => `y: ${datum.y}`}
  labelComponent={<VictoryTooltip  flyoutWidth={90} containerId="tooltipprefix" />}
/>

@carbonrobot carbonrobot removed the Note: Good first issue 🤩 Good issue for new contributors label Oct 19, 2023
@noranda
Copy link
Contributor Author

noranda commented Oct 19, 2023

Thanks @carbonrobot!

  1. We only need an aria-label on the tooltip SVG element. A screenreader will be able to read the text in the tooltip since it's wrapped in a <text> tag, but accessibility guidelines state that all SVGs should have an accessible label.
  2. I think option 1 is fine here for my purposes so long as an aria-label is added to the SVG element. I don't have any reason to define a prefix id for it, but perhaps others might.

@scottrippey
Copy link
Member

There's a way to do this, by customizing the desc of the tooltip. Here's an example:

<VictoryChart
  domain={{ x: [0, 11], y: [-10, 10] }}
>
  <VictoryBar
    labelComponent={
      <VictoryTooltip 
        labelComponent={
          <VictoryLabel desc={datum => datum.text} />
        }
      />}
    data={[
      {x: 2, y: 5, label: "right-side-up"},
      {x: 4, y: -6, label: "upside-down"},
      {x: 6, y: 4, label: "tiny"},
      {x: 8, y: -5, label: "or a little \n BIGGER"},
      {x: 10, y: 7, label: "automatically"}
    ]}
    style={{
      data: {fill: "tomato", width: 20}
    }}
  />
</VictoryChart>

It's a little deep, but you customize the <VictoryLabel> inside the <VictoryTooltip> as you can see above.
In the SVG, it renders something like <text><desc>LABEL</desc><tspan>LABEL</tspan></text>. Obviously you could customize the desc prop if you want it to say something else, based on the data.

Here's a screenshot:
image

@scottrippey
Copy link
Member

@noranda Just wanted to follow up; does this achieve the desired accessibility? The <desc> segment should be accessible to screen readers.
I'd be happy to discuss whether there should be an easier solution. Perhaps we shouldn't require 2-layers-deep of customization to add a desc attribute; perhaps it should be an attribute on VictoryTooltip. Let me know what you think!

@noranda
Copy link
Contributor Author

noranda commented Nov 2, 2023

@scottrippey I believe this solution will work, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement ✏️ An enhancement or feature proposal that will be addressed after the next release
Projects
None yet
Development

No branches or pull requests

3 participants