feat(LEMS-2525): move position of visible label in aria label #1739

anakaren-rojas · 2024-10-10T18:02:03Z

Summary:

Moves the position of the visible label
New aria label structure will be:
Element Type Visible Label(s) Math Details Visual Traits

Issue: LEMS-2525

Test plan:

Navigate to storybook
Select "Interactive Graph" from the Add widget dropdown
Scroll down to the Add locked figure dropdown - select any from the dropdown
Click 'auto generate' button to see basic auto generated aria label
Click 'add visible label' button to expand section and add visible label
Regenerate aria label by clicking 'auto generate' button to see updated label text

Screenshots

Screen.Recording.2024-10-10.at.1.47.25.PM.mov

khan-actions-bot · 2024-10-10T18:02:18Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/twelve-chairs-listen.md, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.test.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.test.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.tsx, 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, 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/locked-polygon-settings.test.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-polygon-settings.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-vector-settings.test.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-vector-settings.tsx

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

github-actions · 2024-10-10T18:08:17Z

Size Change: -54 B (-0.01%)

Total Size: 866 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	280 kB	-54 B (-0.02%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.3 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`	78 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`	419 kB
`packages/perseus/dist/es/strings.js`	3.4 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

github-actions · 2024-10-10T18:08:26Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1739

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

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

benchristel

I think we might want to have some kind of punctuation between labels. Overall, though, this looks good to me!

(I realize we haven't talked about how each of us approaches code reviews. I try to follow our LEMS guidelines. None of my comments in this review are blocking; if you've read my comments, made whatever updates you feel are right, and have no further questions, I'm happy for this to land.)

benchristel · 2024-10-11T18:42:39Z

...-editor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.test.tsx

@@ -495,8 +495,7 @@ describe("LockedEllipseSettings", () => {

            // Assert
            expect(onChangeProps).toHaveBeenCalledWith({
-                ariaLabel:
-                    "Circle with radius 2, centered at (0, 0), with label A",
+                ariaLabel: "Circle A with radius 2, centered at (0, 0)",


Where did we land on spelling out punctuation with words (slack discussion)? I think the coordinates should either be written

centered at left parenthesis 0 comma 0 right parenthesis

or

centered at 0 comma 0

EDIT: okay, I see that that change is out of scope for this PR (which is just about moving the visible labels). Cool cool.

...rseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.tsx

benchristel · 2024-10-11T19:07:57Z

...editor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.test.tsx

@@ -819,7 +819,7 @@ describe("Locked Function Settings", () => {

                // Assert
                expect(onChangeProps).toHaveBeenCalledWith({
-                    ariaLabel: "Function with equation y=x^2, with labels A, B",
+                    ariaLabel: "Function A B with equation y=x^2",


Random thought: I wonder how screenreaders will handle stuff like x^2.

We could use the Speech Rule Engine that comes with MathJax to convert this to words (x squared). I think it would be okay to add a dependency on MathJax to the perseus-editor package for that purpose. The speech rule engine is pretty big, but since this code is not loaded on the learner-facing site, bundle size is less of a concern.

You can find an example of the SpeechRuleEngine in use in mathquill-instance.ts:

perseus/packages/math-input/src/components/input/mathquill-instance.ts

Lines 136 to 138 in 5e30fcd

SpeechRuleEngine.setup(locale).then((SRE) =>

mathField.setMathspeakOverride(SRE.texToSpeech),

);

It's kind of annoying that setting up the SpeechRuleEngine is async, but I think that once we get over that hurdle, we can call:

SRE.texToSpeech("y=x^2")

and it will generate the text for us.

We can set the SRE's locale to "en", because these strings will be translated manually along with the rest of the content.

Unfortunately, there will be a few additional complications with this approach. I believe our current syntax for locked function equations uses e.g. sin(x) and cos(x) for trig functions, and those won't render correctly when interpreted as TeX. TeX systems will interpret them as e.g. s i n x (four separate variables).

For some functions, the fix will be quick: the correct syntax is \sin(x), \cos(x), \tan(x), etc. I'm not sure what all the available functions are for locked functions, though. E.g. do we have atan / arctan? How about sec and csc? ln? We'd want to make sure all the possible functions render correctly when converted to TeX.

Sorry for the wall of text, but I just realized that this is significant additional scope and wanted to write all my thoughts down before I forget them. No action needed for this PR, but I think it's probably worth having a conversation with Caitlyn and content creators about what they need/expect.

...seus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.tsx

@jeremywiebe

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/perseus@38.0.0 ### Major Changes - [#1717](#1717) [`8a40e99e6`](8a40e99) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Remove duplicate WidgetDict type and bespoke typings of `widgets` in favour of `PerseusWidgetsMap` type. ### Patch Changes - [#1752](#1752) [`c4d96ccaf`](c4d96cc) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - BUGFIX - [Number Line] - Tick labels not showing in some exercises - [#1730](#1730) [`2cc20b32e`](2cc20b3) Thanks [@Myranae](https://github.com/Myranae)! - Refine Grapher types and clean up relevant code ## @khanacademy/perseus-editor@14.8.0 ### Minor Changes - [#1739](#1739) [`ab0a10eec`](ab0a10e) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - updates position of visible label within aria label ### Patch Changes - [#1717](#1717) [`8a40e99e6`](8a40e99) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Remove duplicate WidgetDict type and bespoke typings of `widgets` in favour of `PerseusWidgetsMap` type. - Updated dependencies \[[`8a40e99e6`](8a40e99), [`c4d96ccaf`](c4d96cc), [`2cc20b32e`](2cc20b3)]: - @khanacademy/perseus@38.0.0 Author: khan-actions-bot Reviewers: mark-fitzgerald Required Reviewers: Approved By: mark-fitzgerald Checks: ⏭️ Publish npm snapshot, ✅ 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), ✅ Cypress (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1750

move position of visible label in aria label

c5cd558

anakaren-rojas self-assigned this Oct 10, 2024

khan-actions-bot requested a review from a team October 10, 2024 18:02

anakaren-rojas changed the title ~~move position of visible label in aria label~~ feat(LEMS-2525): move position of visible label in aria label Oct 10, 2024

changeset

85064e1

benchristel approved these changes Oct 11, 2024

View reviewed changes

add commas to separate labels

2ea6f20

anakaren-rojas merged commit ab0a10e into main Oct 15, 2024
9 checks passed

anakaren-rojas deleted the LEMS-2525-move-point-label branch October 15, 2024 20:55

khan-actions-bot mentioned this pull request Oct 15, 2024

Version Packages #1750

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(LEMS-2525): move position of visible label in aria label #1739

feat(LEMS-2525): move position of visible label in aria label #1739

anakaren-rojas commented Oct 10, 2024 •

edited

Loading

khan-actions-bot commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

benchristel left a comment

benchristel Oct 11, 2024

benchristel Oct 11, 2024

	SpeechRuleEngine.setup(locale).then((SRE) =>
	mathField.setMathspeakOverride(SRE.texToSpeech),
	);

feat(LEMS-2525): move position of visible label in aria label #1739

feat(LEMS-2525): move position of visible label in aria label #1739

Conversation

anakaren-rojas commented Oct 10, 2024 • edited Loading

Summary:

Test plan:

Screenshots

khan-actions-bot commented Oct 10, 2024 • edited Loading

Gerald

github-actions bot commented Oct 10, 2024 • edited Loading

github-actions bot commented Oct 10, 2024 • edited Loading

npm Snapshot: Published

benchristel left a comment

Choose a reason for hiding this comment

benchristel Oct 11, 2024

Choose a reason for hiding this comment

benchristel Oct 11, 2024

Choose a reason for hiding this comment

anakaren-rojas commented Oct 10, 2024 •

edited

Loading

khan-actions-bot commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading