From 30333ff7b2b0c5dccee63c45b5ef98a6315c43d2 Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Tue, 17 Dec 2024 17:04:33 -0800 Subject: [PATCH 1/3] [SR] Linear graph - add grab handle description and aria lives 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. --- packages/perseus/src/strings.ts | 19 ++++ .../__snapshots__/movable-line.test.tsx.snap | 18 ++- .../graphs/components/movable-line.tsx | 63 +++++++++-- .../interactive-graphs/graphs/linear.test.tsx | 103 ++++++++++++++++-- .../interactive-graphs/graphs/linear.tsx | 7 ++ 5 files changed, 186 insertions(+), 24 deletions(-) diff --git a/packages/perseus/src/strings.ts b/packages/perseus/src/strings.ts index 58ccd2b6d7..6df397b3da 100644 --- a/packages/perseus/src/strings.ts +++ b/packages/perseus/src/strings.ts @@ -209,6 +209,17 @@ export type PerseusStrings = { yIntercept: string; }) => string; srLinearGraphOriginIntercept: string; + srLinearGrabHandle: ({ + point1X, + point1Y, + point2X, + point2Y, + }: { + point1X: string; + point1Y: string; + point2X: string; + point2Y: string; + }) => string; // The above strings are used for interactive graph SR descriptions. }; @@ -478,6 +489,12 @@ export const strings: { "Screenreader-only description of the line's intercept when the intercept is the graph's origin.", message: "The line crosses the x and y axes at the graph's origin.", }, + srLinearGrabHandle: { + context: + "Screenreader-only label on the grab handle for the line on a linear graph.", + message: + "Line from %(point1X)s comma %(point1Y)s to %(point2X)s comma %(point2Y)s.", + }, // The above strings are used for interactive graph SR descriptions. }; @@ -678,5 +695,7 @@ export const mockStrings: PerseusStrings = { `The line crosses the X-axis at ${xIntercept} comma 0 and the Y-axis at 0 comma ${yIntercept}.`, srLinearGraphOriginIntercept: "The line crosses the x and y axes at the graph's origin.", + srLinearGrabHandle: ({point1X, point1Y, point2X, point2Y}) => + `Line from ${point1X} comma ${point1Y} to ${point2X} comma ${point2Y}.`, // The above strings are used for interactive graph SR descriptions. }; diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/__snapshots__/movable-line.test.tsx.snap b/packages/perseus/src/widgets/interactive-graphs/graphs/components/__snapshots__/movable-line.test.tsx.snap index 560d781cdf..b3e109b8af 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/__snapshots__/movable-line.test.tsx.snap +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/__snapshots__/movable-line.test.tsx.snap @@ -16,15 +16,17 @@ exports[`Rendering Does NOT render extensions of line when option is disabled 1` > @@ -61,7 +63,7 @@ exports[`Rendering Does NOT render extensions of line when option is disabled 1` @@ -201,7 +205,7 @@ exports[`Rendering Does NOT render extensions of line when option is not provide @@ -395,7 +401,7 @@ exports[`Rendering Does render extensions of line when option is enabled 1`] = ` { const { + points: [start, end], ariaLabels, ariaDescribedBy, - onMoveLine = () => {}, - onMovePoint = () => {}, color, - points: [start, end], extend, + onMoveLine = () => {}, + onMovePoint = () => {}, } = props; + // Aria live states for (0) point 1, (1) point 2, and (2) grab handle. + // When moving an element, set its aria live to "polite" and the others + // to "off". Otherwise, other connected elements that move at the same + // time might override the currently focused element's aria live. + const [ariaLives, setAriaLives] = React.useState>([ + "off", + "off", + "off", + ]); + // We use separate focusableHandle elements, instead of letting the movable // points themselves be focusable, to allow the tab order of the points to // be different from the rendering order. We had to solve for the following @@ -59,28 +71,42 @@ export const MovableLine = (props: Props) => { useControlPoint({ ariaLabel: ariaLabels?.point1AriaLabel, ariaDescribedBy: ariaDescribedBy, + ariaLive: ariaLives[0], point: start, sequenceNumber: 1, color, - onMove: (p) => onMovePoint(0, p), + onMove: (p) => { + setAriaLives(["polite", "off", "off"]); + onMovePoint(0, p); + }, }); const {visiblePoint: visiblePoint2, focusableHandle: focusableHandle2} = useControlPoint({ ariaLabel: ariaLabels?.point2AriaLabel, ariaDescribedBy: ariaDescribedBy, + ariaLive: ariaLives[1], point: end, sequenceNumber: 2, color, - onMove: (p) => onMovePoint(1, p), + onMove: (p) => { + setAriaLives(["off", "polite", "off"]); + onMovePoint(1, p); + }, }); const line = ( { + setAriaLives(["off", "off", "polite"]); + onMoveLine(delta); + }} /> ); @@ -100,8 +126,9 @@ const defaultStroke = "var(--movable-line-stroke-color)"; type LineProps = { start: vec.Vector2; end: vec.Vector2; - onMove: (delta: vec.Vector2) => unknown; - stroke?: string | undefined; + ariaLabel?: string; + ariaDescribedBy?: string; + ariaLive?: AriaLive; /* Extends the line to the edge of the graph with an arrow */ extend?: | undefined @@ -109,10 +136,21 @@ type LineProps = { start: boolean; end: boolean; }; + stroke?: string | undefined; + onMove: (delta: vec.Vector2) => unknown; }; const Line = (props: LineProps) => { - const {start, end, onMove, extend, stroke = defaultStroke} = props; + const { + start, + end, + ariaLabel, + ariaDescribedBy, + ariaLive, + extend, + stroke = defaultStroke, + onMove, + } = props; const [startPtPx, endPtPx] = useTransformVectorsToPixels(start, end); const { @@ -150,9 +188,16 @@ const Line = (props: LineProps) => { {/** * This transparent line creates a nice big click/touch target. diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx index 6953d153b3..5b514f5879 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx @@ -1,4 +1,5 @@ import {render, screen} from "@testing-library/react"; +import {userEvent as userEventLib} from "@testing-library/user-event"; import * as React from "react"; import {Dependencies} from "@khanacademy/perseus"; @@ -8,6 +9,7 @@ import {MafsGraph} from "../mafs-graph"; import {getBaseMafsGraphPropsForTests} from "../utils"; import type {InteractiveGraphState} from "../types"; +import type {UserEvent} from "@testing-library/user-event"; const baseMafsGraphProps = getBaseMafsGraphPropsForTests(); const baseLinearState: InteractiveGraphState = { @@ -27,7 +29,11 @@ const baseLinearState: InteractiveGraphState = { const overallGraphLabel = "A line on a coordinate plane."; describe("Linear graph screen reader", () => { + let userEvent: UserEvent; beforeEach(() => { + userEvent = userEventLib.setup({ + advanceTimers: jest.advanceTimersByTime, + }); jest.spyOn(Dependencies, "getDependencies").mockReturnValue( testDependencies, ); @@ -50,21 +56,36 @@ describe("Linear graph screen reader", () => { ); }); - test("should have aria labels for both points on the line", () => { + test("should have aria labels and describedbys for both points and grab handle on the line", () => { // Arrange render(); // Act - // eslint-disable-next-line testing-library/no-node-access - const points = screen.getAllByTestId("movable-point__focusable-handle"); + // Moveable elements: point 1, grab handle, point 2 + const movableElements = screen.getAllByRole("button"); + const [point1, grabHandle, point2] = movableElements; // Assert - // Check aria-label for both points on the line. - expect(points[0]).toHaveAttribute( + // Check aria-label and describedby on interactive elements. + // (The actual description text is tested separately below.) + expect(point1).toHaveAttribute("aria-label", "Point 1 at -5 comma 5"); + // We don't know the exact ID because of React.useID(), but we can + // check the suffix. + expect(point1.getAttribute("aria-describedby")).toContain("-intercept"); + expect(point1.getAttribute("aria-describedby")).toContain("-slope"); + + expect(grabHandle).toHaveAttribute( "aria-label", - "Point 1 at -5 comma 5", + "Line from -5 comma 5 to 5 comma 5.", ); - expect(points[1]).toHaveAttribute("aria-label", "Point 2 at 5 comma 5"); + expect(grabHandle.getAttribute("aria-describedby")).toContain( + "-intercept", + ); + expect(grabHandle.getAttribute("aria-describedby")).toContain("-slope"); + + expect(point2).toHaveAttribute("aria-label", "Point 2 at 5 comma 5"); + expect(point2.getAttribute("aria-describedby")).toContain("-intercept"); + expect(point2.getAttribute("aria-describedby")).toContain("-slope"); }); test("points description should include points info", () => { @@ -87,7 +108,7 @@ describe("Linear graph screen reader", () => { ${"x intercept only"} | ${[[5, 5], [5, 2]]} | ${"The line crosses the X-axis at 5 comma 0."} ${"y intercept only"} | ${[[5, 5], [2, 5]]} | ${"The line crosses the Y-axis at 0 comma 5."} `( - `slope description should include slope info for $case`, + "slope description should include slope info for $case", ({coords, interceptDescription}) => { // Arrange render( @@ -115,7 +136,7 @@ describe("Linear graph screen reader", () => { ${"horizontal line"} | ${[[1, 1], [3, 1]]} | ${"Its slope is zero."} ${"vertical line"} | ${[[1, 1], [1, 3]]} | ${"Its slope is undefined."} `( - `slope description should include slope info for $case`, + "slope description should include slope info for $case", ({coords, slopeDescription}) => { // Arrange render( @@ -135,4 +156,68 @@ describe("Linear graph screen reader", () => { expect(linearGraph).toHaveTextContent(slopeDescription); }, ); + + test("aria label reflects updated values", async () => { + // Arrange + + // Act + render( + , + ); + + const interactiveElements = screen.getAllByRole("button"); + const [point1, grabHandle, point2] = interactiveElements; + + // Assert + // Check updated aria-label for the linear graph. + expect(point1).toHaveAttribute("aria-label", "Point 1 at -2 comma 3"); + expect(grabHandle).toHaveAttribute( + "aria-label", + "Line from -2 comma 3 to 3 comma 3.", + ); + expect(point2).toHaveAttribute("aria-label", "Point 2 at 3 comma 3"); + }); + + test.each` + elementName | index + ${"point1"} | ${0} + ${"grabHandle"} | ${1} + ${"point2"} | ${2} + `( + "Should update the aria-live when $elementName is moved", + async ({index}) => { + // Arrange + render( + , + ); + const interactiveElements = screen.getAllByRole("button"); + const [point1, grabHandle, point2] = interactiveElements; + const movingElement = interactiveElements[index]; + + // Act - Move the element + movingElement.focus(); + await userEvent.keyboard("{ArrowRight}"); + + const expectedAriaLive = ["off", "off", "off"]; + expectedAriaLive[index] = "polite"; + + // Assert + expect(point1).toHaveAttribute("aria-live", expectedAriaLive[0]); + expect(grabHandle).toHaveAttribute( + "aria-live", + expectedAriaLive[1], + ); + expect(point2).toHaveAttribute("aria-live", expectedAriaLive[2]); + }, + ); }); diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx index b20f1d25c4..ccee745c4f 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx @@ -43,6 +43,12 @@ const LinearGraph = (props: LinearGraphProps, key: number) => { point2X: srFormatNumber(line[1][0], locale), point2Y: srFormatNumber(line[1][1], locale), }); + const grabHandleAriaLabel = strings.srLinearGrabHandle({ + point1X: srFormatNumber(line[0][0], locale), + point1Y: srFormatNumber(line[0][1], locale), + point2X: srFormatNumber(line[1][0], locale), + point2Y: srFormatNumber(line[1][1], locale), + }); // Slope description const slope = (line[1][1] - line[0][1]) / (line[1][0] - line[0][0]); @@ -96,6 +102,7 @@ const LinearGraph = (props: LinearGraphProps, key: number) => { > { From 586c962d7ce086c605ff940ed9ececca57f9673f Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Tue, 17 Dec 2024 17:08:20 -0800 Subject: [PATCH 2/3] docs(changeset): [SR] Linear graph - add grab handle description and aria lives --- .changeset/cyan-bees-appear.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/cyan-bees-appear.md diff --git a/.changeset/cyan-bees-appear.md b/.changeset/cyan-bees-appear.md new file mode 100644 index 0000000000..8b03f8ce2d --- /dev/null +++ b/.changeset/cyan-bees-appear.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus": patch +--- + +[SR] Linear graph - add grab handle description and aria lives From 6c7ecb5dd5f2037333d212bbef64ada1e75ad3d0 Mon Sep 17 00:00:00 2001 From: Nisha Yerunkar Date: Wed, 18 Dec 2024 16:19:27 -0800 Subject: [PATCH 3/3] Address PR comments - update tests, use figure role on whole graph --- .../interactive-graphs/graphs/linear.test.tsx | 52 +++++++++++++------ .../interactive-graphs/graphs/linear.tsx | 1 + 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx index 5b514f5879..68b87e4a48 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx @@ -56,36 +56,54 @@ describe("Linear graph screen reader", () => { ); }); - test("should have aria labels and describedbys for both points and grab handle on the line", () => { + test.each` + element | index | expectedValue + ${"point1"} | ${0} | ${"Point 1 at -5 comma 5"} + ${"grabHandle"} | ${1} | ${"Line from -5 comma 5 to 5 comma 5."} + ${"point2"} | ${2} | ${"Point 2 at 5 comma 5"} + `( + "should have aria label for $element on the line", + ({index, expectedValue}) => { + // Arrange + render( + , + ); + + // Act + // Moveable elements: point 1, grab handle, point 2 + const movableElements = screen.getAllByRole("button"); + const element = movableElements[index]; + + // Assert + // Check aria-label and describedby on interactive elements. + // (The actual description text is tested separately below.) + expect(element).toHaveAttribute("aria-label", expectedValue); + }, + ); + + test.each` + element | index + ${"point1"} | ${0} + ${"grabHandle"} | ${1} + ${"point2"} | ${2} + `("should have aria describedby for $element on the line", ({index}) => { // Arrange render(); // Act // Moveable elements: point 1, grab handle, point 2 const movableElements = screen.getAllByRole("button"); - const [point1, grabHandle, point2] = movableElements; + const element = movableElements[index]; // Assert - // Check aria-label and describedby on interactive elements. + // Check aria-describedby on interactive elements. // (The actual description text is tested separately below.) - expect(point1).toHaveAttribute("aria-label", "Point 1 at -5 comma 5"); // We don't know the exact ID because of React.useID(), but we can // check the suffix. - expect(point1.getAttribute("aria-describedby")).toContain("-intercept"); - expect(point1.getAttribute("aria-describedby")).toContain("-slope"); - - expect(grabHandle).toHaveAttribute( - "aria-label", - "Line from -5 comma 5 to 5 comma 5.", - ); - expect(grabHandle.getAttribute("aria-describedby")).toContain( + expect(element.getAttribute("aria-describedby")).toContain( "-intercept", ); - expect(grabHandle.getAttribute("aria-describedby")).toContain("-slope"); - - expect(point2).toHaveAttribute("aria-label", "Point 2 at 5 comma 5"); - expect(point2.getAttribute("aria-describedby")).toContain("-intercept"); - expect(point2.getAttribute("aria-describedby")).toContain("-slope"); + expect(element.getAttribute("aria-describedby")).toContain("-slope"); }); test("points description should include points info", () => { diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx index ccee745c4f..915084532d 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx @@ -99,6 +99,7 @@ const LinearGraph = (props: LinearGraphProps, key: number) => { // Outer line minimal description aria-label={strings.srLinearGraph} aria-describedby={`${pointsDescriptionId} ${interceptDescriptionId} ${slopeDescriptionId}`} + role="figure" >