Skip to content

Commit

Permalink
[i-like-to-move-it] Ensure that keyboard users can move points across…
Browse files Browse the repository at this point in the history
… invalid locations for all graphs. (#2264)

## Summary:
This PR is part of the Interactive Graph Project! 

The work in this PR ensures that users can move points across invalid locations for all graph types. This required updates to the following Interactive Graph types:

- Quadratic 
- Sinsoid
- Circle
- All Graphs that use Movable Line
  - Segment
  - Linear 
  - Linear System
  - Ray

NOTE: This PR does not change the functionality of the Polygon or Angle Graphs as I believe both have custom implementations that have already been implemented. 

 Issue: LEMS-2014 

## Video:
https://github.com/user-attachments/assets/317d5263-7fe9-450b-b2a9-8a5b19e17134

## Storybook Link:
https://650db21c3f5d1b2f13c02952-ehxlgsxqwf.chromatic.com/

Issue: LEMS-2014

## Test plan:
- All current tests pass
- New tests 
- Manual testing in Storybook

Author: SonicScrewdriver

Reviewers: SonicScrewdriver, nishasy, catandthemachines

Required Reviewers:

Approved By: nishasy

Checks: ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #2264
  • Loading branch information
SonicScrewdriver authored Feb 24, 2025
1 parent 5de2e74 commit 4eb9fe0
Show file tree
Hide file tree
Showing 10 changed files with 595 additions and 61 deletions.
5 changes: 5 additions & 0 deletions .changeset/slimy-mirrors-sneeze.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": minor
---

Ensure that keyboard users can move points across invalid locations for all graphs.
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,17 @@ import {MafsGraph} from "../mafs-graph";
import * as ReducerGraphConfig from "../reducer/use-graph-config";
import {getBaseMafsGraphPropsForTests} from "../utils";

import {CircleGraph, describeCircleGraph} from "./circle";
import {
CircleGraph,
describeCircleGraph,
getCircleKeyboardConstraint,
} from "./circle";
import * as UseDraggableModule from "./use-draggable";

import type {GraphConfig} from "../reducer/use-graph-config";
import type {InteractiveGraphState} from "../types";
import type {UserEvent} from "@testing-library/user-event";
import type {vec} from "mafs";

const baseMafsGraphProps = getBaseMafsGraphPropsForTests();
const baseCircleState: InteractiveGraphState = {
Expand Down Expand Up @@ -363,3 +368,24 @@ describe("describeCircleGraph", () => {
);
});
});

