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-2664): update use control point and movable point with optional props #1906

Merged
merged 9 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all 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/lazy-geckos-suffer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

update moveable point component and use control point method to have optional params
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ exports[`Rendering Does NOT render extensions of line when option is disabled 1`
>
<g
aria-label="Point 1 at -1 comma -1"
aria-live="assertive"
aria-live="polite"
class="movable-point__focusable-handle"
data-testid="movable-point__focusable-handle"
role="button"
Expand Down Expand Up @@ -61,7 +61,7 @@ exports[`Rendering Does NOT render extensions of line when option is disabled 1`
</g>
<g
aria-label="Point 2 at 1 comma 1"
aria-live="assertive"
aria-live="polite"
class="movable-point__focusable-handle"
data-testid="movable-point__focusable-handle"
role="button"
Expand Down Expand Up @@ -156,7 +156,7 @@ exports[`Rendering Does NOT render extensions of line when option is not provide
>
<g
aria-label="Point 1 at -1 comma -1"
aria-live="assertive"
aria-live="polite"
class="movable-point__focusable-handle"
data-testid="movable-point__focusable-handle"
role="button"
Expand Down Expand Up @@ -201,7 +201,7 @@ exports[`Rendering Does NOT render extensions of line when option is not provide
</g>
<g
aria-label="Point 2 at 1 comma 1"
aria-live="assertive"
aria-live="polite"
class="movable-point__focusable-handle"
data-testid="movable-point__focusable-handle"
role="button"
Expand Down Expand Up @@ -296,7 +296,7 @@ exports[`Rendering Does render extensions of line when option is enabled 1`] = `
>
<g
aria-label="Point 1 at -1 comma -1"
aria-live="assertive"
aria-live="polite"
class="movable-point__focusable-handle"
data-testid="movable-point__focusable-handle"
role="button"
Expand Down Expand Up @@ -395,7 +395,7 @@ exports[`Rendering Does render extensions of line when option is enabled 1`] = `
</g>
<g
aria-label="Point 2 at 1 comma 1"
aria-live="assertive"
aria-live="polite"
class="movable-point__focusable-handle"
data-testid="movable-point__focusable-handle"
role="button"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,4 +285,112 @@ describe("MovablePoint", () => {

expect(focusSpy).toHaveBeenCalledTimes(1);
});

describe("accessibility", () => {
it("uses the default sequence number when ariaLabel and sequence number are not provided", () => {
render(
<Mafs width={200} height={200}>
<MovablePoint point={[0, 0]} />
</Mafs>,
);

expect(
screen.getByLabelText("Point 1 at 0 comma 0"),
).toBeInTheDocument();
});

it("uses sequence number when sequence is provided and aria label is not provided", () => {
render(
<Mafs width={200} height={200}>
<MovablePoint point={[0, 0]} sequenceNumber={2} />
</Mafs>,
);

expect(
screen.getByLabelText("Point 2 at 0 comma 0"),
).toBeInTheDocument();
});

it("uses the ariaLabel when both sequence and ariaLabel are provided", () => {
render(
<Mafs width={200} height={200}>
<MovablePoint
point={[0, 0]}
sequenceNumber={1}
ariaLabel="Aria Label being used instead of sequence number"
/>
</Mafs>,
);

expect(
screen.getByLabelText(
"Aria Label being used instead of sequence number",
),
).toBeInTheDocument();
});

it("uses the ariaLabel when only ariaLabel is provided", () => {
render(
<Mafs width={200} height={200}>
<MovablePoint
point={[0, 0]}
ariaLabel="Custom aria label"
/>
</Mafs>,
);

expect(
screen.getByLabelText("Custom aria label"),
).toBeInTheDocument();
});

it("uses the ariaDescribedBy when provided", () => {
render(
<Mafs width={200} height={200}>
<MovablePoint
point={[0, 0]}
ariaDescribedBy="description"
/>
<p id="description">Aria is described by me</p>
</Mafs>,
);

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(
<Mafs width={200} height={200}>
<MovablePoint point={[0, 0]} ariaLive="assertive" />
</Mafs>,
);

expect(
screen.getByLabelText("Point 1 at 0 comma 0"),
).toHaveAttribute("aria-live", "assertive");
});

it("uses the default ariaLive when not provided", () => {
render(
<Mafs width={200} height={200}>
<MovablePoint point={[0, 0]} />
</Mafs>,
);

expect(
screen.getByLabelText("Point 1 at 0 comma 0"),
).toHaveAttribute("aria-live", "polite");
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,18 @@ 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";

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.
Expand All @@ -16,14 +23,11 @@ type Props = {
* 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;
sequenceNumber?: number;
onBlur?: (event: React.FocusEvent) => unknown;
onClick?: () => unknown;
color?: string;
cursor?: CSSCursor | undefined;
constrain?: KeyboardMovementConstraint;
onFocus?: ((event: React.FocusEvent) => unknown) | undefined;
onBlur?: ((event: React.FocusEvent) => unknown) | undefined;
onFocus?: (event: React.FocusEvent) => unknown;
onMove?: (newPoint: vec.Vector2) => unknown;
};

export const MovablePoint = React.forwardRef(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,20 @@ 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";

type Params = {
point: vec.Vector2;
ariaDescribedBy?: string;
ariaLabel?: string;
ariaLive?: AriaLive;
color?: string | undefined;
constrain?: KeyboardMovementConstraint;
cursor?: CSSCursor | undefined;
// The focusableHandle element is assigned to the forwarded ref.
forwardedRef?: React.ForwardedRef<SVGGElement | null> | undefined;
/**
* Represents where this point stands in the overall point sequence.
* This is used to provide screen readers with context about the point.
Expand All @@ -25,14 +32,11 @@ type Params = {
* Note: This number is 1-indexed, and should restart from 1 for each
* interactive figure on the graph.
*/
sequenceNumber: number;
constrain?: KeyboardMovementConstraint;
sequenceNumber?: number;
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<SVGGElement | null> | undefined;
};

type Return = {
Expand All @@ -46,15 +50,18 @@ export function useControlPoint(params: Params): Return {
const {snapStep, disableKeyboardInteraction} = useGraphConfig();
const {
point,
sequenceNumber,
ariaDescribedBy,
ariaLabel,
ariaLive = "polite",
Copy link
Contributor Author

@anakaren-rojas anakaren-rojas Nov 23, 2024

Choose a reason for hiding this comment

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

Only removing point or moving out of bounds should be assertive per design doc

color,
cursor,
constrain = (p) => snap(snapStep, p),
cursor,
forwardedRef = noop,
sequenceNumber = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

onMove = noop,
onClick = noop,
onFocus = noop,
onBlur = noop,
forwardedRef = noop,
} = params;

const {strings, locale} = usePerseusI18n();
Expand All @@ -76,6 +83,15 @@ 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,
anakaren-rojas marked this conversation as resolved.
Show resolved Hide resolved
x: srFormatNumber(point[X], locale),
y: srFormatNumber(point[Y], locale),
});

useLayoutEffect(() => {
setForwardedRef(forwardedRef, focusableHandleRef.current);
}, [forwardedRef]);
Expand All @@ -87,14 +103,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);
Expand Down
2 changes: 2 additions & 0 deletions packages/perseus/src/widgets/interactive-graphs/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,3 +135,5 @@ export type GraphDimensions = {
width: number; // pixels
height: number; // pixels
};

export type AriaLive = "off" | "assertive" | "polite" | undefined;