flekschas/svelte-simple-modal

Hidden and disabled elements break tabulation

jassenjj opened this issue · 8 comments

jassenjj commented

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

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

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

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!

flekschas commented

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

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

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

flekschas commented

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