feat(LEMS-2662): handle angle aria label updates #1940

anakaren-rojas · 2024-12-02T19:41:19Z

Summary:

Adds SR functionality for angle sides and vertex
Adds variations of 6 variations of angle strings - initial side copy (x2), updated side copy (x2), initial vertex copy, updated vertex copy

Issue: LEMS-2662

Test plan:

Testing Steps

Navigate to chromatic
Add an interactive graph widget and update the answer type to Angle
With screen reader on - move any of the points on the graph
Enable Show angle measures and move points
Enable Allow reflex angles and move points

Expected behavior

Screen Reader reads point coordinates (always)
When Show angle measures is not enabled, screen reader does not read out angle measure
When Show angle measures is enabled, angle measure is read out with vertex
When Show angle measures and Allow reflex angles are enabled, Screen Reader reads out reflex angles

Screen Recordings:

Without angle measures

Screen.Recording.2024-12-03.at.14.53.13.mov

With angle measures

Screen.Recording.2024-12-03.at.14.53.53.mov

github-actions · 2024-12-03T17:18:47Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (b055724) and published it to npm. You
can install it using the tag PR1940.

Example:

yarn add @khanacademy/perseus@PR1940

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1940

github-actions · 2024-12-03T17:18:58Z

Size Change: +799 B (+0.06%)

Total Size: 1.29 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	426 kB	+512 B (+0.12%)
`packages/perseus/dist/es/strings.js`	3.99 kB	+287 B (+7.76%)	🔍

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	77.9 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	697 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

khan-actions-bot · 2024-12-03T23:09:15Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/famous-rockets-occur.md, packages/perseus/src/strings.ts, packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/angle.tsx, packages/perseus/src/widgets/interactive-graphs/math/angles.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

benchristel

This is exciting!

Requesting changes because we're ultimately going to want a different approach to aria-live notifications (see my inline comment about avoiding useState and building for the upcoming Wonder Blocks API) and I'd prefer to start designing for that now.

packages/perseus/src/strings.ts

benchristel · 2024-12-04T00:02:41Z

packages/perseus/src/widgets/interactive-graphs/graphs/angle.tsx

+        updateVertexAriaLabel(coords[1]);
+        // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [showAngles]);
+


I think we could avoid using the useState and useEffect hooks here if we:

compute aria labels from the graphState prop

use aria-live to notify the user of changes to the graph

This would ensure that the SR view and the visible view never get out of sync, since both would be pure functions of graphState. Currently, all the individual graph components are stateless (or nearly so — there are a couple useStates for hover and dragging effects) and I'd like to keep it that way.

Wonder Blocks is planning an API for sending aria-live notifications, but since it's not built yet, I think what we could do is write a minimal implementation of the same API and swap in the WB one when it's ready. I'd be down to pair or consult on that if you want.

packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.test.ts

benchristel · 2024-12-04T00:22:25Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.tsx

+export function getClockwiseCoords(
+    coords: Vector2[],
+    vertex: vec.Vector2,
+    allowReflexAngles: boolean = false,
+) {
+    // Check if the points are clockwise or not, depending on whether we allow reflex angles
+    const areClockwise = clockwise([...coords, vertex]);
+    const shouldReverseCoords = areClockwise && !allowReflexAngles;
+
+    // Reverse the coordinates accordingly to ensure the angle is calculated correctly
+    return shouldReverseCoords ? coords : coords.reverse();
+}


Thanks for refactoring this code!

It seems like this duplicates some of the logic from getClockwiseAngle in interactive-graphs/math/angles.ts. Could we use that function to calculate the angle measure here? getClockwiseAngle is used for scoring, and it would be nice if we could use the same function to calculate the angle label.

oh yes, you're right, it looks like it combines both the getClockwiseCoords and the getWholeAngleMeasure methods into one! let me test it out

ok, so I tested this out and using the getClockwiseAngle function introduces a regression in the graph 😢