describe("getCircleKeyboardConstraint", () => {
it("should snap to the snapStep and avoid putting points on a vertical line", () => {
const radiusPoint: vec.Vector2 = [0, 0];
const center: vec.Vector2 = [1, 0];
const snapStep: vec.Vector2 = [1, 1];

const constraint = getCircleKeyboardConstraint(
center,
radiusPoint,
snapStep,
);

expect(constraint).toEqual({
up: [0, 1],
down: [0, -1],
left: [-1, 0],
right: [2, 0], // Avoids overlapping the points
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type CircleGraphProps = MafsGraphProps<CircleGraphState>;
// Exported for testing
export function CircleGraph(props: CircleGraphProps) {
const {dispatch, graphState} = props;
const {center, radiusPoint} = graphState;
const {center, radiusPoint, snapStep} = graphState;

const {strings, locale} = usePerseusI18n();
const [radiusPointAriaLive, setRadiusPointAriaLive] =
Expand Down Expand Up @@ -102,6 +102,11 @@ export function CircleGraph(props: CircleGraphProps) {
setRadiusPointAriaLive("polite");
dispatch(actions.circle.moveRadiusPoint(newRadiusPoint));
}}
constrain={getCircleKeyboardConstraint(
center,
radiusPoint,
snapStep,
)}
/>
{/* Hidden elements to provide the descriptions for the
circle and radius point's `aria-describedby` properties. */}
Expand Down Expand Up @@ -233,6 +238,50 @@ function getCircleGraphDescription(
return strings.srCircleInteractiveElement;
}

export const getCircleKeyboardConstraint = (
center: vec.Vector2,
radiusPoint: vec.Vector2,
snapStep: vec.Vector2,
): {
up: vec.Vector2;
down: vec.Vector2;
left: vec.Vector2;
right: vec.Vector2;
} => {
// Create a helper function that moves the point and then checks
// if it overlaps with the center point after the move.
const movePointWithConstraint = (
moveFunc: (coord: vec.Vector2) => vec.Vector2,
): vec.Vector2 => {
// Move the point
let movedCoord = moveFunc(radiusPoint);
// If the moved point overlaps with the center point,
// move the point again.
if (vec.dist(movedCoord, center) === 0) {
movedCoord = moveFunc(movedCoord);
}
return movedCoord;
};

// Check if the new point overlaps the center point.
// If it does, we need to snap the point to the left
// or right an additional snapStep to avoid the overlap.
return {
up: movePointWithConstraint((coord) =>
vec.add(coord, [0, snapStep[1]]),
),
down: movePointWithConstraint((coord) =>
vec.sub(coord, [0, snapStep[1]]),
),
left: movePointWithConstraint((coord) =>
vec.sub(coord, [snapStep[0], 0]),
),
right: movePointWithConstraint((coord) =>
vec.add(coord, [snapStep[0], 0]),
),
};
};

// Exported for testing
export function describeCircleGraph(
state: CircleGraphState,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import React from "react";

import * as UseDraggableModule from "../use-draggable";

import {MovableLine, trimRange} from "./movable-line";
import {
getMovableLineKeyboardConstraint,
MovableLine,
trimRange,
} from "./movable-line";

import type {Interval, vec} from "mafs";

Expand Down Expand Up @@ -170,3 +174,22 @@ describe("Rendering", () => {
expect(line?.classList).toContain("movable-dragging");
});
});
describe("getMovableLineKeyboardConstraint", () => {
it("should snap to the snapStep and avoid putting points on a vertical line", () => {
const line: [vec.Vector2, vec.Vector2] = [
[0, 0],
[1, 0],
];
const snapStep: vec.Vector2 = [1, 1];

// We're moving the first point
const constraint = getMovableLineKeyboardConstraint(line, snapStep, 0);

expect(constraint).toEqual({
up: [0, 1],
down: [0, -1],
left: [-1, 0],
right: [2, 0], // Avoids overlapping the points
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ export const MovableLine = (props: Props) => {
onMovePoint = () => {},
} = props;

const {snapStep} = useGraphConfig();

// 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
Expand Down Expand Up @@ -79,6 +81,11 @@ export const MovableLine = (props: Props) => {
setAriaLives(["polite", "off", "off"]);
onMovePoint(0, p);
},
constrain: getMovableLineKeyboardConstraint(
[start, end],
snapStep,
0,
),
});
const {visiblePoint: visiblePoint2, focusableHandle: focusableHandle2} =
useControlPoint({
Expand All @@ -92,6 +99,11 @@ export const MovableLine = (props: Props) => {
setAriaLives(["off", "polite", "off"]);
onMovePoint(1, p);
},
constrain: getMovableLineKeyboardConstraint(
[start, end],
snapStep,
1,
),
});

const line = (
Expand Down Expand Up @@ -240,6 +252,54 @@ const Line = (props: LineProps) => {
);
};

export const getMovableLineKeyboardConstraint = (
line: [vec.Vector2, vec.Vector2],
snapStep: vec.Vector2,
pointIndex: number,
): {
up: vec.Vector2;
down: vec.Vector2;
left: vec.Vector2;
right: vec.Vector2;
} => {
// Separate the two points into their own variables, and determine which point is being moved
const coordToBeMoved = line[pointIndex];
const otherPoint = line[1 - pointIndex];

// Create a helper function that moves the point and then checks
// if it overlaps with the other point in the line after the move.
const movePointWithConstraint = (
moveFunc: (coord: vec.Vector2) => vec.Vector2,
): vec.Vector2 => {
// Move the point
let movedCoord = moveFunc(coordToBeMoved);
// If the moved point overlaps with the other point in the line,
// move the point again.
if (vec.dist(movedCoord, otherPoint) === 0) {
movedCoord = moveFunc(movedCoord);
}
return movedCoord;
};

// Check if the new point will overlap the other point.
// If it will, we need to snap the point to the left or right an additional
// snapStep to avoid overlap.
return {
up: movePointWithConstraint((coord) =>
vec.add(coord, [0, snapStep[1]]),
),
down: movePointWithConstraint((coord) =>
vec.sub(coord, [0, snapStep[1]]),
),
left: movePointWithConstraint((coord) =>
vec.sub(coord, [snapStep[0], 0]),
),
right: movePointWithConstraint((coord) =>
vec.add(coord, [snapStep[0], 0]),
),
};
};

export function trimRange(
range: [Interval, Interval],
graphDimensionsInPixels: vec.Vector2,
Expand Down
Loading

0 comments on commit 4eb9fe0

Please sign in to comment.