React-md: [FocusContainer] - Hidden input at the end breaks focus trap

[garrettn]

Description

If you have a dialog with inputs inside it, and the last input is type="hidden", then the focus trapping no longer works. If you tab forward, it breaks outside of the dialog. If you tab backward, it gets stuck at the beginning.

After some digging I discovered that the _focusables array that FocusContainer creates includes every input, including hidden ones. But hidden inputs can't receive focus. So as you're tabbing forward and FocusContainer is checking if you're on the last element in _focusables, it will never match, so the event is not prevented and focus breaks out of the container. If you're going backwards, it tries to focus the last element, but it can't, so it gets stuck there while it keeps trying.

Normally we could get around this by _not_ having a hidden input at the end, but as luck would have it, we're using a DataTable in the dialog, which has one built in, so it's a bit more complicated :smile:.

I think the fix would be to filter out hidden inputs when creating the _focusables array. It would also have to filter inputs hidden with CSS using display: none or visibility: hidden, if I understand correctly.

I'd be happy to make a PR if you think this is on the right track.

Thanks!

Link to a gist or code sample where the issue can be reproduced

https://stackblitz.com/edit/react-k4bekc?file=Demo.js

Version

• React: 16.1.1 (on Stackblitz, but also seen on 15.6.2 locally)
• React-MD: 1.3.2
• Browser: Firefox/Chromium

[mlaursen]

Oof. I'll patch this some time tomorrow with what I have in my "next" branch.

-const hrefables = ['a', 'area'].map(tag => `${tag}[href],`).join('');
-const disableables = ['button', 'input', 'textarea', 'select'].map(tag => `${tag}:not([disabled]),`).join('');
-const FOCUSABLE_QUERY = `${hrefables}${disableables}*[tabIndex]`;
+const BASE_FOCUSABLE_ELEMENTS = '[href],[tabindex]:not([tabindex="-1"]),input:not([disabled]):not([type="hidden"])';
+const FOCUSABLE_QUERY = ['button', 'textarea', 'select']
+  .reduce((queryString, element) => `${queryString},${element}:not([disabled])`, BASE_FOCUSABLE_ELEMENTS);

which is a bit closer to what it should be for "natural" browser tab behavior.

I'm a bit iffy about if I want to support filtering items out via class names, I think it would be better to disable or remove the tab index from other items. It probably doesn't hurt to add the css-filter feature in though. I'll think a bit more about it.

[mlaursen]

Sorry, ended up being a day late. Should be fixed and published now.

Right now I did not add a filter ability, but I think in the next minor release I will make it so that you can provide a focusQuery yourself which will do what we want.

[garrettn]

Works great! Thank you!