[Locked Figure Aria] Use spoken math in Locked Line settings autogen labels #1854

nishasy · 2024-11-14T01:21:41Z

Summary:

When auto-generating the aria labels for locked figures, we want it to use
words as if they were spoken rather than math expressions that might be
read incorrectly by the screen reader.

Use the generateSpokenMathDetails utility within LockedLineSettings,
following the pattern used in LockedPointSettings for this.
Update the locked line autogen function to include the labels
on the locked points that define the locked line.

Issue: https://khanacademy.atlassian.net/browse/LEMS-2548

Test plan:

yarn jest packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.test.tsx

Storybook

Go to http://localhost:6006/iframe.html?args=&id=perseuseditor-widgets-interactive-graph--mafs-with-locked-figure-labels-all-flags&viewMode=story
Open the locked line settings
Change the visible label to hvae a mix of TeX (with $...$ ) and non-TeX
Press the "Auto-generate" button
Confirm that the input changes to include spoken math words for the TeX
Also try this with no labels, multiple labels, and the labels changed
for the locked points that define the line

… within Locked Point aria labels

… label, update type on condenseTextNodes params

…labels When auto-generating the aria labels for locked figures, we want it to use words as if they were spoken rather than math expressions that might be read incorrectly by the screen reader. - Use the `generateSpokenMathDetails` utility within LockedLineSettings, following the pattern used in LockedPointSettings for this. - Update the locked line autogen function to include the labels on the locked points that define the locked line. Issue: https://khanacademy.atlassian.net/browse/LEMS-2548 Test plan: - `yarn jest packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.test.tsx` Storybook - Go to http://localhost:6006/iframe.html?args=&id=perseuseditor-widgets-interactive-graph--mafs-with-locked-figure-labels-all-flags&viewMode=story - Open the locked line settings - Change the visible label to hvae a mix of TeX (with `$...$`) and non-TeX - Press the "Auto-generate" button - Confirm that the input changes to include spoken math words for the TeX - Also try this with no labels, multiple labels, and the labels changed for the locked points that define the line

github-actions · 2024-11-14T01:24:40Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1854

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

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

github-actions · 2024-11-14T01:25:01Z

Size Change: +72 B (+0.01%)

Total Size: 1.29 MB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	699 kB	+72 B (+0.01%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.9 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.8 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/index.js`	420 kB
`packages/perseus/dist/es/strings.js`	3.54 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

khan-actions-bot · 2024-11-14T01:33:26Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/weak-glasses-allow.md, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.test.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx

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

…is clicked rather when the props are changed

…is clicked rather than when the props are changed

benchristel

I think there might be a bug when the labels contain incomplete TeX expressions (i.e. an odd number of unescaped dollar signs). I left some comments/questions inline.

benchristel · 2024-11-14T21:09:15Z

.../perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx

        let visiblelabel = "";
+        let point1VisibleLabel = "";
+        let point2VisibleLabel = "";


Why are these variables called *VisibleLabel if this function is generating an aria label?

...oh, I think I get it. The term "label" is overloaded. We're using "visible label" to contrast with "aria label". If we just called the point labels "labels" that might be confusing.

Yeah, it's expressing the the locked (visible) label that's associated with this figure is being added to the aria label.

benchristel · 2024-11-14T21:22:42Z

.../perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx

+
+        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]})`,
+        );


The duplication of the labels.map((l) => l.text).join(", ") logic makes me think there's a missing abstraction here. What exactly that abstraction should be probably depends on how labels are being constructed for other locked figure types. It would be nice if we could represent this logic in one place.

I'm imagining something like:

function describeLockedPoint(point: LockedPointType): string { const {coord, labels} = point; const visibleLabel = labels && labels.length > 0 ? ` ${labels.map((l) => l.text).join(", ")}` : ""; return `point${visibleLabel} at (${coord[0]}, ${coord[1]})` }

but I'm not sure this would be suitable for all locked figures.

Alternatively, maybe we could have a function like joinLockedFigureLabels that takes in an array of LockedLabels and then gets used within here. I thought about abstracting away the whole description, but the description does change a bit for every figure, which is why I didn't end up going that way.

I would definitely be for creating a function for this. Especially since we could potentially re-use it in future implementations. Plus we can make calls to joinLockedFigureLabels and add a comment describing the string it's generating. 😊

benchristel · 2024-11-14T21:46:31Z

.../perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx

