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

Merged
merged 22 commits into from
Jan 15, 2025

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 (11f2a4a) 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: +683 B (+0.05%)

Total Size: 1.47 MB

Filename Size Change
packages/perseus/dist/es/index.js 412 kB +596 B (+0.14%)
packages/perseus/dist/es/strings.js 4.91 kB +87 B (+1.81%)
ℹ️ 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 83.1 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 23.1 kB
packages/perseus-editor/dist/es/index.js 689 kB
packages/perseus-linter/dist/es/index.js 22.2 kB
packages/perseus-score/dist/es/index.js 103 kB
packages/pure-markdown/dist/es/index.js 3.66 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.

Copy link
Member

Choose a reason for hiding this comment

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

@nishasy , can you remind me of the resolution we decided on this? I remember we discussed this before the holidays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the full graph description ("Interactive elements: ...") to this PR. You can see the commit below as "Add full graph description of all interactive elements" - 7d96885

@@ -478,6 +516,13 @@ export const strings: {
srAngleGraphAriaLabel: "An angle on a coordinate plane.",
srAngleGraphAriaDescription:
"The angle measure is %(angleMeasure)s degrees with a vertex at %(vertexX)s comma %(vertexY)s, a point on the starting side at %(startingSideX)s comma %(startingSideY)s and a point on the ending side at %(endingSideX)s comma %(endingSideY)s",
srLinearSystemGraph: "Two lines on a coordinate plane.",
srLinearSystemPoints:
"Line %(lineSequence)s has two points, point 1 at %(point1X)s comma %(point1Y)s and point 2 at %(point2X)s comma %(point2Y)s.",
Copy link
Member

Choose a reason for hiding this comment

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

%(lineSequence)s made me think this is a placeholder for a description of a sequence of lines. Translators might find it confusing as well. I might prefer lineSequenceNumber or maybe just lineNumber.

Suggested change
"Line %(lineSequence)s has two points, point 1 at %(point1X)s comma %(point1Y)s and point 2 at %(point2X)s comma %(point2Y)s.",
"Line %(lineSequenceNumber)s has two points, point 1 at %(point1X)s comma %(point1Y)s and point 2 at %(point2X)s comma %(point2Y)s.",

expect(linearSystemGraph).toBeInTheDocument();
expect(linearSystemGraph).toHaveAttribute(
"aria-describedby",
":r1:-line1-points :r1:-line1-intercept :r1:-line1-slope :r1:-line2-points :r1:-line2-intercept :r1:-line2-slope",
Copy link
Member

Choose a reason for hiding this comment

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

I wish we could assert on the description text instead of the IDs of the description elements. That way we'd be testing what the user actually sees. If React Testing Library doesn't have any matchers for this (I didn't find any with a cursory search) maybe we should write our own. Then we could do something like:

expect(linearSystemGraph).toHaveAriaDescription("line 1 through 0 comma 0 and 5 comma 5 ...")

Our custom matchers for Jest are defined in config/test/custom-matchers.ts.

test.each`
case | coords | slopeDescription
${"positive slope"} | ${[[1, 1], [3, 3]]} | ${`Its slope increases from left to right.`}
${"negative slope"} | ${[[3, 3], [1, 6]]} | ${`Its slope decreases from left to right.`}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should get a content creator to weigh in on this wording. Strictly speaking, I don't think the slope increases or decreases; the slope is constant and it's the y-coordinate that increases or decreases.

Maybe "Its slope is positive/negative/zero/undefined" would be a good way to phrase this? In any case, I'd defer to Charlie or Nick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. Personally, I don't think that should be a blocker, I think we should update the wording when we do our strings pass before creating translation tickets.

Comment on lines 42 to 44
pointsDescriptionId: id + `-line${i + 1}-points`,
interceptDescriptionId: id + `-line${i + 1}-intercept`,
slopeDescriptionId: id + `-line${i + 1}-slope`,
Copy link
Member

Choose a reason for hiding this comment

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

Minor: as long as we're using a template string, we could include id in it too:

Suggested change
pointsDescriptionId: id + `-line${i + 1}-points`,
interceptDescriptionId: id + `-line${i + 1}-intercept`,
slopeDescriptionId: id + `-line${i + 1}-slope`,
pointsDescriptionId: `${id}-line${i + 1}-points`,
interceptDescriptionId: `${id}-line${i + 1}-intercept`,
slopeDescriptionId: `${id}-line${i + 1}-slope`,

<g
key={slopeDescriptionId}
id={slopeDescriptionId}
style={{display: "hidden"}}
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of display: hidden here? I don't think hidden is a valid value for display (MDN docs) and either display: none or visibility: hidden will hide the content from screenreaders.

I think we probably want to apply an srOnly set of styles here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omg

Thank you for pointing this out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this also means we can use innerText in the SR tree like you suggested rather than textContent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benchristel I just remembered why I tried to hide this from screen readers. I don't want these descriptions to something people can read out, it's just there for the other component to refer to. It would be really confusing if a screen reader user navigated to this part after the graph and then it started randomly reading these descriptions.

That being said... it doesn't seem like they're being read even after changing the style to a11y.srOnly - and this is even reflected in the SR tree. This is super confusing to me. I don't know why they're not being read after the graph.

});
}
const slopeString = getSlopeStringForLine(line, strings);
const interceptString = getInterceptStringForLine(line, strings, locale);
Copy link
Member

Choose a reason for hiding this comment

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

👍 for refactoring this!


export function getSlopeStringForLine(line: PairOfPoints, strings): string {
const slope = (line[1][1] - line[0][1]) / (line[1][0] - line[0][0]);
if (slope === Infinity || slope === -Infinity) {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, you could use Number.isFinite here:

Suggested change
if (slope === Infinity || slope === -Infinity) {
if (!Number.isFinite(slope)) {

That will also cause srLinearGraphSlopeVertical to be returned if the slope is NaN. (A slope of NaN shouldn't be possible, since we prevent the two control points of the line from having the same coordinates, so this is more paranoia than anything.)

Comment on lines 92 to 93
const hasXIntercept = xIntercept !== Infinity && xIntercept !== -Infinity;
const hasYIntercept = yIntercept !== Infinity && yIntercept !== -Infinity;
Copy link
Member

Choose a reason for hiding this comment

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

Number.isFinite would be a nice simplification here, too. Actually, I think there's a bug that isFinite would fix, because NaN is a possibility here. E.g. if line[0][1] is 0 and the slope is also 0, the xIntercept will be NaN (since the line lies on the x-axis). Might be worth proving that with a test.

Suggested change
const hasXIntercept = xIntercept !== Infinity && xIntercept !== -Infinity;
const hasYIntercept = yIntercept !== Infinity && yIntercept !== -Infinity;
const hasXIntercept = Number.isFinite(xIntercept);
const hasYIntercept = Number.isFinite(yIntercept);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh hmm. You're right that the line crossing the x-axis causes it to be weird. Right now, it says it crosses the x-axis at [some number] comma 0, which isn't really correct.

I'll ask Caitlyn if we should add a custom string for that case, and same thing for Y axis.

@@ -18,7 +23,9 @@ export function renderLinearSystemGraph(
): InteractiveGraphElementSuite {
return {
graph: <LinearSystemGraph graphState={state} dispatch={dispatch} />,
interactiveElementsDescription: null,
interactiveElementsDescription: (
<LinearSystemGraphDescription state={state} />
Copy link
Contributor

Choose a reason for hiding this comment

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

should all of our graphs be updating this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anakaren-rojas Yes! I went back and added it for Circle and Linear. We'll have to double check and make sure all the graphs have this.

Comment on lines +162 to +166
function LinearSystemGraphDescription({
state,
}: {
state: LinearSystemGraphState;
}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a nit since I still don't quite understand the purpose of the destructuring that's happening here

Suggested change
function LinearSystemGraphDescription({
state,
}: {
state: LinearSystemGraphState;
}) {
function LinearSystemGraphDescription(
state: LinearSystemGraphState) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a component! The {state} is the props. It needs to be like that because it gets called like <LinearSystemGraphDescription state={state} />.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I seeee
There's an alternative way to declare functional components that might make the distinction between a FC and a regular function more apparent to the layman like me

type LinearSystemGraphDescriptionProps = {
 state: LinearSystemGraphState
}

const LinearSystemGraphDescription: React.FC<LinearSystemGraphDescriptionProps> =({state}) => {

@@ -62,3 +65,54 @@ export function getArrayWithoutDuplicates(array: Array<Coord>): Array<Coord> {

return returnArray;
}

export function getSlopeStringForLine(line: PairOfPoints, strings): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

can strings either be renamed to perseusStrings or the type added?

export function getInterceptStringForLine(
line: PairOfPoints,
strings,
locale,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
locale,
locale : string,

@nishasy nishasy merged commit d96821e into main Jan 15, 2025
8 checks passed
@nishasy nishasy deleted the sr-linear-system branch January 15, 2025 23:55
jeremywiebe pushed a commit that referenced this pull request Jan 20, 2025
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.


# Releases
## @khanacademy/perseus@51.0.0

### Major Changes

-   [#2101](#2101) [`9cabe689a`](9cabe68) Thanks [@handeyeco](https://github.com/handeyeco)! - Move scorers and validators to `perseus-score`

### Minor Changes

-   [#2127](#2127) [`6f2813cfc`](6f2813c) Thanks [@benchristel](https://github.com/benchristel)! - Avoid adding undefined values to objects parsed from Perseus JSON when properties are missing.

### Patch Changes

-   [#2130](#2130) [`165305e11`](165305e) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Change interactive-graph visual regression stories to Storybook's CSF v3


-   [#2077](#2077) [`faccc2d59`](faccc2d) Thanks [@benchristel](https://github.com/benchristel)! - Internal: Enable the exhaustive test tool for parsePerseusItem to test articles.


-   [#2030](#2030) [`d96821e08`](d96821e) Thanks [@nishasy](https://github.com/nishasy)! - [SR] Linear System - add screen reader support for Linear System interactive graph


-   [#2036](#2036) [`0f8d11c0b`](0f8d11c) Thanks [@nishasy](https://github.com/nishasy)! - [SR] Ray graph - Add screen reader support for Ray interactive graph


-   [#2109](#2109) [`41ffd4a71`](41ffd4a) Thanks [@dependabot](https://github.com/apps/dependabot)! - Updating our wonder-blocks packages with the latest versions.

-   Updated dependencies \[[`9cabe689a`](9cabe68), [`41ffd4a71`](41ffd4a)]:
    -   @khanacademy/kmath@0.3.0
    -   @khanacademy/perseus-core@3.2.0
    -   @khanacademy/perseus-score@1.1.0
    -   @khanacademy/math-input@22.2.1
    -   @khanacademy/kas@0.4.11
    -   @khanacademy/keypad-context@1.0.14
    -   @khanacademy/perseus-linter@1.2.13
    -   @khanacademy/pure-markdown@0.3.22
    -   @khanacademy/simple-markdown@0.13.15

## @khanacademy/kmath@0.3.0

### Minor Changes

-   [#2101](#2101) [`9cabe689a`](9cabe68) Thanks [@handeyeco](https://github.com/handeyeco)! - Move scorers and validators to `perseus-score`

### Patch Changes

-   Updated dependencies \[[`9cabe689a`](9cabe68)]:
    -   @khanacademy/perseus-core@3.2.0

## @khanacademy/perseus-core@3.2.0

### Minor Changes

-   [#2101](#2101) [`9cabe689a`](9cabe68) Thanks [@handeyeco](https://github.com/handeyeco)! - Move scorers and validators to `perseus-score`

## @khanacademy/perseus-score@1.1.0

### Minor Changes

-   [#2101](#2101) [`9cabe689a`](9cabe68) Thanks [@handeyeco](https://github.com/handeyeco)! - Move scorers and validators to `perseus-score`

### Patch Changes

-   Updated dependencies \[[`9cabe689a`](9cabe68)]:
    -   @khanacademy/kmath@0.3.0
    -   @khanacademy/perseus-core@3.2.0
    -   @khanacademy/kas@0.4.11

## @khanacademy/kas@0.4.11

### Patch Changes

-   Updated dependencies \[[`9cabe689a`](9cabe68)]:
    -   @khanacademy/perseus-core@3.2.0

## @khanacademy/keypad-context@1.0.14

### Patch Changes

-   Updated dependencies \[[`9cabe689a`](9cabe68)]:
    -   @khanacademy/perseus-core@3.2.0

## @khanacademy/math-input@22.2.1

### Patch Changes

-   [#2101](#2101) [`9cabe689a`](9cabe68) Thanks [@handeyeco](https://github.com/handeyeco)! - Move scorers and validators to `perseus-score`


-   [#2109](#2109) [`41ffd4a71`](41ffd4a) Thanks [@dependabot](https://github.com/apps/dependabot)! - Updating our wonder-blocks packages with the latest versions.

-   Updated dependencies \[[`9cabe689a`](9cabe68)]:
    -   @khanacademy/perseus-core@3.2.0
    -   @khanacademy/keypad-context@1.0.14

## @khanacademy/perseus-editor@17.3.1

### Patch Changes

-   [#2101](#2101) [`9cabe689a`](9cabe68) Thanks [@handeyeco](https://github.com/handeyeco)! - Move scorers and validators to `perseus-score`


-   [#2126](#2126) [`518b005f2`](518b005) Thanks [@benchristel](https://github.com/benchristel)! - Fix a React warning about non-unique component keys in the exercise editor.


-   [#2109](#2109) [`41ffd4a71`](41ffd4a) Thanks [@dependabot](https://github.com/apps/dependabot)! - Updating our wonder-blocks packages with the latest versions.

-   Updated dependencies \[[`165305e11`](165305e), [`6f2813cfc`](6f2813c), [`faccc2d59`](faccc2d), [`9cabe689a`](9cabe68), [`d96821e08`](d96821e), [`0f8d11c0b`](0f8d11c), [`41ffd4a71`](41ffd4a)]:
    -   @khanacademy/perseus@51.0.0
    -   @khanacademy/kmath@0.3.0
    -   @khanacademy/perseus-core@3.2.0
    -   @khanacademy/perseus-score@1.1.0
    -   @khanacademy/math-input@22.2.1
    -   @khanacademy/kas@0.4.11
    -   @khanacademy/keypad-context@1.0.14
    -   @khanacademy/pure-markdown@0.3.22

## @khanacademy/perseus-linter@1.2.13

### Patch Changes

-   Updated dependencies \[[`9cabe689a`](9cabe68)]:
    -   @khanacademy/perseus-core@3.2.0

## @khanacademy/pure-markdown@0.3.22

### Patch Changes

-   Updated dependencies \[[`9cabe689a`](9cabe68)]:
    -   @khanacademy/perseus-core@3.2.0
    -   @khanacademy/simple-markdown@0.13.15

## @khanacademy/simple-markdown@0.13.15

### Patch Changes

-   Updated dependencies \[[`9cabe689a`](9cabe68)]:
    -   @khanacademy/perseus-core@3.2.0

## @khanacademy/perseus-dev-ui@5.1.1

### Patch Changes

-   [#2101](#2101) [`9cabe689a`](9cabe68) Thanks [@handeyeco](https://github.com/handeyeco)! - Move scorers and validators to `perseus-score`


-   [#2109](#2109) [`41ffd4a71`](41ffd4a) Thanks [@dependabot](https://github.com/apps/dependabot)! - Updating our wonder-blocks packages with the latest versions.

-   Updated dependencies \[[`9cabe689a`](9cabe68), [`41ffd4a71`](41ffd4a)]:
    -   @khanacademy/kmath@0.3.0
    -   @khanacademy/perseus-core@3.2.0
    -   @khanacademy/math-input@22.2.1
    -   @khanacademy/kas@0.4.11
    -   @khanacademy/perseus-linter@1.2.13
    -   @khanacademy/pure-markdown@0.3.22
    -   @khanacademy/simple-markdown@0.13.15

Author: khan-actions-bot

Reviewers: jeremywiebe

Required Reviewers:

Approved By: jeremywiebe

Checks: ⏭️  Publish npm snapshot, ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x)

Pull Request URL: #2117
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.

4 participants