[Locked Figure Aria] Fix $ edge cases for spoken math aria labels (#1874

) ## Summary: I received a couple suggestions in PR #1854. I'm implmementing them here: - Wrote a helper function that joins labels together so that we don't have to duplicate that code for every single locked figure. - Stopped calling `generateSpokenMathDetails()` on the whole aria string for the locked figure. Instead, it's now being called within the new helper function so that the labels can individually be converted, and we don't have to worry about the edge case with two different labels having dollar signs and causing all the content in between to be counted as TeX when it shouldn't. - Handled the case with a lone unescaped dollar sign, which should not count as TeX. Note: The lone unescaped dollar isn't handled for the TeX inside the graph itself yet. This will be in the next PR. Issue: https://khanacademy.atlassian.net/browse/LEMS-2548 ## Test plan: `yarn jest` Storybook - http://localhost:6006/?path=/story/perseuseditor-widgets-interactive-graph--mafs-with-locked-figure-labels-all-flags - Add two visible labels to the point, with one having "$1 and" as the label and the other having "$1" as the label. - Press the autogenerate button and confirm that the "and" is normal and not "a n d". Also confirm that the dollar signs show up normally. - Add two TeX labels and confirm that the auto-generated text is as expected. - Repeat for all the other locked figures. Author: nishasy Reviewers: benchristel, nishasy, catandthemachines, anakaren-rojas Required Reviewers: Approved By: benchristel, catandthemachines Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1874
Khan · Nov 26, 2024 · 42d25c2 · 42d25c2
1 parent 56fd448
commit 42d25c2
Show file tree

Hide file tree

Showing 15 changed files with 221 additions and 145 deletions.
diff --git a/.changeset/kind-moose-lick.md b/.changeset/kind-moose-lick.md
@@ -0,0 +1,5 @@
+---
+"@khanacademy/perseus-editor": patch
+---
+
+[Locked Figure Aria] Fix $ edge cases for spoken math aria labels
diff --git a/...itor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.test.tsx b/...itor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.test.tsx
@@ -6,7 +6,10 @@ import * as React from "react";
 import {flags} from "../../../__stories__/flags-for-api-options";
 
 import LockedEllipseSettings from "./locked-ellipse-settings";
-import {getDefaultFigureForType} from "./util";
+import {
+    getDefaultFigureForType,
+    mockedJoinLabelsAsSpokenMathForTests,
+} from "./util";
 
 import type {UserEvent} from "@testing-library/user-event";
 
@@ -29,9 +32,8 @@ const defaultLabel = getDefaultFigureForType("label");
 // Mock the async function generateSpokenMathDetails
 jest.mock("./util", () => ({
     ...jest.requireActual("./util"),
-    generateSpokenMathDetails: (input) => {
-        return Promise.resolve(`Spoken math details for ${input}`);
-    },
+    joinLabelsAsSpokenMath: (input) =>
+        mockedJoinLabelsAsSpokenMathForTests(input),
 }));
 
 describe("LockedEllipseSettings", () => {
@@ -391,7 +393,7 @@ describe("LockedEllipseSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Circle with radius 2, centered at (0, 0). Appearance solid gray border, with no fill.",
+                    "Circle with radius 2, centered at (0, 0). Appearance solid gray border, with no fill.",
             });
         });
 
@@ -419,7 +421,7 @@ describe("LockedEllipseSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Circle with radius 2, centered at (0, 0). Appearance solid gray border, with no fill.",
+                    "Circle with radius 2, centered at (0, 0). Appearance solid gray border, with no fill.",
             });
         });
 
@@ -446,7 +448,7 @@ describe("LockedEllipseSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Ellipse with x radius 2 and y radius 3, centered at (0, 0). Appearance solid gray border, with no fill.",
+                    "Ellipse with x radius 2 and y radius 3, centered at (0, 0). Appearance solid gray border, with no fill.",
             });
         });
 
@@ -474,7 +476,7 @@ describe("LockedEllipseSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Ellipse with x radius 2 and y radius 3, centered at (0, 0), rotated by 90 degrees. Appearance solid gray border, with no fill.",
+                    "Ellipse with x radius 2 and y radius 3, centered at (0, 0), rotated by 90 degrees. Appearance solid gray border, with no fill.",
             });
         });
 
@@ -506,7 +508,7 @@ describe("LockedEllipseSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Circle A with radius 2, centered at (0, 0). Appearance solid gray border, with no fill.",
+                    "Circle spoken A with radius 2, centered at (0, 0). Appearance solid gray border, with no fill.",
             });
         });
 