+
+        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]})`,
+        );


I think there might be a bug here when the labels include dollar signs.

Because we're calling generateSpokenMathDetails on the concatenated string, an unescaped dollar sign in one label might pair with an unescaped dollar sign in another label to create a TeX expression where there shouldn't be one.

For example, if one point had the label $1 and another point had the label $2, the string passed to generateSpokenMathDetails would be:

Line from point $1 at (0, 0) to point $2 at (1, 1)

And that would get interpreted as containing a TeX expression 1 at (0, 0) to point which is not going to speechify nicely (because every letter will be interpreted as a separate variable).

We could fix this by calling generateSpokenMathDetails for each label individually. For the example above, that would mean we'd get TeX syntax errors for both labels due to the unescaped dollar signs.

That makes me wonder: how are we handling TeX syntax errors?

A few points:

I don't think you can use unescaped dollar signs as labels, so I'm not sure how to recreate the case you're referring to.

I can still call generateSpokenMathDetails on each label individually. The reason I didn't do this to begin with is that if it's a math block, it doesn't include the space before, but if it's regular text, it is. So I'd have to add an extra bit of logic to handle this case.

TeX syntax errors show up really ugly in the preview with big red text. We don't handle them specifically.

Interesting! Can we file a bug to at some point add an error message into the form for Visible labels? We don't need to inflate the scope of your work but I think this is a small change that would make this error more clear to content creators! ❤️

catandthemachines

Looking pretty good, would love to see an abstracted function for the label joining and a bug for the syntax errors <3

catandthemachines · 2024-11-15T17:43:35Z

.../perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx

+
+        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]})`,
+        );


I would definitely be for creating a function for this. Especially since we could potentially re-use it in future implementations. Plus we can make calls to joinLockedFigureLabels and add a comment describing the string it's generating. 😊

catandthemachines · 2024-11-15T17:47:19Z

.../perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx

+
+        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]})`,
+        );


Interesting! Can we file a bug to at some point add an error message into the form for Visible labels? We don't need to inflate the scope of your work but I think this is a small change that would make this error more clear to content creators! ❤️

nishasy added 10 commits November 7, 2024 17:42

[Locked Figure Labels] Util function to generate spoken math + use it…

096a79d

… within Locked Point aria labels

Use SimpleMarkdown parser to make sure dollar signs are parsed correctly

a3bef8a

Add pure markdown as a dependency

144b937

Add TODO comment

d00f96d

Merge branch 'main' into spoken-aria-labels

4fc07d8

Add more tests. Fix null nodes erroring out.

d2139d6

changeset

007dbfa

PR feedback: mock promise in test, update comment about autogenerated…

8872044

… label, update type on condenseTextNodes params

handle race condition in locked point settings race condition

7916eca

nishasy self-assigned this Nov 14, 2024

nishasy requested review from benchristel, catandthemachines, anakaren-rojas and a team November 14, 2024 01:22

nishasy marked this pull request as ready for review November 14, 2024 01:33

nishasy added 8 commits November 14, 2024 11:07

Only run the getPrepopulatedAriaLabel async function when the button …

44cc790

…is clicked rather when the props are changed

Merge branch 'spoken-aria-labels' into spoken-aria-lines

9302276

Only run the getPrepopulatedAriaLabel async funciton when the button …

e593568

…is clicked rather than when the props are changed

Add loading state (for cases when the whole HTTP fetch needs to happen)

2662370

Merge branch 'spoken-aria-labels' into spoken-aria-lines

77f17be

Merge branch 'main' into spoken-aria-labels

fc8c21d

Merge branch 'main' into spoken-aria-labels

af82ae2

Merge branch 'spoken-aria-labels' into spoken-aria-lines

508cabe

benchristel reviewed Nov 14, 2024

View reviewed changes

nishasy changed the base branch from spoken-aria-labels to main November 14, 2024 22:30

An error occurred while trying to automatically change base from spoken-aria-labels to main November 14, 2024 22:30

Merge branch 'main' into spoken-aria-lines

5aa9a90

khan-actions-bot requested a review from a team November 14, 2024 22:30

catandthemachines requested changes Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Locked Figure Aria] Use spoken math in Locked Line settings autogen labels #1854

[Locked Figure Aria] Use spoken math in Locked Line settings autogen labels #1854

nishasy commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

khan-actions-bot commented Nov 14, 2024 •

edited

Loading

benchristel left a comment

benchristel Nov 14, 2024

nishasy Nov 14, 2024

benchristel Nov 14, 2024

nishasy Nov 14, 2024

catandthemachines Nov 15, 2024

benchristel Nov 14, 2024

nishasy Nov 14, 2024

catandthemachines Nov 15, 2024

catandthemachines left a comment

catandthemachines Nov 15, 2024

catandthemachines Nov 15, 2024

[Locked Figure Aria] Use spoken math in Locked Line settings autogen labels #1854

Are you sure you want to change the base?

[Locked Figure Aria] Use spoken math in Locked Line settings autogen labels #1854

Conversation

nishasy commented Nov 14, 2024 • edited Loading

Summary:

Test plan:

github-actions bot commented Nov 14, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Nov 14, 2024 • edited Loading

khan-actions-bot commented Nov 14, 2024 • edited Loading

Gerald

benchristel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catandthemachines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishasy commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

khan-actions-bot commented Nov 14, 2024 •

edited

Loading