From 7eb276125129f7e4f4503de00f04777859b114c8 Mon Sep 17 00:00:00 2001 From: Anakaren Rojas Date: Fri, 22 Nov 2024 16:01:01 -0800 Subject: [PATCH 1/8] update use control point and movable point with optional props --- .../__snapshots__/movable-line.test.tsx.snap | 12 +++---- .../graphs/components/movable-point.tsx | 12 ++++--- .../graphs/components/use-control-point.tsx | 33 ++++++++++++------- .../src/widgets/interactive-graphs/types.ts | 2 ++ 4 files changed, 37 insertions(+), 22 deletions(-) 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 4c129238f2..560d781cdf 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,7 +16,7 @@ exports[`Rendering Does NOT render extensions of line when option is disabled 1` > unknown; - onClick?: () => unknown; + ariaDescribedBy?: string; + ariaLabel?: string; + ariaLive?: AriaLive; color?: string; - cursor?: CSSCursor | undefined; constrain?: KeyboardMovementConstraint; - onFocus?: ((event: React.FocusEvent) => unknown) | undefined; + cursor?: CSSCursor | undefined; onBlur?: ((event: React.FocusEvent) => unknown) | undefined; + onClick?: () => unknown; + onFocus?: ((event: React.FocusEvent) => unknown) | undefined; + onMove?: (newPoint: vec.Vector2) => unknown; }; export const MovablePoint = React.forwardRef( diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx index 6687b79543..9111d7dd4d 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx @@ -12,11 +12,10 @@ import {MovablePointView} from "./movable-point-view"; import type {CSSCursor} from "./css-cursor"; import type {KeyboardMovementConstraint} from "../use-draggable"; import type {vec} from "mafs"; +import { AriaLive } from "../../types"; type Params = { point: vec.Vector2; - color?: string | undefined; - cursor?: CSSCursor | undefined; /** * Represents where this point stands in the overall point sequence. * This is used to provide screen readers with context about the point. @@ -26,13 +25,18 @@ type Params = { * interactive figure on the graph. */ sequenceNumber: number; + ariaDescribedBy?: string; + ariaLabel?: string; + ariaLive?: AriaLive; + color?: string | undefined; + cursor?: CSSCursor | undefined; constrain?: KeyboardMovementConstraint; + // The focusableHandle element is assigned to the forwarded ref. + forwardedRef?: React.ForwardedRef | undefined; onMove?: ((newPoint: vec.Vector2) => unknown) | undefined; onClick?: (() => unknown) | undefined; onFocus?: ((event: React.FocusEvent) => unknown) | undefined; onBlur?: ((event: React.FocusEvent) => unknown) | undefined; - // The focusableHandle element is assigned to the forwarded ref. - forwardedRef?: React.ForwardedRef | undefined; }; type Return = { @@ -47,6 +51,9 @@ export function useControlPoint(params: Params): Return { const { point, sequenceNumber, + ariaDescribedBy, + ariaLabel, + ariaLive = "polite", color, cursor, constrain = (p) => snap(snapStep, p), @@ -76,6 +83,13 @@ export function useControlPoint(params: Params): Return { constrainKeyboardMovement: constrain, }); + // if custom aria label is not provided, will use default of sequence number and point coordinates + const pointAriaLabel = ariaLabel || strings.srPointAtCoordinates({ + num: sequenceNumber, + x: srFormatNumber(point[X], locale), + y: srFormatNumber(point[Y], locale), + }) + useLayoutEffect(() => { setForwardedRef(forwardedRef, focusableHandleRef.current); }, [forwardedRef]); @@ -87,14 +101,9 @@ export function useControlPoint(params: Params): Return { tabIndex={disableKeyboardInteraction ? -1 : 0} ref={focusableHandleRef} role="button" - aria-label={strings.srPointAtCoordinates({ - num: sequenceNumber, - x: srFormatNumber(point[X], locale), - y: srFormatNumber(point[Y], locale), - })} - // aria-live="assertive" causes the new location of the point to be - // announced immediately on move. - aria-live="assertive" + aria-describedby={ariaDescribedBy} + aria-label={pointAriaLabel} + aria-live={ariaLive} onFocus={(event) => { onFocus(event); setFocused(true); diff --git a/packages/perseus/src/widgets/interactive-graphs/types.ts b/packages/perseus/src/widgets/interactive-graphs/types.ts index 565643ca81..140cea9f02 100644 --- a/packages/perseus/src/widgets/interactive-graphs/types.ts +++ b/packages/perseus/src/widgets/interactive-graphs/types.ts @@ -135,3 +135,5 @@ export type GraphDimensions = { width: number; // pixels height: number; // pixels }; + +export type AriaLive = "off" | "assertive" | "polite" | undefined; From 83839b508a0f2ceeaafd3b66a7ae3f5fdd83a08d Mon Sep 17 00:00:00 2001 From: Anakaren Rojas Date: Mon, 25 Nov 2024 09:28:18 -0800 Subject: [PATCH 2/8] lint fix --- .../graphs/components/movable-point.tsx | 2 +- .../graphs/components/use-control-point.tsx | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx index f02705848b..1bfa77a905 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx @@ -3,9 +3,9 @@ import * as React from "react"; import {useControlPoint} from "./use-control-point"; import type {CSSCursor} from "./css-cursor"; +import type {AriaLive} from "../../types"; import type {KeyboardMovementConstraint} from "../use-draggable"; import type {vec} from "mafs"; -import { AriaLive } from "../../types"; type Props = { point: vec.Vector2; diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx index 9111d7dd4d..a24c8672a5 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx @@ -10,9 +10,9 @@ import {useDraggable} from "../use-draggable"; import {MovablePointView} from "./movable-point-view"; import type {CSSCursor} from "./css-cursor"; +import type {AriaLive} from "../../types"; import type {KeyboardMovementConstraint} from "../use-draggable"; import type {vec} from "mafs"; -import { AriaLive } from "../../types"; type Params = { point: vec.Vector2; @@ -84,11 +84,13 @@ export function useControlPoint(params: Params): Return { }); // if custom aria label is not provided, will use default of sequence number and point coordinates - const pointAriaLabel = ariaLabel || strings.srPointAtCoordinates({ - num: sequenceNumber, - x: srFormatNumber(point[X], locale), - y: srFormatNumber(point[Y], locale), - }) + const pointAriaLabel = + ariaLabel || + strings.srPointAtCoordinates({ + num: sequenceNumber, + x: srFormatNumber(point[X], locale), + y: srFormatNumber(point[Y], locale), + }); useLayoutEffect(() => { setForwardedRef(forwardedRef, focusableHandleRef.current); From 4b4236cca651fe18f24f14bc88ba459106bcd692 Mon Sep 17 00:00:00 2001 From: Anakaren Rojas Date: Mon, 25 Nov 2024 09:53:30 -0800 Subject: [PATCH 3/8] changeset --- .changeset/lazy-geckos-suffer.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/lazy-geckos-suffer.md diff --git a/.changeset/lazy-geckos-suffer.md b/.changeset/lazy-geckos-suffer.md new file mode 100644 index 0000000000..9b5853b83f --- /dev/null +++ b/.changeset/lazy-geckos-suffer.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus": patch +--- + +update moveable point component and use control point method to have optional params From f01b2b4feb52f0cd8aecaf8b8b29ab1aba223fb6 Mon Sep 17 00:00:00 2001 From: Anakaren Rojas Date: Mon, 25 Nov 2024 11:02:25 -0800 Subject: [PATCH 4/8] add accessibility tests for movable point --- .../graphs/components/movable-point.test.tsx | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx index 847b6d521c..0f363736b9 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx @@ -285,4 +285,80 @@ describe("MovablePoint", () => { expect(focusSpy).toHaveBeenCalledTimes(1); }); + + describe("accessibility", () => { + it("uses the default ariaLabel when not provided", () => { + render( + + + + ); + + expect(screen.getByLabelText("Point 1 at 0 comma 0")).toBeInTheDocument(); + }); + + it("uses the ariaLabel when provided", () => { + render( + + + + ); + + expect(screen.getByLabelText("Custom aria label")).toBeInTheDocument(); + }) + + it("uses the ariaDescribedBy when provided", () => { + render( + + +

Aria is described by me

+
+ ); + + const pointElement = screen.getByRole("button", { name: "Point 1 at 0 comma 0" }); + expect(pointElement).toHaveAttribute("aria-describedby", "description"); + + const descriptionElement = screen.getByText("Aria is described by me"); + expect(descriptionElement).toBeInTheDocument(); + }); + + it("uses the ariaLive when provided", () => { + render( + + + + ); + + expect(screen.getByLabelText("Point 1 at 0 comma 0")).toHaveAttribute( + "aria-live", + "assertive", + ); + }); + + it("uses the default ariaLive when not provided", () => { + render( + + + + ); + + expect(screen.getByLabelText("Point 1 at 0 comma 0")).toHaveAttribute( + "aria-live", + "polite", + ); + }); + }) + }); From 059baef48a80bf3f9bf9941a7c828eb1ae925155 Mon Sep 17 00:00:00 2001 From: Anakaren Rojas Date: Mon, 25 Nov 2024 11:47:21 -0800 Subject: [PATCH 5/8] lint fix --- .../graphs/components/movable-point.test.tsx | 50 +++++++++++-------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx index 0f363736b9..4325ceaed7 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx @@ -291,10 +291,12 @@ describe("MovablePoint", () => { render( - + , ); - expect(screen.getByLabelText("Point 1 at 0 comma 0")).toBeInTheDocument(); + expect( + screen.getByLabelText("Point 1 at 0 comma 0"), + ).toBeInTheDocument(); }); it("uses the ariaLabel when provided", () => { @@ -305,11 +307,13 @@ describe("MovablePoint", () => { sequenceNumber={1} ariaLabel="Custom aria label" /> - + , ); - expect(screen.getByLabelText("Custom aria label")).toBeInTheDocument(); - }) + expect( + screen.getByLabelText("Custom aria label"), + ).toBeInTheDocument(); + }); it("uses the ariaDescribedBy when provided", () => { render( @@ -320,13 +324,20 @@ describe("MovablePoint", () => { ariaDescribedBy="description" />

Aria is described by me

- + , ); - const pointElement = screen.getByRole("button", { name: "Point 1 at 0 comma 0" }); - expect(pointElement).toHaveAttribute("aria-describedby", "description"); + const pointElement = screen.getByRole("button", { + name: "Point 1 at 0 comma 0", + }); + expect(pointElement).toHaveAttribute( + "aria-describedby", + "description", + ); - const descriptionElement = screen.getByText("Aria is described by me"); + const descriptionElement = screen.getByText( + "Aria is described by me", + ); expect(descriptionElement).toBeInTheDocument(); }); @@ -338,27 +349,24 @@ describe("MovablePoint", () => { sequenceNumber={1} ariaLive="assertive" /> - + , ); - expect(screen.getByLabelText("Point 1 at 0 comma 0")).toHaveAttribute( - "aria-live", - "assertive", - ); + expect( + screen.getByLabelText("Point 1 at 0 comma 0"), + ).toHaveAttribute("aria-live", "assertive"); }); it("uses the default ariaLive when not provided", () => { render( - + , ); - expect(screen.getByLabelText("Point 1 at 0 comma 0")).toHaveAttribute( - "aria-live", - "polite", - ); + expect( + screen.getByLabelText("Point 1 at 0 comma 0"), + ).toHaveAttribute("aria-live", "polite"); }); - }) - + }); }); From 8bcc3b79aa01fef74a9953973ccd0b2aaf64f481 Mon Sep 17 00:00:00 2001 From: Anakaren Rojas Date: Tue, 26 Nov 2024 09:46:56 -0800 Subject: [PATCH 6/8] make sequenceNumber optional --- .../graphs/components/movable-point.test.tsx | 44 ++++++++++++++----- .../graphs/components/movable-point.tsx | 14 +++--- .../graphs/components/use-control-point.tsx | 22 +++++----- 3 files changed, 52 insertions(+), 28 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx index 4325ceaed7..02b467560d 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.test.tsx @@ -287,10 +287,10 @@ describe("MovablePoint", () => { }); describe("accessibility", () => { - it("uses the default ariaLabel when not provided", () => { + it("uses the default sequence number when ariaLabel and sequence number are not provided", () => { render( - + , ); @@ -299,12 +299,41 @@ describe("MovablePoint", () => { ).toBeInTheDocument(); }); - it("uses the ariaLabel when provided", () => { + it("uses sequence number when sequence is provided and aria label is not provided", () => { + render( + + + , + ); + + expect( + screen.getByLabelText("Point 2 at 0 comma 0"), + ).toBeInTheDocument(); + }); + + it("uses the ariaLabel when both sequence and ariaLabel are provided", () => { render( + , + ); + + expect( + screen.getByLabelText( + "Aria Label being used instead of sequence number", + ), + ).toBeInTheDocument(); + }); + + it("uses the ariaLabel when only ariaLabel is provided", () => { + render( + + , @@ -320,7 +349,6 @@ describe("MovablePoint", () => {

Aria is described by me

@@ -344,11 +372,7 @@ describe("MovablePoint", () => { it("uses the ariaLive when provided", () => { render( - + , ); @@ -360,7 +384,7 @@ describe("MovablePoint", () => { it("uses the default ariaLive when not provided", () => { render( - + , ); diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx index 1bfa77a905..02d7d1dc82 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx @@ -9,6 +9,12 @@ import type {vec} from "mafs"; type Props = { point: vec.Vector2; + ariaDescribedBy?: string; + ariaLabel?: string; + ariaLive?: AriaLive; + color?: string; + constrain?: KeyboardMovementConstraint; + cursor?: CSSCursor | undefined; /** * Represents where this point stands in the overall point sequence. * This is used to provide screen readers with context about the point. @@ -17,13 +23,7 @@ type Props = { * Note: This number is 1-indexed, and should restart from 1 for each * interactive figure on the graph. */ - sequenceNumber: number; - ariaDescribedBy?: string; - ariaLabel?: string; - ariaLive?: AriaLive; - color?: string; - constrain?: KeyboardMovementConstraint; - cursor?: CSSCursor | undefined; + sequenceNumber?: number; onBlur?: ((event: React.FocusEvent) => unknown) | undefined; onClick?: () => unknown; onFocus?: ((event: React.FocusEvent) => unknown) | undefined; diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx index a24c8672a5..65adc3c53a 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx @@ -16,15 +16,6 @@ import type {vec} from "mafs"; type Params = { point: vec.Vector2; - /** - * Represents where this point stands in the overall point sequence. - * This is used to provide screen readers with context about the point. - * Example: sequenceNumber={1} ==> "Point 1 at x comma y" - * - * Note: This number is 1-indexed, and should restart from 1 for each - * interactive figure on the graph. - */ - sequenceNumber: number; ariaDescribedBy?: string; ariaLabel?: string; ariaLive?: AriaLive; @@ -33,6 +24,15 @@ type Params = { constrain?: KeyboardMovementConstraint; // The focusableHandle element is assigned to the forwarded ref. forwardedRef?: React.ForwardedRef | undefined; + /** + * Represents where this point stands in the overall point sequence. + * This is used to provide screen readers with context about the point. + * Example: sequenceNumber={1} ==> "Point 1 at x comma y" + * + * Note: This number is 1-indexed, and should restart from 1 for each + * interactive figure on the graph. + */ + sequenceNumber?: number; onMove?: ((newPoint: vec.Vector2) => unknown) | undefined; onClick?: (() => unknown) | undefined; onFocus?: ((event: React.FocusEvent) => unknown) | undefined; @@ -50,18 +50,18 @@ export function useControlPoint(params: Params): Return { const {snapStep, disableKeyboardInteraction} = useGraphConfig(); const { point, - sequenceNumber, ariaDescribedBy, ariaLabel, ariaLive = "polite", color, cursor, constrain = (p) => snap(snapStep, p), + forwardedRef = noop, + sequenceNumber = 1, onMove = noop, onClick = noop, onFocus = noop, onBlur = noop, - forwardedRef = noop, } = params; const {strings, locale} = usePerseusI18n(); From 7c19062fa0d8d17cdfc38c9aa5efb297c5ba85b4 Mon Sep 17 00:00:00 2001 From: Anakaren Rojas Date: Tue, 26 Nov 2024 13:44:34 -0800 Subject: [PATCH 7/8] remove unneeded undefined --- .../interactive-graphs/graphs/components/movable-point.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx index 02d7d1dc82..b855da3e1c 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx @@ -24,9 +24,9 @@ type Props = { * interactive figure on the graph. */ sequenceNumber?: number; - onBlur?: ((event: React.FocusEvent) => unknown) | undefined; + onBlur?: (event: React.FocusEvent) => unknown; onClick?: () => unknown; - onFocus?: ((event: React.FocusEvent) => unknown) | undefined; + onFocus?: (event: React.FocusEvent) => unknown; onMove?: (newPoint: vec.Vector2) => unknown; }; From 0ec45c5087b3ac9dc5777220e6d9133883b9fb58 Mon Sep 17 00:00:00 2001 From: Anakaren Rojas Date: Tue, 26 Nov 2024 13:46:05 -0800 Subject: [PATCH 8/8] reorder params --- .../graphs/components/use-control-point.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx index 65adc3c53a..0cc35e3f6b 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/components/use-control-point.tsx @@ -20,8 +20,8 @@ type Params = { ariaLabel?: string; ariaLive?: AriaLive; color?: string | undefined; - cursor?: CSSCursor | undefined; constrain?: KeyboardMovementConstraint; + cursor?: CSSCursor | undefined; // The focusableHandle element is assigned to the forwarded ref. forwardedRef?: React.ForwardedRef | undefined; /** @@ -54,8 +54,8 @@ export function useControlPoint(params: Params): Return { ariaLabel, ariaLive = "polite", color, - cursor, constrain = (p) => snap(snapStep, p), + cursor, forwardedRef = noop, sequenceNumber = 1, onMove = noop,