Focus trap incorrectly finds tabbable elements

bouncehead13 commented 1 year ago

Summary:

The regex to determine focusable elements is not specific enough.

https://github.com/reactjs/react-modal/blob/v3.16.1/src/helpers/tabbable.js#L16

const tabbableNode = /input|select|textarea|button|object|iframe/;

In the world of web-components, it is very common to have these names be used in the element tag. Say I have an internal library or a library pulled from the community that is solely build with web-components. Unlike react, the web-component name shows as part of the DOM Tree.

<awesome-button>
  <button ...></button>
</awesome-button>

<awesome-input-field>
  <input ... />
</awesome-input-field>

The regex is matching both <awesome-button> and <button> as focusable. With this, the head and tail checks don't work correctly. https://github.com/reactjs/react-modal/blob/v3.16.1/src/helpers/scopeTab.js#L32-L38

Steps to reproduce:

Created a codepen https://codepen.io/bouncehead13/pen/WNabvGY where focus does not stay trap.

The general problem is all with the regex matcher being too broad with its checks.

/input|select|textarea|button|object|iframe/.test('button') -- GOOD
/input|select|textarea|button|object|iframe/.test('awesome-button') -- Should not match, but does
/input|select|textarea|button|object|iframe/.test('input') -- GOOD
/input|select|textarea|button|object|iframe/.test('awesome-input-field') -- Should not match, but does

Expected behavior:

Only true focusable nodes are allowed.

Proposed solution:

Instead of the regex checking as 'contains', change it to enforce 'full word' matches

// bad, checks nodeName is anywhere
const tabbableNode = /input|select|textarea|button|object|iframe/;

// good, matches whole nodeName `/^(...)$/`
const tabbableNode = /^(input|select|textarea|button|object|iframe)$/;

diasbruno commented 1 year ago

Actually, in order to get it right on web components, there are a little bit more work to be done. This will work for components that have this pattern, but maybe, form components are deep in the shadow dom tree.

bouncehead13 commented 1 year ago

@diasbruno From what I can tell, it’s working correctly with web components. The focusable element inside is found correctly. The core problem is finding too many elements, not all which are focusable.

diasbruno commented 1 year ago

https://codepen.io/diasbruno/pen/PoyqpWP I've made a few changes to your example and the trap works.

One thing I noticed, it should focus the first button, inside of the web component (WC :)), but it's probably matching with the WC tag first. After that, it works ok...

Here is an idea, can you write a test case showing the expected behavior?

bouncehead13 commented 1 year ago

Correct, your code pen works because the node name is free of any "reserved" words. I tried to explain, albeit poorly, when opening the issue. It finds the web component tag first, then the button next. For that reason, a few things happen

head === activeElement will never match because <button> will never be index 0... so it begins focusing outside the modal content aka not trapping
head is not actually a focusable element so when tail === activeElement... [0].focus() will never work.

Here is an idea, can you write a test case showing the expected behavior?

If that is asking for a PR, sure I can open something up with a fix and some tests. To me it's just a regex change, but maybe there's something else that I'm missing.

diasbruno commented 1 year ago

This is actually the correct behavior. The first focus is on the modal's content div, so the screen reader can read the attributes associated with it. Than, it must cycle through the elements inside of the content's div.

bouncehead13 commented 1 year ago

The behavior is broken, but I'm just now realizing the exact cause. Only a web component without a shadow-root is the core problem. React modal does correctly skip the web component "root" and dives into the shadow root when a shadow root exist.

When no shadow root is given, both the component and what's inside are found as focusable elements... and everything mentioned above is the outcome of that with broken accessibility controls.

Original code-pen with a conflicting DOM Node: https://codepen.io/bouncehead13/pen/WNabvGY

New code-pen with a conflicting web component node, but no shadow-root: https://codepen.io/bouncehead13/pen/PoyqEEZ

diasbruno commented 1 year ago

:thinking: interesting. Can you point out any references regarding this behavior?

bouncehead13 commented 1 year ago

So the code pens are based on a similar structure for an internal component library for my company, so no direct references I can point. In that library, a shadow root is not always used, which is definitely a thing in the web component space.

The suggested solution from the description should work, unless you see more edge cases. Happy to provide more examples or if you want to just talk through something.

diasbruno commented 1 year ago

Ohhhhh, i think I got it. But haven't seen any different behavior. When I open the modal and ask for the document.activeElement, it says div.react...content. When I hit the tab button, it goes and focus the right button on both cases.

https://github.com/reactjs/react-modal/blob/v3.16.1/src/helpers/tabbable.js#L83

Here we check if the element has a shadowRoot, if it has we go deeper on the element.

So, maybe a browser version issues or if you think you can make this fail, you can write a test against findTabbableDescendants.

bouncehead13 commented 1 year ago

@diasbruno I created a PR which outlines what I had in mind as a fix, along with tests and a new example page as well.

Let me know your thoughts!

Edit: I noticed some tests were failing before I even made any changes to the code. Not sure if you're aware or if I would need to fix them for my PR.

diasbruno commented 1 year ago

I had this problem before in another project, it's the node version. Probably it's using the latest version (on the action).

reactjs / react-modal