Add Key handling to modal example

[SteveALee]

This is a basic accessibility requirement for key access

• Escape key closes the modal
• Tab key does not cycle round focusable elements in background
• Close key gets focus

I'm fast 'n' playing loose with the check list as its an update to one of the examples

Before submitting the PR, please make sure you do the following

• It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
• This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
• Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

• Run the tests tests with npm test or yarn test)

[ynohtna]

Looks like a typo here with a missing s at the start of "</script>"

Suggested change

	</cript>
	</script>

[SteveALee]

yikes! you are correct. Thanks

[Rich-Harris]

Thank you! I wanted to better understand focus trapping, and disappeared down a very deep rabbit hole, and I am sorry to report that The Platform is still flaming hot garbage.

Specifically, I figured it'd be better to continue to allow tabbing, but restrict it to the contents of the modal. You'd think, given that this is a W3C recommended best practice, that there'd be a sensible declarative way to achieve this — <div trapfocus> or something. There isn't. I found many solutions that involve acres of JavaScript, but they're all a bit gross.

Eventually settled on this:

const handle_keydown = e => {
  if (e.key === 'Escape') {
    close();
    return;
  }

  if (e.key === 'Tab') {
    // trap focus
    const nodes = modal.querySelectorAll('*');
    const tabbable = Array.from(nodes).filter(n => n.tabIndex >= 0);

    let index = tabbable.indexOf(document.activeElement);
    if (index === -1 && e.shiftKey) index = 0;

    index += tabbable.length + (e.shiftKey ? -1 : 1);
    index %= tabbable.length;

    tabbable[index].focus();
    e.preventDefault();
  }
};

It's not perfect — it doesn't respect non-zero tabindexes (which you should avoid anyway, apparently), and it focuses <a> elements (this is the behaviour in most browsers, but not in Firefox on Mac OS, unless you have the right combination of system settings 🙄 ), but it feels better than all the others.

Also added role="dialog" and aria-modal, per the recommendations. fe34128

[SteveALee]

hey @Rich-Harris that is fantastic and a good implementation too, even restoring focus; plus ARIA!

Thanks for being so conscientious! It must have been conciderable work. I must admit I kept it really simple as not sure how much you would accept here. So I decided to just do enough to highlight there’s a need to handle key as well as pointer/touch.

a) examples generally show platform features and are not necessarily feature complete best practices (the details of which can obscure what is being demonstrated)
b) there’s only the single button on the modal.
c) ARIA not used in other examples (that I spotted)

I’m so pleased you took the basic accessibility requirement as something Svelte should promote with decent implementation! \0/

I wanted to better understand focus trapping,

that can actually happen anywhere when tabbing gets stuck, not just in modals

disappeared down a very deep rabbit hole, and I am sorry to report that The Platform is still flaming hot garbage.

Yeah! The need to roll your own modal (and combo box) are a massive source of bad implementations and accessibility bugs. Basically, devs have to reproduce missing browser behaviour and need to test all the hard discovered edge cases. We REALLY need the dialog element to be properly supported by browsers ASAP!

Specifically, I figured it'd be better to continue to allow tabbing, but restrict it to the contents of the modal.

Exactly right! I fudged it in the PR :-/

You'd think, given that this is a W3C recommended best practice, that there'd be a sensible declarative way to achieve this — <div trapfocus> or something.

I think it’s only required in a modal DIALOG, so again we need the element to be supported

I found many solutions that involve acres of JavaScript, but they're all a bit gross.

Can’t argue with that. We should not need to do it at all, but rather just let the browser take the strain!

Eventually settled on this:

That’s really nice modern ‘functional’ style. The kind that I try to write these days . I hadn’t realised the tab index property is computed, that saves a tonne of pain.

it focuses elements
good! :)

but not in Firefox on Mac OS, unless you have the right combination of system settings

weird. And then, Safari disables tab by default !!!??

but it feels better than all the others
I’m going to change my svelte modal to this :)
Also added role="dialog" and aria-modal, per the recommendations
1000 Bonus points!!!!

Again thanks for including this in the Modal example that other svelete devs will likely copy!

[SteveALee]

@LJWatson @hidde @yatil @stevefaulkner

Would any of you mind reviewing @Rich-Harris's code above for the official Sveltejs modal example?

It's a great opportunity to have an example that includes the very best practice.

I submitted a woeful PR to improve the key access / a11y and Rich rolled with it and came up with a solution that looks really good to me. It's based on the WAI ARIA example pattern for modal dialogs

Thanks (assuming you got notifications)

[hidde]

Woa, great work @Rich-Harris, I haven't seen an implementation this concise. Will look closer when time allows later in the week.

Re: a trapfocus attribute, if you had not seen it you may be interested in following along this focus meta bug.

One thing I noticed and perhaps you have too… I am not sure if

const tabbable = Array.from(nodes).filter(n => n.tabIndex >= 0);

is going to return the right set of elements. For example, elements with hidden attribute can have a tabindex of 0, but would not be tabbable.

[SteveALee]

:) Thanks @hidde!

For example, elements with hidden attribute can have a tabindex of 0, but would not be tabbable.

Oh, great spot! So should we also check the computed styles for non visible?

[hidde]

Could be a good way to handle it, but there may be more browser specifics.

The allyjs project has done some interesting research into this. Their tabbable has a number of browser specifics to take into account.

[SteveALee]

wow,, that's a lot of code!!! bring on the dialog element and we can have zero code.!!!

[hidde]

Yes, looking forward to some of the standards work around this to come to browsers. See also the inert attribute that could be added to anything that's not the modal.

[SteveALee]

@Rich-Harris I noticed the PR has not deployed yet - just in case you have CD in place and it glitched

[SteveALee]

@Rich-Harris Just copying you code and I noticed you switched to the autofocus attribute. I didn't use it as according to caniuse it doesn't work in Safari iOS (pah!) and there's a firefox scrolling bug (though should not be an issue for a modal)

Why oh why can we just use the declarative version :(