flekschas / svelte-simple-modal

A simple, small, and content-agnostic modal for Svelte v3 and v4
https://svelte.dev/repl/b95ce66b0ef34064a34afc5c0249f313
MIT License
422 stars 30 forks source link

Hidden and disabled elements break tabulation #82

Closed jassenjj closed 2 years ago

jassenjj commented 2 years ago

When focus trap is used the tabulation in a modal breaks on disabled and hidden elements.

Is there any particular design consideration behind this?

The issue can be fixed with the following change in the filtration of the "tabbable" elements:

    if (Component && event.key === 'Tab' && !state.disableFocusTrap) {
      // trap focus
      const nodes = modalWindow.querySelectorAll('*');
      const tabbable = Array.from(nodes).filter((node) => node.tabIndex >= 0 && !node.hidden && !node.disabled)
      ...
jassenjj commented 2 years ago

By the way, another break happens for elements with display:none which can be handled with

const tabbable = Array.from(nodes).filter((node) => node.tabIndex >= 0 && !node.hidden && !node.disabled && !(node.display=="none"))
flekschas commented 2 years ago

Thanks a lot for finding and reporting the issue!

tabulation in a modal breaks on disabled and hidden elements

May I ask, in what way does the tabulation break? Do you have an example by chance?

I think your proposed change looks fine. Only minor thing, !(node.display=="none") can be simplified to node.display !== 'none'. Also, should it be node.display or node.style.display?

Do you want to setup a PR with a live demo (just branch https://svelte.dev/repl/033e824fad0a4e34907666e7196caec4?version=3)? Otherwise I can tackle it.

jassenjj commented 2 years ago

node.style.display is correct, although (surprisingly) node.display also works.

I started generalizing the issue and I found that there are other cases of "untabbable" elements. I doubt that I would be able to produce an exhaustive list.

For instance an input with type="hidden" is also not tabbable.

<input type="hidden" />

In the demo click on the "Show a Dialogue!" button, focus the first input and hit Tab repeatedly. https://svelte.dev/repl/6e32ef1170c84ccf9d2407e5e71dccbc?version=3.48.0

I really don't feel comfortable with a PR as I cannot set my environment for the standalone package.

My fix as of now is:

      const tabbable = Array.from(nodes).filter((node) => node.tabIndex >= 0 && 
        !node.hidden && 
        !node.disabled && 
        node.style.display!=="none" && 
        node.type!=="hidden")
        .sort((a, b) => a.tabIndex - b.tabIndex);

Thank you!

jassenjj commented 2 years ago

And here the Dialog works with the fix: https://svelte.dev/repl/726f6515fb8d4f8eb04065cd95ba7ae4?version=3.48.0

flekschas commented 2 years ago

Awesome thanks! I'll take a look tonight or tomorrow and can put together a PR.

One other approach we could also go down is using an existing library like https://github.com/focus-trap/tabbable. I'll think about whether we might want to use that library instead.

flekschas commented 2 years ago

I've opted for a lightweight dependency-free base line implementation with the ability to specify a custom function for doing the tabbability check (via a newly property called isTabbable).

Could you do me a favor and check if the current approach catches most of the issues? You could either check out the current master or play with https://svelte.dev/repl/b95ce66b0ef34064a34afc5c0249f313?version=3.48.0

If it works then I'll release a new version!

Massive thanks again for pointing this out and taking a stab at the addressing the problem!

jassenjj commented 2 years ago

It works with my list of isolated cases. Thank you very much for the prompt response.

flekschas commented 2 years ago

New version with the fix is out! Thanks again for reporting and tackling the problem 🎉