@@ -542,7 +544,7 @@ describe("LockedEllipseSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Circle A, B with radius 2, centered at (0, 0). Appearance solid gray border, with no fill.",
+                    "Circle spoken A, spoken B with radius 2, centered at (0, 0). Appearance solid gray border, with no fill.",
             });
         });
     });

diff --git a/...us-editor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.tsx b/...us-editor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.tsx
@@ -22,8 +22,8 @@ import LockedFigureSettingsActions from "./locked-figure-settings-actions";
 import LockedLabelSettings from "./locked-label-settings";
 import {
     generateLockedFigureAppearanceDescription,
-    generateSpokenMathDetails,
     getDefaultFigureForType,
+    joinLabelsAsSpokenMath,
 } from "./util";
 
 import type {LockedFigureSettingsCommonProps} from "./locked-figure-settings";
@@ -68,22 +68,15 @@ const LockedEllipseSettings = (props: Props) => {
      * with the math details converted into spoken words.
      */
     async function getPrepopulatedAriaLabel() {
-        let visiblelabel = "";
-        if (labels && labels.length > 0) {
-            visiblelabel += ` ${labels.map((l) => l.text).join(", ")}`;
-        }
+        const visiblelabel = await joinLabelsAsSpokenMath(labels);
 
         const isCircle = radius[0] === radius[1];
         let str = "";
 
         if (isCircle) {
-            str += await generateSpokenMathDetails(
-                `Circle${visiblelabel} with radius ${radius[0]}`,
-            );
+            str += `Circle${visiblelabel} with radius ${radius[0]}`;
         } else {
-            str += await generateSpokenMathDetails(
-                `Ellipse${visiblelabel} with x radius ${radius[0]} and y radius ${radius[1]}`,
-            );
+            str += `Ellipse${visiblelabel} with x radius ${radius[0]} and y radius ${radius[1]}`;
         }
 
         str += `, centered at (${center[0]}, ${center[1]})`;

diff --git a/...tor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.test.tsx b/...tor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.test.tsx
@@ -9,7 +9,10 @@ import {flags} from "../../../__stories__/flags-for-api-options";
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 import examples from "./locked-function-examples";
 import LockedFunctionSettings from "./locked-function-settings";
-import {getDefaultFigureForType} from "./util";
+import {
+    getDefaultFigureForType,
+    mockedJoinLabelsAsSpokenMathForTests,
+} from "./util";
 
 import type {Props} from "./locked-function-settings";
 import type {UserEvent} from "@testing-library/user-event";
@@ -27,9 +30,8 @@ const defaultLabel = getDefaultFigureForType("label");
 // Mock the async function generateSpokenMathDetails
 jest.mock("./util", () => ({
     ...jest.requireActual("./util"),
-    generateSpokenMathDetails: (input) => {
-        return Promise.resolve(`Spoken math details for ${input}`);
-    },
+    joinLabelsAsSpokenMath: (input) =>
+        mockedJoinLabelsAsSpokenMathForTests(input),
 }));
 
 const exampleEquationsMock = {
@@ -687,7 +689,7 @@ describe("Locked Function Settings", () => {
                 // Assert
                 expect(onChangeProps).toHaveBeenCalledWith({
                     ariaLabel:
-                        "Spoken math details for Function with equation y=x^2. Appearance solid gray.",
+                        "Function with equation y=x^2. Appearance solid gray.",
                 });
             });
 
@@ -714,7 +716,7 @@ describe("Locked Function Settings", () => {
                 // Assert
                 expect(onChangeProps).toHaveBeenCalledWith({
                     ariaLabel:
-                        "Spoken math details for Function with equation x=y^2. Appearance solid gray.",
+                        "Function with equation x=y^2. Appearance solid gray.",
                 });
             });
 
@@ -740,7 +742,7 @@ describe("Locked Function Settings", () => {
                 // Assert
                 expect(onChangeProps).toHaveBeenCalledWith({
                     ariaLabel:
-                        "Spoken math details for Function with equation y=x^2, domain from 1 to 2. Appearance solid gray.",
+                        "Function with equation y=x^2, domain from 1 to 2. Appearance solid gray.",
                 });
             });
 
@@ -766,7 +768,7 @@ describe("Locked Function Settings", () => {
                 // Assert
                 expect(onChangeProps).toHaveBeenCalledWith({
                     ariaLabel:
-                        "Spoken math details for Function with equation y=x^2. Appearance solid gray.",
+                        "Function with equation y=x^2. Appearance solid gray.",
                 });
             });
 
@@ -797,7 +799,7 @@ describe("Locked Function Settings", () => {
                 // Assert
                 expect(onChangeProps).toHaveBeenCalledWith({
                     ariaLabel:
-                        "Spoken math details for Function A with equation y=x^2. Appearance solid gray.",
+                        "Function spoken A with equation y=x^2. Appearance solid gray.",
                 });
             });
 
@@ -832,7 +834,7 @@ describe("Locked Function Settings", () => {
                 // Assert
                 expect(onChangeProps).toHaveBeenCalledWith({
                     ariaLabel:
-                        "Spoken math details for Function A, B with equation y=x^2. Appearance solid gray.",
+                        "Function spoken A, spoken B with equation y=x^2. Appearance solid gray.",
                 });
             });
         });

diff --git a/...s-editor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.tsx b/...s-editor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.tsx
@@ -30,8 +30,8 @@ import examples from "./locked-function-examples";
 import LockedLabelSettings from "./locked-label-settings";
 import {
     generateLockedFigureAppearanceDescription,
-    generateSpokenMathDetails,
     getDefaultFigureForType,
+    joinLabelsAsSpokenMath,
 } from "./util";
 
 import type {LockedFigureSettingsCommonProps} from "./locked-figure-settings";
@@ -90,14 +90,9 @@ const LockedFunctionSettings = (props: Props) => {
      * with the math details converted into spoken words.
      */
     async function getPrepopulatedAriaLabel() {
-        let visiblelabel = "";
-        if (labels && labels.length > 0) {
-            visiblelabel += ` ${labels.map((l) => l.text).join(", ")}`;
-        }
+        const visiblelabel = await joinLabelsAsSpokenMath(labels);
 
-        let str = await generateSpokenMathDetails(
-            `Function${visiblelabel} with equation ${equationPrefix}${equation}`,
-        );
+        let str = `Function${visiblelabel} with equation ${equationPrefix}${equation}`;
 
         // Add the domain/range constraints to the aria label
         // if they are not the default values.

diff --git a/...-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.test.tsx b/...-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.test.tsx
@@ -6,7 +6,10 @@ import * as React from "react";
 import {flags} from "../../../__stories__/flags-for-api-options";
 
 import LockedLineSettings from "./locked-line-settings";
-import {getDefaultFigureForType} from "./util";
+import {
+    getDefaultFigureForType,
+    mockedJoinLabelsAsSpokenMathForTests,
+} from "./util";
 
 import type {UserEvent} from "@testing-library/user-event";
 
@@ -29,9 +32,8 @@ const defaultLabel = getDefaultFigureForType("label");
 // Mock the async function generateSpokenMathDetails
 jest.mock("./util", () => ({
     ...jest.requireActual("./util"),
-    generateSpokenMathDetails: (input) => {
-        return Promise.resolve(`Spoken math details for ${input}`);
-    },
+    joinLabelsAsSpokenMath: (input) =>
+        mockedJoinLabelsAsSpokenMathForTests(input),
 }));
 
 describe("LockedLineSettings", () => {
@@ -623,7 +625,7 @@ describe("LockedLineSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Segment from point at (0, 0) to point at (2, 2). Appearance solid gray.",
+                    "Segment from point at (0, 0) to point at (2, 2). Appearance solid gray.",
             });
         });
 
@@ -649,7 +651,7 @@ describe("LockedLineSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Line from point at (0, 0) to point at (2, 2). Appearance solid gray.",
+                    "Line from point at (0, 0) to point at (2, 2). Appearance solid gray.",
             });
         });
 
