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

[SR] Linear System - add screen reader support for Linear System interactive graph #2030

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nishasy
Copy link
Contributor

@nishasy nishasy commented Dec 18, 2024

Summary:

Add the aria label and descriptions for the full graph and the
interactive elements in the Linear System graph, based on the
SRUX doc.

Issue: https://khanacademy.atlassian.net/browse/LEMS-1727

Test plan:

yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/linear-system.test.tsx

Storybook

View Storybook publish in the checks below to try it yourself.

Screen.Recording.2024-12-18.at.2.04.53.PM.mov

The [SRUX doc](https://khanacademy.atlassian.net/wiki/spaces/LC/pages/3460366348/Linear) still needs a label for the grab handle, but I tried my best in the meantime.

- Add a label and describedby for the grab handle.
- Add aria-live states for the different interactive elements
  so they don't override each other.

Issue: https://khanacademy.atlassian.net/browse/LEMS-1726

Test plan:
`yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx`

Storybook
- http://localhost:6006/iframe.html?id=perseuseditor-widgets-interactive-graph--interactive-graph-linear&viewMode=story
- Try all the different slopes and intercepts
- Move different elements and confirm that the updated aria-label
  is what is read out, and none of the other elements override the
  currently focused one.
@nishasy nishasy self-assigned this Dec 18, 2024
Copy link
Contributor

github-actions bot commented Dec 18, 2024

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (193ee2a) and published it to npm. You
can install it using the tag PR2030.

Example:

yarn add @khanacademy/perseus@PR2030

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR2030

Copy link
Contributor

github-actions bot commented Dec 18, 2024

Size Change: +484 B (+0.04%)

Total Size: 1.27 MB

Filename Size Change
packages/perseus/dist/es/index.js 418 kB +334 B (+0.08%)
packages/perseus/dist/es/strings.js 5.08 kB +150 B (+3.05%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 39 kB
packages/keypad-context/dist/es/index.js 760 B
packages/kmath/dist/es/index.js 4.26 kB
packages/math-input/dist/es/index.js 78 kB
packages/math-input/dist/es/strings.js 1.79 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-editor/dist/es/index.js 688 kB
packages/perseus-linter/dist/es/index.js 22.2 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.5 kB

compressed-size-action

@nishasy nishasy marked this pull request as ready for review December 18, 2024 22:10
Base automatically changed from sr-linear-body to main December 19, 2024 00:24
@nishasy
Copy link
Contributor Author

nishasy commented Dec 19, 2024

The parent pull-request (#2025) has been merged into main, but this branch (sr-linear-system) now has conflicts with the new base branch. These conflicts must be resolved before checks can complete on this pull-request.

Copy link
Member

@catandthemachines catandthemachines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The experience is working for the individual points and lines, but I think we need to circle back and make sure it's working at the all up graph level.

return (
<>
<g
aria-label={strings.srLinearSystemGraph}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This aria-label is not getting read out when navigating to the whole graph. This is because the whole graph descriptions needs to happen at the mafs-graph.tsx level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants