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

feat(LEMS-1733): adds aria labels to line segment #2032

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fuzzy-cheetahs-develop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

adds aria labels to line segment
95 changes: 94 additions & 1 deletion packages/perseus/src/strings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,47 @@ export type PerseusStrings = {
tsX: string;
tsY: string;
}) => string;
srSingleSegmentGraphAriaLabel: string;
srMultipleSegmentGraphAriaLabel: ({
countOfSegments,
}: {
countOfSegments: number;
}) => string;
srIndividualSegmentAriaDescription: ({
point1X,
point1Y,
point2X,
point2Y,
length,
indexOfSegment,
}: {
point1X: string;
point1Y: string;
point2X: string;
point2Y: string;
length: string;
indexOfSegment: number;
}) => string;
srSingleSegmentGraphEndpointAriaLabel: ({
endpointNumber,
x,
y,
}: {
endpointNumber: number;
x: string;
y: string;
}) => string;
srMultipleSegmentGraphEndpointAriaLabel: ({
endpointNumber,
x,
y,
indexOfSegment,
}: {
endpointNumber: number;
x: string;
y: string;
indexOfSegment: number;
}) => string;
// The above strings are used for interactive graph SR descriptions.
};

Expand Down Expand Up @@ -543,13 +584,44 @@ export const strings: {
message:
"Point 2, vertex at %(x)s comma %(y)s. Angle %(angleMeasure)s degrees",
},
srAngleGraphAriaLabel: "An angle on a coordinate plane.",
srAngleGraphAriaLabel: {
context:
"Screenreader-accessible label for an angle on a coordinate plane.",
message: "An angle on a coordinate plane",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a period to the end of this?

},
srAngleGraphAriaDescription: {
context:
"Screenreader-only description of an angle on a coordinate plane.",
message:
"The angle measure is %(angleMeasure)s degrees with a vertex at %(vertexX)s comma %(vertexY)s, a point on the initial side at %(isX)s comma %(isY)s and a point on the terminal side at %(tsX)s comma %(tsY)s",
},
srSingleSegmentGraphAriaLabel: {
context:
"Screenreader-accessible description of a line segment on a coordinate plane.",
message: "A line segment on a coordinate plane.",
},
srMultipleSegmentGraphAriaLabel: {
context:
"Screenreader accessible description of multiple line segments on a coordinate plane.",
message: "%(countOfSegments)s segments on a coordinate plane.",
},
srIndividualSegmentAriaDescription: {
context:
"Screenreader-only description of a line segment on a coordinate plane.",
message:
"Segment %(indexOfSegment)s: Endpoint 1 at %(point1X)s comma %(point1Y)s. Endpoint 2 %(point2X)s comma %(point2Y)s. Segment length %(length)s units.",
},
srSingleSegmentGraphEndpointAriaLabel: {
context:
"Screenreader-accessible label for an endpoint of a line segment on a coordinate plane.",
message: "Endpoint %(endpointNumber)s at %(x)s comma %(y)s.",
},
srMultipleSegmentGraphEndpointAriaLabel: {
context:
"Screenreader-accessible label for an endpoint of a line segment on a coordinate plane.",
message:
"Endpoint %(endpointNumber)s on segment %(indexOfSegment)s at %(x)s comma %(y)s.",
},
// The above strings are used for interactive graph SR descriptions.
};

Expand Down Expand Up @@ -767,5 +839,26 @@ export const mockStrings: PerseusStrings = {
tsY,
}) =>
`The angle measure is ${angleMeasure} degrees with a vertex at ${vertexX} comma ${vertexY}, a point on the initial side at ${isX} comma ${isY} and a point on the terminal side at ${tsX} comma ${tsY}.`,
srSingleSegmentGraphAriaLabel: "A line segment on a coordinate plane.",
srMultipleSegmentGraphAriaLabel: ({countOfSegments}) =>
`${countOfSegments} segments on a coordinate plane.`,
srIndividualSegmentAriaDescription: ({
point1X,
point1Y,
point2X,
point2Y,
length,
indexOfSegment,
}) =>
`Segment ${indexOfSegment}: Endpoint 1 at ${point1X} comma ${point1Y}. Endpoint 2 at ${point2X} comma ${point2Y}. Segment length ${length} units.`,
srSingleSegmentGraphEndpointAriaLabel: ({endpointNumber, x, y}) =>
`Endpoint ${endpointNumber} at ${x} comma ${y}.`,
srMultipleSegmentGraphEndpointAriaLabel: ({
endpointNumber,
x,
y,
indexOfSegment,
}) =>
`Endpoint ${endpointNumber} on segment ${indexOfSegment} at ${x} comma ${y}.`,
// The above strings are used for interactive graph SR descriptions.
};
141 changes: 118 additions & 23 deletions packages/perseus/src/widgets/interactive-graphs/graphs/segment.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import {point as kpoint} from "@khanacademy/kmath";
import * as React from "react";

import {usePerseusI18n} from "../../../components/i18n-context";
import {X, Y} from "../math";
import {actions} from "../reducer/interactive-graph-action";

import {MovableLine} from "./components/movable-line";
import {srFormatNumber} from "./screenreader-text";