The very small difference between how we determine whether the coordinates should be reversed is the culprit:
In getClockwiseAngle
const shouldReverseCoords = !areClockwise || allowReflexAngles;
In getWholeAngleMeasure
const shouldReverseCoords = areClockwise && !allowReflexAngles;

EDIT:
I realized that using getClockwiseAngle in angles served the original purpose that these two new methods were supposed to serve, so there's no longer a need to pull this logic out of angle-indicators; I'm leaving angle-indicators as-is

benchristel · 2024-12-04T00:25:59Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.test.ts

I like these tests!

benchristel · 2024-12-04T00:27:16Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.test.ts

+            [1, 0],
+            [0, 0],
+        ]);
+    });


I'm a bit confused by this. The function is called getClockwiseCoords but it can return counterclockwise coords?

Co-authored-by: Ben Christel <[email protected]>

…LEMS-2622-update-point-copy

anakaren-rojas added 3 commits November 26, 2024 15:19

handle side aria label updates

cffe953

handle angle aria label updates

b31207e

clean up code

7dd98bc

anakaren-rojas self-assigned this Dec 2, 2024

update test

a620826

anakaren-rojas changed the title ~~handle angle aria label updates~~ feat(LEMS-2662): handle angle aria label updates Dec 2, 2024

Merge branch 'main' into LEMS-2622-update-point-copy

59b0d2f

anakaren-rojas added 4 commits December 3, 2024 11:32

add tests for getWholeAngleMeasure and getClockwiseCoords

c79e92a

remove unused import

170a938

fix updateVertexAriaLabel method

e845e00

update test

ec17148

anakaren-rojas marked this pull request as ready for review December 3, 2024 23:09

anakaren-rojas requested a review from benchristel December 3, 2024 23:09

khan-actions-bot requested a review from a team December 3, 2024 23:09

anakaren-rojas requested review from nishasy and catandthemachines and removed request for a team December 3, 2024 23:09

benchristel requested changes Dec 4, 2024

View reviewed changes

anakaren-rojas and others added 6 commits December 4, 2024 13:45

Apply suggestions from code review

d71854b

Co-authored-by: Ben Christel <[email protected]>

rename func and use getClockwiseAngle func

2c9983c

update strings descriptions

c491156

update strings descriptions

3a72287

Merge remote-tracking branch 'origin/LEMS-2622-update-point-copy' into …

96d2474

…LEMS-2622-update-point-copy

remove update of angle-indicator files

b055724

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(LEMS-2662): handle angle aria label updates #1940

feat(LEMS-2662): handle angle aria label updates #1940

anakaren-rojas commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

khan-actions-bot commented Dec 3, 2024 •

edited

Loading

benchristel left a comment

benchristel Dec 4, 2024

benchristel Dec 4, 2024

anakaren-rojas Dec 4, 2024

anakaren-rojas Dec 4, 2024 •

edited

Loading

benchristel Dec 4, 2024

benchristel Dec 4, 2024

feat(LEMS-2662): handle angle aria label updates #1940

Are you sure you want to change the base?

feat(LEMS-2662): handle angle aria label updates #1940

Conversation

anakaren-rojas commented Dec 2, 2024 • edited Loading

Summary:

Test plan:

Testing Steps

Expected behavior

Screen Recordings:

Without angle measures

With angle measures

github-actions bot commented Dec 3, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Dec 3, 2024 • edited Loading

khan-actions-bot commented Dec 3, 2024 • edited Loading

Gerald

benchristel left a comment

Choose a reason for hiding this comment

benchristel Dec 4, 2024

Choose a reason for hiding this comment

benchristel Dec 4, 2024

Choose a reason for hiding this comment

anakaren-rojas Dec 4, 2024

Choose a reason for hiding this comment

anakaren-rojas Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

benchristel Dec 4, 2024

Choose a reason for hiding this comment

benchristel Dec 4, 2024

Choose a reason for hiding this comment

anakaren-rojas commented Dec 2, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

github-actions bot commented Dec 3, 2024 •

edited

Loading

khan-actions-bot commented Dec 3, 2024 •

edited

Loading

anakaren-rojas Dec 4, 2024 •

edited

Loading