-
Notifications
You must be signed in to change notification settings - Fork 350
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
[SR] Add aria labels to interactive Circle graph #1928
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (193facf) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1928 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1928 |
Size Change: +976 B (+0.08%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
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.
Unfortunately, I don't think I can test the changes to the aria properties on update with jest. I don't even think I can test that the radius point's aria live value state updates when the circle is moved.
Someone please tell me if I'm wrong. I'm open to suggestions for how to test this stuff.
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.
You should be able to use a command (can't remember it off the top of my head) where you fire onMove
or move
event and then you can use expect(aria-live value in element to be "polite")
.
It's worth a 15 min time box to see if this can be done.
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 article might have what you're looking for, the tool is called fireEvent
:
https://vinayak-hegde.medium.com/js-react-and-customevent-testing-using-jest-and-react-testing-library-abb247fd758a
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.
Added tests!
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.
Approved! If you can give the test one extra try to trigger a move event and then check that the aria-live changes. Other than that you look good. ❤️
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.
You should be able to use a command (can't remember it off the top of my head) where you fire onMove
or move
event and then you can use expect(aria-live value in element to be "polite")
.
It's worth a 15 min time box to see if this can be done.
@@ -20,32 +21,6 @@ function expectLabelInDoc(label: string) { | |||
expect(screen.getByLabelText(label)).toBeInTheDocument(); | |||
} | |||
|
|||
function getBaseMafsGraphProps(): MafsGraphProps { |
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! Glad you moved this <3
@@ -37,31 +38,101 @@ function CircleGraph(props: CircleGraphProps) { | |||
const {dispatch, graphState} = props; | |||
const {center, radiusPoint} = graphState; | |||
|
|||
const {strings} = usePerseusI18n(); | |||
const [radiusPointAriaLive, setRadiusPointAriaLive] = React.useState< | |||
"off" | "polite" |
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'd recommend using the aria live type in
packages/perseus/src/widgets/interactive-graphs/types.ts
</> | ||
{/* Hidden elements to provide the descriptions for the | ||
circle and radius point's `aria-describedby` properties. */} | ||
<g id={radiusId} style={{display: "hidden"}}> |
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.
If I remember correctly, using display: "hidden"
also hides it from the screen reader. Did you have any issues when testing this?
Using a11y.srOnly
might be an alternative if you do have issues
packages/perseus/src/util/a11y.ts
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 used display: hidden
because I don't want the screen reader (or any user) to ever actually access this element. It's only there for its text and ID so that other stuff can refer to it. It seemed to work fine in my manual tests, so I think this should be okay?
Summary:
Make the Circle interactive graph screen reader accessible as outlined by the Circle SRUX doc.
strings.ts
file for translations laterIssue: https://khanacademy.atlassian.net/browse/LEMS-1706
Test plan: