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 6 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tests! 🎉

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) | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to explicitly have | undefined if it already has the ?? Same question with onFocus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I kept them as they were, but it would be safe to remove the | undefined

onClick?: () => unknown;
color?: string;
cursor?: CSSCursor | undefined;
constrain?: KeyboardMovementConstraint;
onFocus?: ((event: React.FocusEvent) => unknown) | undefined;
onBlur?: ((event: React.FocusEvent) => unknown) | undefined;
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;
cursor?: CSSCursor | undefined;
constrain?: KeyboardMovementConstraint;
// 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),
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;
Loading