-
Notifications
You must be signed in to change notification settings - Fork 359
Improve keyboard experience for definition widget #3094
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
base: main
Are you sure you want to change the base?
Conversation
<full summary> Issue: <url or "none"> Test plan: <see below>
…board experience. We are enabling closing the definition widgets popover for tooltips in two new ways: hitting the escape key, tabbing out of the popover (forwards or backwards).
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +39 B (+0.01%) Total Size: 499 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (ab8930c) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3094If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3094If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3094 |
| <DefinitionConsumer> | ||
| {({activeDefinitionId, setActiveDefinitionId}) => ( | ||
| <Popover | ||
| dismissEnabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enables closing when hitting the escape key or tapping out.
|
|
||
| // Assert - Popover should be closed | ||
| expect(screen.queryByRole("dialog")).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend adding a test that shows that other keyboard events do not close the popover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a good idea! Any in particular you'd recommend?
mark-fitzgerald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for improving the accessibility of this widget!
ivyolamit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ealonas changes looks logical to me. I'm wondering if this accessibility needs be addressed in WonderBlock level since this is using PopoverContentCore. Though this is the closest I can find in the docs https://khan.github.io/wonder-blocks/?path=/docs/packages-popover-popover-accessibility--docs
| <View | ||
| onKeyDown={(e) => { | ||
| // Close popover when tabbing off the close button | ||
| // (the only tabbable element in the popover) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have anything that enforces that no other clickable elements appear in the popover, but the style guide for content creators says to only use basic text, so this should be safe within Khan Academy. I wonder if we need to do something more for other users.
aag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pretty good to me, though the solution might have to change once we move to Floating UI for the Popover. I'd like to get @jandrade to weigh in on the change, since he's working on the Floating UI migration.
Summary:
We are using the definition widget in Assessments for ELA (and probably Math at some point). These changes bring us closer to the accessibility requirements to be compliant with WCAG.
Issue: AX-2119
Test plan:
--
--
Without Screenreader
test-no-screenreader.mov
With Screenreader
test-with-screenreader.mov