Add readOnly mode to interactive graphs to prevent keyboard interactions when question marked correct

[Myranae]

Summary:

Widgets are placed into a readOnly mode when they are part of an exercise that was answered correctly. This is to prevent a state where the question is marked correct, but the user has modified the graph to an incorrect state. Mouse interactions are prevented by adding an invisible div over the widget, but this does not prevent keyboard interactions. This change adds a readOnly property to graphs that reduces the tabIndex to -1 in instances where readOnly mode is active (when the question was answered correctly).

Issue: LEMS-2175

Test plan:

• Confirm all tests pass
• Checkout the branch associated with this PR. Run yarn storybook
• Confirm the graph in the readOnly mode story is non-interactive: http://localhost:6006/?path=/story/perseus-widgets-interactive-graph--polygon-with-mafs-read-only
• Compare the interactivity with the corresponding story not in readOnly mode: http://localhost:6006/?path=/story/perseus-widgets-interactive-graph--polygon-with-mafs
• Use the PR npm snapshot to test this in webapp with a ZND to confirm when a graph related exercise is answered correctly that the graph cannot be interacted with either via mouse or keyboard (don't see a snapshot, so maybe this isn't possible)

[github-actions]

Size Change: +64 B (+0.01%)

Total Size: 855 kB

Filename	Size	Change
packages/perseus/dist/es/index.js	413 kB	+64 B (+0.02%)

ℹ️ View Unchanged

Filename	Size
packages/kas/dist/es/index.js	38.3 kB
packages/kmath/dist/es/index.js	4.27 kB
packages/math-input/dist/es/index.js	80.3 kB
packages/math-input/dist/es/strings.js	1.73 kB
packages/perseus-core/dist/es/index.js	1.48 kB
packages/perseus-editor/dist/es/index.js	275 kB
packages/perseus-linter/dist/es/index.js	21.6 kB
packages/perseus/dist/es/strings.js	3.29 kB
packages/pure-markdown/dist/es/index.js	3.67 kB
packages/simple-markdown/dist/es/index.js	12.4 kB

_{compressed-size-action}

[khan-actions-bot]

Gerald

Required Reviewers

• @Khan/perseus for changes to .changeset/short-paws-suffer.md, packages/perseus/src/types.ts, packages/perseus/src/widgets/interactive-graph.tsx, packages/perseus/src/widgets/__stories__/interactive-graph.stories.tsx, packages/perseus/src/widgets/__tests__/interactive-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx, packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.test.tsx, packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/circle.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx, packages/perseus/src/widgets/interactive-graphs/reducer/use-graph-config.ts, packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-line.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-point-view.tsx

---

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.62%. Comparing base (8089560) to head (f415144).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1487      +/-   ##
==========================================
+ Coverage   69.86%   70.62%   +0.76%     
==========================================
  Files         509      513       +4     
  Lines      105491   105693     +202     
  Branches     7588    11430    +3842     
==========================================
+ Hits        73700    74649     +949     
+ Misses      31674    31044     -630     
+ Partials      117        0     -117     

Files	Coverage Δ
packages/perseus/src/widgets/interactive-graph.tsx	50.76% <100.00%> (-0.17%)	⬇️
...s/src/widgets/interactive-graphs/graphs/circle.tsx	98.46% <100.00%> (ø)
...eractive-graphs/graphs/components/movable-line.tsx	100.00% <100.00%> (ø)
...ve-graphs/graphs/components/movable-point-view.tsx	94.66% <100.00%> (+0.10%)	⬆️
.../src/widgets/interactive-graphs/graphs/polygon.tsx	90.47% <100.00%> (ø)
...seus/src/widgets/interactive-graphs/mafs-graph.tsx	99.42% <100.00%> (+<0.01%)	⬆️
...ets/interactive-graphs/reducer/use-graph-config.ts	100.00% <100.00%> (ø)
...widgets/interactive-graphs/stateful-mafs-graph.tsx	93.93% <100.00%> (+3.46%)	⬆️

... and 157 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 8089560...f415144. Read the comment docs.

[github-actions]

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1487

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

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

[benchristel]

This looks good! I'd prefer it if the hintMode || readOnly logic didn't have to be duplicated in multiple components. I left a comment inline describing one way to achieve that. But I don't think that will need another round of review.

[benchristel]

Thanks for documenting this!

[benchristel]

Optional suggestion: I wonder if we could make this more skimmable and save some duplicated code by refactoring to something like this:

export const PolygonWithMafsReadOnly = (
    args: StoryArgs,
): React.ReactElement => (
    <RendererWithDebugUI
        apiOptions={{...enableMafs, readOnly: true}}
        question={polygonQuestion}
    />
);

where enableMafs would be defined like this:

const enableMafs: ApiOptions = {
    flags: {
        mafs: {
            segment: true,
            polygon: true,
            // ... other flags as needed ...
        },
    },
}

The other stories could be refactored to use a similar pattern. That way, we wouldn't have to have the mafs flags defined in two places.

[Myranae]

Love it! I tried to refactor the {...mafsOptions} part, but wasn't quite sure how to do it. I think I might have been missing the double brackets. 🤦‍♀️ Thank you for pointing this out! Updated :)

[benchristel]

Since both hintMode and readOnly both come from the graph config and are always used together, what do you think about consolidating them? Creating the graph config would look like this:

<GraphConfigContext.Provider
  value={{
    // ...
    disableKeyboardInteraction: hintMode || readOnly,
  }}
>

And then individual components would just look at disableKeyboardInteraction. That way, the || logic wouldn't have to be duplicated in every component.

[Myranae]

I was also considering this. Consolidating some of these modes didn't seem like it was the direction we were going, so I wasn't sure something like this would be the right option. I love this though, so definitely will update. Hint mode disables all interaction though, right? Since readOnly also disables mouse interaction (just not as a result of this setting though), why don't we call it disableInteraction? Ooo, or is it called disableKeyboardInteraction because we use it for tabIndex?

[benchristel]

I don't recall exactly where the mouse interactions get disabled. If the code that disables them is reading from useGraphConfig() to decide what to do, then disableInteraction sounds good to me.

[Myranae]

I'll double check :)

[Myranae]

Looks like mouse interactions are disabled just below where we set the config. Doesn't look like it's reading from the config, so will stick with disableKeyboardInteractions.

[Myranae]

Updated! 😁

[jeremywiebe]

This all looks great. I wonder if it would be worth one test (maybe per graph type) that ensures when readOnly: true that nothing can be interacted with.

One idea could be to run the "answer graph correctly" tests with readOnly: true and ensure they fail because nothing can be moved with the keyboard?