@@ -680,7 +682,7 @@ describe("LockedLineSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Line A from point at (0, 0) to point at (2, 2). Appearance solid gray.",
+                    "Line spoken A from point at (0, 0) to point at (2, 2). Appearance solid gray.",
             });
         });
 
@@ -715,7 +717,7 @@ describe("LockedLineSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Line A, B from point at (0, 0) to point at (2, 2). Appearance solid gray.",
+                    "Line spoken A, spoken B from point at (0, 0) to point at (2, 2). Appearance solid gray.",
             });
         });
 
@@ -756,7 +758,7 @@ describe("LockedLineSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Line A from point C at (0, 0) to point D at (2, 2). Appearance solid gray.",
+                    "Line spoken A from point spoken C at (0, 0) to point spoken D at (2, 2). Appearance solid gray.",
             });
         });
 
@@ -807,7 +809,7 @@ describe("LockedLineSettings", () => {
             // Assert
             expect(onChangeProps).toHaveBeenCalledWith({
                 ariaLabel:
-                    "Spoken math details for Line A, B from point C, C2 at (0, 0) to point D, D2 at (2, 2). Appearance solid gray.",
+                    "Line spoken A, spoken B from point spoken C, spoken C2 at (0, 0) to point spoken D, spoken D2 at (2, 2). Appearance solid gray.",
             });
         });
     });

