DT-131 - improve keyboard navigation for modals #966
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
- add some support for screen readers - focus "close" button when modal opens
src/lib/holocene/modal.svelte
Outdated
| <div | ||
| class="body" | ||
| class:large | ||
| aria-modal="true" |
There was a problem hiding this comment.
| aria-modal="true" | |
| tabindex="-1" | |
| bind:this={modal} // add a modal (HTMLDivElement) variable | |
| role="dialog" | |
| aria-label={ariaLabel} // add an ariaLabel prop |
src/lib/holocene/modal.svelte
Outdated
| event.preventDefault(); | ||
| } | ||
| } else if (document.activeElement === confirmButton.buttonElement) { | ||
| closeButton.focus(); |
There was a problem hiding this comment.
There's an edge case here where if loading is true the focus can escape the modal.
What do you think about a more generic check for a first and last focusable element? e.g.
const focusable = modalElement.querySelectorAll('button');
const firstFocusable = focusable[0];
const lastFocusable = focusable[focusable.length - 1];
if (event.key === 'Tab') {
if (event.shiftKey) {
if (document.activeElement === firstFocusable) {
lastFocusable.focus();
event.preventDefault();
}
} else if (document.activeElement === lastFocusable) {
firstFocusable.focus();
event.preventDefault();
}
}This means we could also remove the focus export on the Button component.
There was a problem hiding this comment.
Ah nice catch. Yeah I had initially thought of a more generic solution like that, but I didn't love the idea of using a querySelectorAll to get all the focusable elements, I thought using bind with only the elements we wanted focusable was safer. That said, I don't know why we wouldn't want any button in a modal to not be focusable, so I think I like this solution better 👍
src/lib/holocene/modal.svelte
Outdated
| {#if !loading} | ||
| <div class="float-right p-6" on:click={cancelModal}> | ||
| <button | ||
| aria-label="Close Modal" |
There was a problem hiding this comment.
| aria-label="Close Modal" | |
| aria-label={cancelText} |
What was changed
tabandshift+tabScreen.Recording.2022-11-29.at.2.27.01.PM.mov
Why?
Checklist
Closes
How was this tested: