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

[Locked Figure Labels] Util function to generate spoken math + use it within Locked Point aria labels #1839

Merged
merged 13 commits into from
Nov 14, 2024

Conversation

nishasy
Copy link
Contributor

@nishasy nishasy commented Nov 8, 2024

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.

  • Create a utility function using the MathJax speech engine that converts
    math details (TeX denoted by the $...$ blocks) to spoken words.
  • Use this utility within LockedPointSettings. (Async + useEffect because
    the speech engine is async. I had to use mocks in the tests for this.)

If this approach is given the okay, I'll go ahead and apply it to the
other locked figures as well in a following PR.

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

Test plan:

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

Storybook

  • http://localhost:6006/iframe.html?args=&id=perseuseditor-widgets-interactive-graph--mafs-with-locked-figure-labels-all-flags&viewMode=story
  • Open the locked point settings
  • Change the visible label to have 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 and with multiple labels
image

Loading state demo

Screen.Recording.2024-11-14.at.11.23.58.AM.mov

Copy link
Contributor

github-actions bot commented Nov 8, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1839

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

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

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Size Change: +414 kB (+47.74%) 🚨

Total Size: 1.28 MB

Filename Size Change
packages/perseus-editor/dist/es/index.js 697 kB +414 kB (+146.18%) 🆘
ℹ️ 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 417 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

@nishasy nishasy marked this pull request as ready for review November 8, 2024 01:55
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Nov 8, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/rare-lamps-cough.md, packages/perseus-editor/package.json, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-figure-aria.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-point-settings.test.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-point-settings.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/util.test.ts, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/util.ts

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

Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

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

Very cool! I left some suggestions inline for making the TeX parsing more robust.

// of the math inside the $ signs.
// Example: "Circle with radius $\frac{1}{2}$" -> "Circle with radius one half"
const convertedSpeech = mathString.replace(
/\$(.*?)\$/g,
Copy link
Member

@benchristel benchristel Nov 8, 2024

Choose a reason for hiding this comment

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

This way of identifying TeX isn't going to work in all cases, because the TeX itself can contain dollar signs (either escaped, or in subexpressions).

For example: This sandwich costs $\$50$ and $\text{$t$ seconds}$

I might recommend using @khanacademy/simple-markdown (see perseus-markdown.tsx) to parse the string. You'll probably want to configure a separate parser instance in order to get TeX to render as speech. You could also use the parsing algorithm from mathMatcher(), defined in pure-markdown/src/index.ts

@khan-actions-bot khan-actions-bot requested a review from a team November 12, 2024 23:43
@nishasy nishasy marked this pull request as draft November 12, 2024 23:45
@nishasy nishasy marked this pull request as ready for review November 13, 2024 21:14
jest.mock("./util", () => ({
...jest.requireActual("./util"),
generateSpokenMathDetails: (input) => {
return `Spoken math details for ${input}`;
Copy link
Member

Choose a reason for hiding this comment

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

Since the actual generateSpokenMathDetails function is async, do we want to return a Promise here to make sure the result is correctly being awaited? (Or maybe we trust TypeScript to tell us if we get that wrong?)

Suggested change
return `Spoken math details for ${input}`;
return Promise.resolve(`Spoken math details for ${input}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great point! I'll change it to the promise.

@@ -112,6 +112,10 @@ type ParsedNode = {
function condenseTextNodes(nodes: Array<ParsedNode>): Array<ParsedNode> {
const result: ParsedNode[] = [];

if (!nodes) {
Copy link
Member

Choose a reason for hiding this comment

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

According to the types, this condition should never be met, because nodes is an array, and all arrays are truthy. I think we need to change the nodes parameter type to something like ParsedNode[] | undefined to match reality.

Comment on lines 119 to 120
* If the point has labels, the aria label will be
* "Point at (x, y) with label1, label2, label3".
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment should be something like

Suggested change
* If the point has labels, the aria label will be
* "Point at (x, y) with label1, label2, label3".
* If the point has labels, the aria label will be
* "Point label1, label2, label3 at (x, y)".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

return str;
}

getPrepopulatedAriaLabel().then(setPrepopulatedAriaLabel);
Copy link
Member

Choose a reason for hiding this comment

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

There's a race condition here because the last getPrepopulatedAriaLabel promise to resolve might not be one from the most recent render. This could result in an aria label being generated with the wrong coords, labels, or pointColor.

I think we could guard against that race condition by doing this in the useEffect callback:

let canceled = false;
getPrepopulatedAriaLabel().then((label) => {
  !canceled && setPrepopulatedAriaLabel(label);
})

return () => canceled = true;

The annoying thing is that this is really hard to test.

What we really want here is something like a useMemo, but unfortunately useMemo doesn't work when the function we want to memoize is async. I don't think it would be too hard to write a useAsyncMemo hook ourselves, though.

Then we could just write this:

const prepopulatedAriaLabel = useAsyncMemo(
  /* initial value */ "",
  () => getPrepopulatedAriaLabel(coord, labels, pointColor),
  [coord, labels, pointColor],
);

@khan-actions-bot khan-actions-bot requested a review from a team November 13, 2024 22:08
@@ -13,15 +13,23 @@ const {InfoTip} = components;

type Props = {
ariaLabel: string | undefined;
prePopulatedAriaLabel: string;
prePopulatedAriaLabel?: string;
getPrepopulatedAriaLabel?: () => Promise<string>;
onChangeProps: (props: {ariaLabel?: string | undefined}) => void;
Copy link
Contributor

@anakaren-rojas anakaren-rojas Nov 14, 2024

Choose a reason for hiding this comment

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

Can onChangeProps be moved up since prePopulatedAriaLabel and getPrepopulatedAriaLabel are optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we have a general style of putting function type props at the end of the props list in all the Khan repos.

Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

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

LGTM!

@nishasy nishasy merged commit 1508888 into main Nov 14, 2024
9 checks passed
SonicScrewdriver added a commit that referenced this pull request Nov 14, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @khanacademy/[email protected]

### Minor Changes

- [#1731](#1731)
[`27126aa00`](27126aa)
Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! -
Conversion of Input Number to Numeric Input

### Patch Changes

- [#1846](#1846)
[`8eb1ff5d1`](8eb1ff5)
Thanks [@benchristel](https://github.com/benchristel)! - Internal: add
widget parsers for ADR 773.


- [#1839](#1839)
[`150888870`](1508888)
Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels]
Util function to generate spoken math + use it within Locked Point aria
labels

## @khanacademy/[email protected]

### Minor Changes

- [#1859](#1859)
[`dcf1fbe35`](dcf1fbe)
Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! -
Addition of a new alert for the content editors when Input numbers are
converted to Numeric Inputs


- [#1731](#1731)
[`27126aa00`](27126aa)
Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! -
Conversion of Input Number to Numeric Input

### Patch Changes

- [#1839](#1839)
[`150888870`](1508888)
Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels]
Util function to generate spoken math + use it within Locked Point aria
labels

- Updated dependencies
\[[`8eb1ff5d1`](8eb1ff5),
[`150888870`](1508888),
[`27126aa00`](27126aa)]:
    -   @khanacademy/[email protected]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants