Fix line segment tab order #1240

benchristel · 2024-05-02T21:38:38Z

During playtesting, Sarah B pointed out that the tab order for segment graphs
was not very intuitive. Tabbing to a segment first selected the whole segment.
Tabbing further selected each endpoint.

This PR updates the tab order so the first endpoint is selected first, then the
segment, then the second endpoint.

There are some interesting constraints on the implementation of this feature:

SVG elements are always painted in document order: SVG has no equivalent of
z-index. We want the line of a movable segment to render behind the points.
That means the line element must be first.
Unfortunately, that leaves us with very few options for customizing the tab
order. Setting tabindex > 0 is not what we want, since it would bump the
movable points to the very front of the tab order for the entire page.

The solution is as follows:

Add some focusable, invisible <g> elements to stand in for the points,
since we cannot reorder the points relative to the line.
Hook up useMovable to those elements, so focusing them lets you move the
points with arrow keys.
Disable tabbing to the points themselves.
Display a focus ring on a point if you have focused its corresponding <g>.

I split the StyledMovablePoint component into two: there is now a MovablePointView
component that is purely presentational, and has no state or useMovable. This
component has two modes for managing focus. You can either set a tabindex and let
the component itself be focusable, or you can pass a boolean that tells the component
whether it should have a focus ring.

Issue: LEMS-1936

Test plan:

View a Mafs segment graph in the dev ui (yarn dev). Tabbing between UI
elements should feel intuitive.

Screen.Recording.2024-05-06.at.10.44.28.AM.mov

khan-actions-bot · 2024-05-02T21:38:58Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/early-paws-move.md, .changeset/honest-pens-count.md, packages/perseus/src/widgets/__stories__/interactive-graph-regression.stories.tsx, packages/perseus/src/widgets/__testdata__/interactive-graph.testdata.ts, packages/perseus/src/widgets/__tests__/interactive-graph.test.ts, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.test.ts, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.ts, packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-styles.css, packages/perseus/src/widgets/__tests__/__snapshots__/interactive-graph.test.ts.snap, packages/perseus/src/widgets/interactive-graphs/graphs/linear.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/ray.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/segment.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-line.test.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point-view.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/segment.test.ts, packages/perseus/src/widgets/interactive-graphs/graphs/components/segment.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-05-02T21:40:29Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1240

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

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

github-actions · 2024-05-02T21:41:42Z

Size Change: +893 B (0%)

Total Size: 835 kB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	401 kB	+893 B (0%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.1 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	80.5 kB
`packages/math-input/dist/es/strings.js`	1.73 kB
`packages/perseus-core/dist/es/index.js`	908 B
`packages/perseus-editor/dist/es/index.js`	267 kB
`packages/perseus-error/dist/es/index.js`	878 B
`packages/perseus-linter/dist/es/index.js`	21.8 kB
`packages/perseus/dist/es/strings.js`	3.22 kB
`packages/pure-markdown/dist/es/index.js`	3.68 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

benchristel · 2024-05-02T21:43:14Z

packages/perseus/src/widgets/__testdata__/interactive-graph.testdata.ts

@@ -769,7 +769,6 @@ export const polygonQuestionDefaultCorrect: PerseusRenderer = {
                        [2.5, 4],
                        [1.5, 2],
                    ],
-                    match: "congruent",


The removal of this line is kind of a long story.

I changed the RTL tests for interative graph to use tab to select the first movable element

This meant that the test for polygon was selecting the entire polygon instead of the first point

The polygon question was graded based on congruence

That meant that moving the entire polygon, without changing its shape, resulted in a correct answer

That caused the test for answering incorrectly to fail.

To test that incorrect answers are graded correctly, we make the grading more strict. When match is not present, the points must be exactly where the content author put them for the question to be marked correct.

packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point-view.tsx

codecov · 2024-05-03T20:04:34Z

Codecov Report

Attention: Patch coverage is 97.66082% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 70.13%. Comparing base (99f32f5) to head (f39b853).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1240      +/-   ##
==========================================
+ Coverage   68.73%   70.13%   +1.40%     
==========================================
  Files         471      476       +5     
  Lines      101003   101174     +171     
  Branches     7171    10882    +3711     
==========================================
+ Hits        69423    70959    +1536     
+ Misses      31394    30215    -1179     
+ Partials      186        0     -186

Files	Coverage Δ
...widgets/__testdata__/interactive-graph.testdata.ts	`100.00% <ø> (ø)`
...ractive-graphs/graphs/components/movable-point.tsx	`100.00% <100.00%> (+6.83%)`	⬆️
...s/interactive-graphs/graphs/components/segment.tsx	`100.00% <100.00%> (ø)`
...s/src/widgets/interactive-graphs/graphs/linear.tsx	`95.45% <100.00%> (-2.28%)`	⬇️
.../src/widgets/interactive-graphs/graphs/polygon.tsx	`97.39% <100.00%> (+1.59%)`	⬆️
...seus/src/widgets/interactive-graphs/graphs/ray.tsx	`100.00% <100.00%> (ø)`
.../src/widgets/interactive-graphs/graphs/segment.tsx	`100.00% <100.00%> (ø)`
...ctive-graphs/interactive-graph-question-builder.ts	`100.00% <100.00%> (ø)`
...ve-graphs/graphs/components/movable-point-view.tsx	`94.24% <94.24%> (ø)`

... and 145 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99f32f5...f39b853. Read the comment docs.

benchristel · 2024-05-06T16:01:58Z

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

    const ref = React.useRef<SVGPolygonElement>(null);
+    const dragReferencePoint = points[0];


This is a sneaky bugfix.

The bug was that sometimes, selecting an entire polygon and pressing an arrow key would not move the polygon. Our tests for the polygon graph happened to catch it (recall that since we're now using tab to select the first movable element in our tests, we're now selecting the entire polygon in the tests instead of selecting the first point)

The bug happened because the midpoint of the polygon was, in general, not on a snap step. This meant that trying to move the polygon sometimes caused it to snap back to its original location, effectively canceling the move.

Since the points of a polygon are always on a snap step, using the first point as the reference point fixes this.

nishasy · 2024-05-06T17:43:25Z

@benchristel Do you have a video of the new tabbing experience?

benchristel · 2024-05-06T17:45:14Z

@nishasy I've attached a video to the PR description.

nishasy · 2024-05-06T20:29:02Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point-view.tsx

@@ -0,0 +1,130 @@
+import {color as WBColor} from "@khanacademy/wonder-blocks-tokens";


For posterity, could you add a small docblock at the top of this file explaning what MovablePointView is and where/how it's used?

nishasy · 2024-05-06T20:35:26Z

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

+                start: false,
+                end: true,
+            }}
+        />