diff --git a/...rseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx b/...rseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx
@@ -27,8 +27,8 @@ import LockedLabelSettings from "./locked-label-settings";
 import LockedPointSettings from "./locked-point-settings";
 import {
     generateLockedFigureAppearanceDescription,
-    generateSpokenMathDetails,
     getDefaultFigureForType,
+    joinLabelsAsSpokenMath,
 } from "./util";
 
 import type {LockedFigureSettingsCommonProps} from "./locked-figure-settings";
@@ -79,29 +79,12 @@ const LockedLineSettings = (props: Props) => {
      * details converted into spoken words.
      */
     async function getPrepopulatedAriaLabel() {
-        let visiblelabel = "";
-        let point1VisibleLabel = "";
-        let point2VisibleLabel = "";
+        const visiblelabel = await joinLabelsAsSpokenMath(labels);
+        const point1VisibleLabel = await joinLabelsAsSpokenMath(point1.labels);
+        const point2VisibleLabel = await joinLabelsAsSpokenMath(point2.labels);
 
-        if (labels && labels.length > 0) {
-            visiblelabel += ` ${labels.map((l) => l.text).join(", ")}`;
-        }
-
-        if (point1.labels && point1.labels.length > 0) {
-            point1VisibleLabel += ` ${point1.labels
-                .map((l) => l.text)
-                .join(", ")}`;
-        }
+        let str = `${capitalizeKind}${visiblelabel} from point${point1VisibleLabel} at (${point1.coord[0]}, ${point1.coord[1]}) to point${point2VisibleLabel} at (${point2.coord[0]}, ${point2.coord[1]})`;
 
-        if (point2.labels && point2.labels.length > 0) {
-            point2VisibleLabel += ` ${point2.labels
-                .map((l) => l.text)
-                .join(", ")}`;
-        }
-
-        let str = await generateSpokenMathDetails(
-            `${capitalizeKind}${visiblelabel} from point${point1VisibleLabel} at (${point1.coord[0]}, ${point1.coord[1]}) to point${point2VisibleLabel} at (${point2.coord[0]}, ${point2.coord[1]})`,
-        );
         const lineAppearance = generateLockedFigureAppearanceDescription(
             lineColor,
             lineStyle,

diff --git a/...editor/src/widgets/interactive-graph-editor/locked-figures/locked-point-settings.test.tsx b/...editor/src/widgets/interactive-graph-editor/locked-figures/locked-point-settings.test.tsx
@@ -6,7 +6,10 @@ import * as React from "react";
 import {flags} from "../../../__stories__/flags-for-api-options";
 
 import LockedPointSettings from "./locked-point-settings";
-import {getDefaultFigureForType} from "./util";
+import {
+    getDefaultFigureForType,
+    mockedJoinLabelsAsSpokenMathForTests,
+} from "./util";
 
 import type {UserEvent} from "@testing-library/user-event";
 
@@ -29,9 +32,8 @@ const defaultLabel = getDefaultFigureForType("label");
 // Mock the async function generateSpokenMathDetails
 jest.mock("./util", () => ({
     ...jest.requireActual("./util"),
-    generateSpokenMathDetails: (input) => {
-        return Promise.resolve(`Spoken math details for ${input}`);
-    },
+    joinLabelsAsSpokenMath: (input) =>
+        mockedJoinLabelsAsSpokenMathForTests(input),
 }));
 
 describe("LockedPointSettings", () => {
@@ -420,8 +422,7 @@ describe("LockedPointSettings", () => {
         // generateSpokenMathDetails is mocked to return the input string
         // with "Spoken math details for " prepended.
         expect(onChangeProps).toHaveBeenCalledWith({
-            ariaLabel:
-                "Spoken math details for Point at (0, 0). Appearance solid gray.",
+            ariaLabel: "Point at (0, 0). Appearance solid gray.",
         });
     });
 
@@ -453,8 +454,7 @@ describe("LockedPointSettings", () => {
         // generateSpokenMathDetails is mocked to return the input string
         // with "Spoken math details for " prepended.
         expect(onChangeProps).toHaveBeenCalledWith({
-            ariaLabel:
-                "Spoken math details for Point A at (0, 0). Appearance solid gray.",
+            ariaLabel: "Point spoken A at (0, 0). Appearance solid gray.",
         });
     });
 
@@ -491,7 +491,7 @@ describe("LockedPointSettings", () => {
         // with "Spoken math details for " prepended.
         expect(onChangeProps).toHaveBeenCalledWith({
             ariaLabel:
-                "Spoken math details for Point A, B at (0, 0). Appearance solid gray.",
+                "Point spoken A, spoken B at (0, 0). Appearance solid gray.",
         });
     });
 });