Skip to content

Commit

Permalink
Fix line segment tab order (#1240)
Browse files Browse the repository at this point in the history 
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.

https://github.com/Khan/perseus/assets/693920/ee92b81b-5f01-44d3-942a-b9d80c74ee9e

Author: benchristel

Reviewers: benchristel, nishasy, #perseus, handeyeco, mark-fitzgerald

Required Reviewers:

Approved By: nishasy

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1240
  • Loading branch information
@benchristel
benchristel committed May 8, 2024
1 parent 99f32f5 commit 4a59b85
Show file tree
Hide file tree
Showing 20 changed files with 613 additions and 317 deletions.
5 changes: 5 additions & 0 deletions .changeset/early-paws-move.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

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.
6 changes: 6 additions & 0 deletions .changeset/honest-pens-count.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@khanacademy/perseus": patch
"@khanacademy/perseus-editor": patch
---

Adjust spacing in locked point and locked line settings
8 changes: 8 additions & 0 deletions packages/perseus/src/widgets/__stories__/interactive-graph-regression.stories.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,14 @@ export const MafsInMobileContainer = (args: StoryArgs): React.ReactElement => (
</div>
);

export const MafsWithMultipleSegments = (
args: StoryArgs,
): React.ReactElement => (
<MafsQuestionRenderer
question={interactiveGraphQuestionBuilder().withSegments(3).build()}
/>
);

function MafsQuestionRenderer(props: {question: PerseusRenderer}) {
const {question} = props;
return (
Expand Down
1 change: 0 additions & 1 deletion packages/perseus/src/widgets/__testdata__/interactive-graph.testdata.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,6 @@ export const polygonQuestionDefaultCorrect: PerseusRenderer = {
[2.5, 4],
[1.5, 2],
],
match: "congruent",
snapTo: "grid",
},
},
Expand Down
58 changes: 38 additions & 20 deletions packages/perseus/src/widgets/__tests__/__snapshots__/interactive-graph.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3039,6 +3039,10 @@ exports[`snapshots should render correctly 1`] = `
/>
</filter>
</defs>
<g
data-testid="movable-point__focusable-handle"
tabindex="0"
/>
<g
class="movable-line"
data-testid="movable-line"
Expand Down Expand Up @@ -3077,10 +3081,13 @@ exports[`snapshots should render correctly 1`] = `
/>
</g>
<g
class="movable-point "
data-testid="movable-point__focusable-handle"
tabindex="0"
/>
<g
class="movable-point "
data-testid="movable-point"
style="--movable-point-color: #1865f2;"
tabindex="0"
>
<circle
class="movable-point-hitbox"
Expand Down Expand Up @@ -3112,10 +3119,9 @@ exports[`snapshots should render correctly 1`] = `
/>
</g>
<g
class="movable-point "
class="movable-point "
data-testid="movable-point"
style="--movable-point-color: #1865f2;"
tabindex="0"
>
<circle
class="movable-point-hitbox"
Expand Down Expand Up @@ -3302,15 +3308,15 @@ exports[`snapshots should render correctly with locked lines 1`] = `
>
<pattern
height="20"
id="cartesian-31"
id="cartesian-30"
patternUnits="userSpaceOnUse"
width="20"
x="0"
y="0"
>
<pattern
height="20"
id="cartesian-31-subdivision"
id="cartesian-30-subdivision"
patternUnits="userSpaceOnUse"
width="20"
>
Expand All @@ -3319,7 +3325,7 @@ exports[`snapshots should render correctly with locked lines 1`] = `
/>
</pattern>
<rect
fill="url(#cartesian-31-subdivision)"
fill="url(#cartesian-30-subdivision)"
height="20"
width="20"
/>
Expand Down Expand Up @@ -3353,7 +3359,7 @@ exports[`snapshots should render correctly with locked lines 1`] = `
</g>
</pattern>
<rect
fill="url(#cartesian-31)"
fill="url(#cartesian-30)"
height="640"
width="640"
x="-320"
Expand Down Expand Up @@ -4267,6 +4273,10 @@ exports[`snapshots should render correctly with locked lines 1`] = `
style="fill: #ffffff; opacity: 1; stroke: #B25071; stroke-width: 2;"
/>
</g>
<g
data-testid="movable-point__focusable-handle"
tabindex="0"
/>
<g
class="movable-line"
data-testid="movable-line"
Expand Down Expand Up @@ -4305,10 +4315,13 @@ exports[`snapshots should render correctly with locked lines 1`] = `
/>
</g>
<g
class="movable-point "
data-testid="movable-point__focusable-handle"
tabindex="0"
/>
<g
class="movable-point "
data-testid="movable-point"
style="--movable-point-color: #1865f2;"
tabindex="0"
>
<circle
class="movable-point-hitbox"
Expand Down Expand Up @@ -4340,10 +4353,9 @@ exports[`snapshots should render correctly with locked lines 1`] = `
/>
</g>
<g
class="movable-point "
class="movable-point "
data-testid="movable-point"
style="--movable-point-color: #1865f2;"
tabindex="0"
>
<circle
class="movable-point-hitbox"
Expand Down Expand Up @@ -4530,15 +4542,15 @@ exports[`snapshots should render correctly with locked points 1`] = `
>
<pattern
height="20"
id="cartesian-30"
id="cartesian-29"
patternUnits="userSpaceOnUse"
width="20"
x="0"
y="0"
>
<pattern
height="20"
id="cartesian-30-subdivision"
id="cartesian-29-subdivision"
patternUnits="userSpaceOnUse"
width="20"
>
Expand All @@ -4547,7 +4559,7 @@ exports[`snapshots should render correctly with locked points 1`] = `
/>
</pattern>
<rect
fill="url(#cartesian-30-subdivision)"
fill="url(#cartesian-29-subdivision)"
height="20"
width="20"
/>
Expand Down Expand Up @@ -4581,7 +4593,7 @@ exports[`snapshots should render correctly with locked points 1`] = `
</g>
</pattern>
<rect
fill="url(#cartesian-30)"
fill="url(#cartesian-29)"
height="640"
width="640"
x="-320"
Expand Down Expand Up @@ -5389,6 +5401,10 @@ exports[`snapshots should render correctly with locked points 1`] = `
r="6"
style="fill: #ffffff; opacity: 1; stroke: #447A53; stroke-width: 2;"
/>
<g
data-testid="movable-point__focusable-handle"
tabindex="0"
/>
<g
class="movable-line"
data-testid="movable-line"
Expand Down Expand Up @@ -5427,10 +5443,13 @@ exports[`snapshots should render correctly with locked points 1`] = `
/>
</g>
<g
class="movable-point "
data-testid="movable-point__focusable-handle"
tabindex="0"
/>
<g
class="movable-point "
data-testid="movable-point"
style="--movable-point-color: #1865f2;"
tabindex="0"
>
<circle
class="movable-point-hitbox"
Expand Down Expand Up @@ -5462,10 +5481,9 @@ exports[`snapshots should render correctly with locked points 1`] = `
/>
</g>
<g
class="movable-point "
class="movable-point "
data-testid="movable-point"
style="--movable-point-color: #1865f2;"
tabindex="0"
>
<circle
class="movable-point-hitbox"
Expand Down

0 comments on commit 4a59b85

Please sign in to comment.