This is beautiful ✨

nishasy · 2024-05-06T20:40:23Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/segment.tsx

+          };
+};
+
+export const MovableLine = (props: MovableLineProps) => {


I don't think MovableLine needs to be exported anymore?

nishasy · 2024-05-06T20:44:01Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/segment.tsx

+        useControlPoint(end, color, (p) => props.onMovePoint(1, p));
+
+    return (
+        <>


Beautiful! For posterity, could you add a comment here explaining that the focusHandles/visiblePoints are done this way because of the weirdness with SVGs and z-index/tabindex?

…general

@jeresig

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/math-input@19.0.0 ### Major Changes - [#1235](#1235) [`87169b22b`](87169b2) Thanks [@jeresig](https://github.com/jeresig)! - Update mathjax-renderer usage, locale is now required for createMathField. ## @khanacademy/perseus@22.4.1 ### Patch Changes - [#1240](#1240) [`4a59b85ab`](4a59b85) Thanks [@benchristel](https://github.com/benchristel)! - Change the tab order for movable line segments in interactive graphs. Previously, the order was `(whole segment, first point, second point)`. Now it's `(first point, whole segment, second point)`, which aligns better with the visual layout of the graph. - [#1240](#1240) [`4a59b85ab`](4a59b85) Thanks [@benchristel](https://github.com/benchristel)! - Adjust spacing in locked point and locked line settings - [#1235](#1235) [`87169b22b`](87169b2) Thanks [@jeresig](https://github.com/jeresig)! - Update mathjax-renderer usage, locale is now required for createMathField. - Updated dependencies \[[`87169b22b`](87169b2)]: - @khanacademy/math-input@19.0.0 ## @khanacademy/perseus-editor@6.2.2 ### Patch Changes - [#1247](#1247) [`99f32f531`](99f32f5) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Only show a11y banner for ruler and protractor when either or both is enabled - [#1240](#1240) [`4a59b85ab`](4a59b85) Thanks [@benchristel](https://github.com/benchristel)! - Adjust spacing in locked point and locked line settings - [#1235](#1235) [`87169b22b`](87169b2) Thanks [@jeresig](https://github.com/jeresig)! - Update mathjax-renderer usage, locale is now required for createMathField. - Updated dependencies \[[`4a59b85ab`](4a59b85), [`4a59b85ab`](4a59b85), [`87169b22b`](87169b2)]: - @khanacademy/perseus@22.4.1 - @khanacademy/math-input@19.0.0 ## @khanacademy/perseus-dev-ui@1.5.6 ### Patch Changes - Updated dependencies \[[`87169b22b`](87169b2)]: - @khanacademy/math-input@19.0.0

My last PR (#1240) left a rename half-finished. This PR finishes it. Issue: none Test plan: `yarn test`

## Summary: My last PR (#1240) left a rename half-finished. This PR finishes it. Issue: none Test plan: `yarn test` Author: benchristel Reviewers: benchristel, jeremywiebe, mark-fitzgerald, nishasy, SonicScrewdriver, handeyeco, Myranae Required Reviewers: Approved By: jeremywiebe Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1250

benchristel self-assigned this May 2, 2024

benchristel requested review from handeyeco, mark-fitzgerald and nishasy May 2, 2024 21:38

khan-actions-bot requested a review from a team May 2, 2024 21:38

benchristel commented May 2, 2024

View reviewed changes

packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point-view.tsx Outdated Show resolved Hide resolved

benchristel force-pushed the benc/segment-tab-order-2 branch from b71bb8b to faf351e Compare May 3, 2024 22:55

benchristel commented May 6, 2024

View reviewed changes

nishasy approved these changes May 6, 2024

View reviewed changes

benchristel added 14 commits May 8, 2024 10:25

Add tests and stories

4f3b92b

Make StyledMovablePoint a pure presentation component

a1dfd68

Track focus state of points manually

f963bbc

Refactor

7b1877b

Restore StyledMovablePoint for use by the other graph types

4a68399

Use MovablePointView in StyledMovablePoint

320cb47

Fix tests

8ac275d

Fix ref type

1e2d7b2

Fix polygon test

f13b2b8

Update snapshots

9393461

Rename variable

bc21ecc

Fix lint

705d4e5

Avoid reusing potentially-mutable object instances; make repeat more …

0597881

…general

Fix fixmes

aa74bb2

benchristel added 20 commits May 8, 2024 10:28

Rename SegmentView to Segment

72735bc

Fix formatting

3cc862a

Fix lint

01b0601

Split Segment component into its own file

c18e63f

Rename variable

8fd91b6

Use the Segment component in Linear graphs

6a6b0cd

Use Segment component in Ray graphs

3078030

Inline MovableLine into segment.tsx

07c9d6e

Inline LineView

52b197d

Remove range from extend options

f5d0be7

Remove redundant fragment

e9e47c7

Delete unused type

9031d61

Fix lint

b36c0f0

Rename stroke prop to color

940abc5

Add changeset

81d2435

Fix test

a2854db

Add test for InteractiveGraphQuestionBuilder.withSegments()

fc27adf

Fix formatting

b77eab7

Add doc comments

073b9e0

Fix up types after rebasing

b793fee

benchristel force-pushed the benc/segment-tab-order-2 branch from 062dea9 to b793fee Compare May 8, 2024 17:35

benchristel added 2 commits May 8, 2024 10:38

Fix lint

31df02b

Update snapshots

f39b853

benchristel merged commit 4a59b85 into main May 8, 2024
13 checks passed

benchristel deleted the benc/segment-tab-order-2 branch May 8, 2024 18:33

khan-actions-bot mentioned this pull request May 8, 2024

Version Packages #1248

Merged

benchristel added a commit that referenced this pull request May 8, 2024

Rename Segment to MovableLine

90ff495

My last PR (#1240) left a rename half-finished. This PR finishes it. Issue: none Test plan: `yarn test`

benchristel mentioned this pull request May 8, 2024

Rename Segment to MovableLine #1250

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix line segment tab order #1240

Fix line segment tab order #1240

benchristel commented May 2, 2024 •

edited

khan-actions-bot commented May 2, 2024 •

edited

github-actions bot commented May 2, 2024 •

edited

github-actions bot commented May 2, 2024 •

edited

benchristel May 2, 2024

codecov bot commented May 3, 2024 •

edited

benchristel May 6, 2024

nishasy commented May 6, 2024

benchristel commented May 6, 2024

nishasy May 6, 2024

nishasy May 6, 2024

nishasy May 6, 2024

nishasy May 6, 2024

		const ref = React.useRef<SVGPolygonElement>(null);
		const dragReferencePoint = points[0];

		@@ -0,0 +1,130 @@
		import {color as WBColor} from "@khanacademy/wonder-blocks-tokens";

Fix line segment tab order #1240

Fix line segment tab order #1240

Conversation

benchristel commented May 2, 2024 • edited

Test plan:

khan-actions-bot commented May 2, 2024 • edited

Gerald

github-actions bot commented May 2, 2024 • edited

npm Snapshot: Published

github-actions bot commented May 2, 2024 • edited

benchristel May 2, 2024

Choose a reason for hiding this comment

codecov bot commented May 3, 2024 • edited

Codecov Report

benchristel May 6, 2024

Choose a reason for hiding this comment

nishasy commented May 6, 2024

benchristel commented May 6, 2024

nishasy May 6, 2024

Choose a reason for hiding this comment

nishasy May 6, 2024

Choose a reason for hiding this comment

nishasy May 6, 2024

Choose a reason for hiding this comment

nishasy May 6, 2024

Choose a reason for hiding this comment

benchristel commented May 2, 2024 •

edited

khan-actions-bot commented May 2, 2024 •

edited

github-actions bot commented May 2, 2024 •

edited

github-actions bot commented May 2, 2024 •

edited

codecov bot commented May 3, 2024 •

edited