Add focus trap and aria attributes to Dialog

theetrain commented 1 year ago

Nice work with these svelte actions. 🕶️

For the Dialog, please consider these accessibility features:

Add aria-modal="true" to opened dialog
Add role="dialog" to opened dialog
Ensure keyboard is focus-trapped within opened dialog

Reference: https://www.w3.org/WAI/ARIA/apg/example-index/dialog-modal/dialog

theetrain commented 1 year ago

Thanks for taking action, though there were some regressions:

Using Spacebar or Enter no longer submits buttons in the modals (observable in docs)
ESC does not dismiss the modal (maybe this wasn't already implemented, I didn't check)

It was likely caused by these lines that calls event.preventDefault() on all key actions:

https://github.com/CaptainCodeman/svelte-headlessui/blob/bcdc989fea6f880649d7a8ec8185d879def39efd/packages/lib/src/lib/internal/focus.ts#L41-L45

Maybe you can short-circuit the onKeyDown method when Tab or ESC are not pressed:

if (event.key !== 'Tab' && event.key !== 'Escape') return

A potential enhancement for later could be a parameter to opt-in to top-level modal placement (since we cannot make assumptions for where the user places their modal); that way you can leverage the inert attribute to disable other direct descendants of <body> as a means of having focus trapping:

<script lang="ts">
    import { createDialog } from 'svelte-headlessui'

    const dialog = createDialog({ label: 'Payment Success', topLevel: true })
</script>

<main inert>
  <!-- main user content -->
</main>
<div  /> <!-- modal goes here -->

CaptainCodeman commented 1 year ago

Ha, I forgot the bit that needs to actually test it's the tab key being pressed! (and ignore if not)

CaptainCodeman commented 1 year ago

Hopefully that fixes it for now and I'll look more into the inert / topLevel option you suggested.

CaptainCodeman / svelte-headlessui

Add focus trap and aria attributes to Dialog #3