import type {
Dispatch,
InteractiveGraphElementSuite,
MafsGraphProps,
PairOfPoints,
SegmentGraphState,
} from "../types";
import type {vec} from "mafs";
Expand All @@ -24,33 +29,123 @@ export function renderSegmentGraph(

type SegmentProps = MafsGraphProps<SegmentGraphState>;

const SegmentGraph = (props: SegmentProps) => {
const {dispatch} = props;
const {coords: segments} = props.graphState;
const SegmentGraph = ({dispatch, graphState}: SegmentProps) => {
const {coords: segments} = graphState;
const {strings, locale} = usePerseusI18n();

function getLengthOfSegment(segment: PairOfPoints) {
return kpoint.distanceToPoint(...segment);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit (Optional): Since this component is not using stuff inside the component (everything it needs is passed into the parameters), you can move it outside the component, maybe at the top of this file. That way it won't re-initialize on every render.

This won't really make a difference in performance in this particular case, so it's really up to you, but I believe that's considered good practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I meant to write "Since this function is not using stuff inside the component"


function getWholeSegmentGraphAriaLabel(): string {
return segments?.length > 1
? strings.srMultipleSegmentGraphAriaLabel({
countOfSegments: segments.length,
})
: strings.srSingleSegmentGraphAriaLabel;
}

const wholeSegmentGraphAriaLabel = getWholeSegmentGraphAriaLabel();

function getIndividualSegmentAriaDescription(
segment: PairOfPoints,
index: number,
) {
return strings.srIndividualSegmentAriaDescription({
point1X: srFormatNumber(segment[0][X], locale),
point1Y: srFormatNumber(segment[0][Y], locale),
point2X: srFormatNumber(segment[1][X], locale),
point2Y: srFormatNumber(segment[1][Y], locale),
length: srFormatNumber(getLengthOfSegment(segment), locale),
indexOfSegment: index + 1,
});
anakaren-rojas marked this conversation as resolved.
Show resolved Hide resolved
}

function getWholeSegmentGraphAriaDescription() {
let description = `${wholeSegmentGraphAriaLabel} `;

segments.forEach((segment, index) => {
description +=
getIndividualSegmentAriaDescription(segment, index) + " ";
});

return description;
}

function formatSegment(
endpointNumber: number,
x: number,
y: number,
index: number,
) {
const segObj = {
endpointNumber: endpointNumber,
x: srFormatNumber(x, locale),
y: srFormatNumber(y, locale),
};

return segments.length > 1
? strings.srMultipleSegmentGraphEndpointAriaLabel({
...segObj,
indexOfSegment: index,
})
: strings.srSingleSegmentGraphEndpointAriaLabel(segObj);
}

return (
<>
<g
aria-label={wholeSegmentGraphAriaLabel}
aria-describedby="wholeSegmentGraphAriaDescription"
>
{segments?.map((segment, i) => (
<MovableLine
key={i}
points={segment}
onMoveLine={(delta: vec.Vector2) => {
dispatch(actions.segment.moveLine(i, delta));
}}
onMovePoint={(
endpointIndex: number,
destination: vec.Vector2,
) => {
dispatch(
actions.segment.movePointInFigure(
i,
endpointIndex,
destination,
<g aria-describedby={`segment-description-${i}`} key={i}>
<MovableLine
key={i}
points={segment}
onMoveLine={(delta: vec.Vector2) => {
dispatch(actions.segment.moveLine(i, delta));
}}
onMovePoint={(
endpointIndex: number,
destination: vec.Vector2,
) => {
dispatch(
actions.segment.movePointInFigure(
i,
endpointIndex,
destination,
),
);
}}
ariaLabels={{
point1AriaLabel: formatSegment(
1,
segment[0][X],
segment[0][Y],
i + 1,
),
point2AriaLabel: formatSegment(
2,
segment[1][X],
segment[1][Y],
i + 1,
),
);
}}
/>
}}
/>
<g
id={`segment-description-${i}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

You still need to use React.useId() to make sure that it's referring to the correct description if there are multiple interactive graphs on the page.

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 I see, you're talking about having multiple instances of the line segment graph on a single page, right?
I wonder if the ID creation should be bubbled up to the Mafs graph to ensure that we're not getting the same ID in multiple instances for all the graphs, then the individual graphs can consume the IDs.
It looks like we already have a unique ID being generated there, we would just need to consume it

Copy link
Contributor

Choose a reason for hiding this comment

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

You can bubble it down, but I think just adding another React.useId() at the top of the current file is much less work. Then you don't have to bubble it down into everything and keep passing it around everywhere even when not strictly necessary.

I'm not aware of performance issues with React.useId() so I'd personally go with that approach myself, but it's up to you.

style={{display: "hidden"}}
>
{getIndividualSegmentAriaDescription(segment, i)}
</g>
</g>
))}
</>
<g
style={{display: "hidden"}}
id="wholeSegmentGraphAriaDescription"
>
{getWholeSegmentGraphAriaDescription()}
</g>
</g>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@ describe("MafsGraph", () => {
/>,
);

expectLabelInDoc("Point 1 at 0 comma 0");
expectLabelInDoc("Point 2 at -7 comma 0.5");
expectLabelInDoc("Endpoint 1 at 0 comma 0.");
expectLabelInDoc("Endpoint 2 at -7 comma 0.5.");
});

it("renders ARIA labels for each point (multiple segments)", () => {
Expand Down Expand Up @@ -187,10 +187,10 @@ describe("MafsGraph", () => {
/>,
);

expectLabelInDoc("Point 1 at 0 comma 0");
expectLabelInDoc("Point 2 at -7 comma 0.5");
expectLabelInDoc("Point 1 at 1 comma 1");
expectLabelInDoc("Point 2 at 7 comma 0.5");
expectLabelInDoc("Endpoint 1 on segment 1 at 0 comma 0.");
expectLabelInDoc("Endpoint 2 on segment 1 at -7 comma 0.5.");
expectLabelInDoc("Endpoint 1 on segment 2 at 1 comma 1.");
expectLabelInDoc("Endpoint 2 on segment 2 at 7 comma 0.5.");
});

it("renders ARIA labels for each point (linear)", () => {
Expand Down
Loading